potential array out-of-bounds access in AcpiDmDumpTable

[ColinIanKing]

Static analysis on today's tip of acpica found a potential issue in source/common/dmtable.c - not sure if this is a false positive but I though I should flag this up:

 928ACPI_STATUS
 929cpiDmDumpTable (
 930    UINT32                  TableLength,
 931    UINT32                  TableOffset,
 932    void                    *Table,
 933    UINT32                  SubtableLength,
 934    ACPI_DMTABLE_INFO       *Info)
 935{
 936    UINT8                   *Target;
 937    UINT32                  CurrentOffset;
 938    UINT32                  ByteLength;
 939    UINT8                   Temp8;
 940    UINT16                  Temp16;
 941    UINT32                  Temp32;
 942    UINT64                  Value;
 943    const AH_TABLE          *TableData;
 944    const char              *Name;
 945    BOOLEAN                 LastOutputBlankLine = FALSE;
 946    ACPI_STATUS             Status;
 947    char                    RepairedName[8];
 948
 949

    1. Condition !Info, taking false branch.

 950    if (!Info)
 951    {
 952        AcpiOsPrintf ("Display not implemented\n");
 953        return (AE_NOT_IMPLEMENTED);
 954    }
 955
 956    /* Walk entire Info table; Null name terminates */
 957

    2. Condition Info->Name, taking true branch.
    18. Condition Info->Name, taking true branch.
    30. Condition Info->Name, taking true branch.

 958    for (; Info->Name; Info++)
 959    {
 960        /*
 961         * Target points to the field within the ACPI Table. CurrentOffset is
 962         * the offset of the field from the start of the main table.
 963         */
 964        Target = ACPI_ADD_PTR (UINT8, Table, Info->Offset);
 965        CurrentOffset = TableOffset + Info->Offset;
 966
 967        /* Check for beyond subtable end or (worse) beyond EOT */
 968

    3. Condition SubtableLength, taking true branch.
    4. Condition Info->Offset >= SubtableLength, taking false branch.
    19. Condition SubtableLength, taking true branch.
    20. Condition Info->Offset >= SubtableLength, taking false branch.
    31. Condition SubtableLength, taking true branch.
    32. Condition Info->Offset >= SubtableLength, taking false branch.

 969        if (SubtableLength && (Info->Offset >= SubtableLength))
 970        {
 971            AcpiOsPrintf (
 972                "/**** ACPI subtable terminates early (Len %u) - "
 973                "may be older version (dump table) */\n", SubtableLength);
 974
 975            /* Move on to next subtable */
 976
 977            return (AE_OK);
 978        }
 979

    5. Condition CurrentOffset >= TableLength, taking false branch.
    21. Condition CurrentOffset >= TableLength, taking false branch.
    33. Condition CurrentOffset >= TableLength, taking false branch.

 980        if (CurrentOffset >= TableLength)
 981        {
 982            AcpiOsPrintf (
 983                "/**** ACPI table terminates "
 984                "in the middle of a data structure! (dump table) */\n");
 985            return (AE_BAD_DATA);
 986        }
 987
 988        /* Generate the byte length for this field */
 989

    6. Switch case value ACPI_DMT_UINT8.
    22. Switch case value ACPI_DMT_CHKSUM.
    34. Switch case value ACPI_DMT_CEDT.

 990        switch (Info->Opcode)
 991        {
 992        case ACPI_DMT_UINT8:
 993        case ACPI_DMT_CHKSUM:
 994        case ACPI_DMT_SPACEID:
 995        case ACPI_DMT_ACCWIDTH:
 996        case ACPI_DMT_CEDT:
 997        case ACPI_DMT_IVRS:
 998        case ACPI_DMT_IVRS_DE:
 999        case ACPI_DMT_GTDT:
1000        case ACPI_DMT_MADT:
1001        case ACPI_DMT_PCCT:
1002        case ACPI_DMT_PMTT:
1003        case ACPI_DMT_PPTT:
1004        case ACPI_DMT_RGRT:
1005        case ACPI_DMT_SDEV:
1006        case ACPI_DMT_SRAT:
1007        case ACPI_DMT_ASF:
1008        case ACPI_DMT_HESTNTYP:
1009        case ACPI_DMT_FADTPM:
1010        case ACPI_DMT_EINJACT:
1011        case ACPI_DMT_EINJINST:
1012        case ACPI_DMT_ERSTACT:
1013        case ACPI_DMT_ERSTINST:
1014        case ACPI_DMT_DMAR_SCOPE:
1015        case ACPI_DMT_VIOT:
1016
1017            ByteLength = 1;

    7. Breaking from switch.
    23. Breaking from switch.
    35. Breaking from switch.

1018            break;
1019
1020        case ACPI_DMT_UINT16:
1021        case ACPI_DMT_DMAR:
1022        case ACPI_DMT_HEST:
1023        case ACPI_DMT_HMAT:
1024        case ACPI_DMT_NFIT:
1025        case ACPI_DMT_PHAT:
1026
1027            ByteLength = 2;
1028            break;
1029
1030        case ACPI_DMT_UINT24:
1031
1032            ByteLength = 3;
1033            break;
1034
1035        case ACPI_DMT_UINT32:
1036        case ACPI_DMT_NAME4:
1037        case ACPI_DMT_SIG:
1038        case ACPI_DMT_LPIT:
1039        case ACPI_DMT_TPM2:
1040
1041            ByteLength = 4;
1042            break;
1043
1044        case ACPI_DMT_UINT40:
1045
1046            ByteLength = 5;
1047            break;
1048
1049        case ACPI_DMT_UINT48:
1050        case ACPI_DMT_NAME6:
1051
1052            ByteLength = 6;
1053            break;
1054
1055        case ACPI_DMT_UINT56:
1056        case ACPI_DMT_BUF7:
1057
1058            ByteLength = 7;
1059            break;
1060
1061        case ACPI_DMT_UINT64:
1062        case ACPI_DMT_NAME8:
1063
1064            ByteLength = 8;
1065            break;
1066
1067        case ACPI_DMT_BUF10:
1068
1069            ByteLength = 10;
1070            break;
1071
1072        case ACPI_DMT_BUF12:
1073
1074            ByteLength = 12;
1075            break;
1076
1077        case ACPI_DMT_BUF16:
1078        case ACPI_DMT_UUID:
1079
1080            ByteLength = 16;
1081            break;
1082
1083        case ACPI_DMT_BUF128:
1084
1085            ByteLength = 128;
1086            break;
1087
1088        case ACPI_DMT_UNICODE:
1089        case ACPI_DMT_BUFFER:
1090        case ACPI_DMT_RAW_BUFFER:
1091
1092            ByteLength = SubtableLength;
1093            break;
1094
1095        case ACPI_DMT_PMTT_VENDOR:
1096            /*
1097             * Calculate the length of the vendor data for the PMTT table:
1098             * Length = (Current Subtable ptr + Subtable length) -
1099             *          Start of the vendor data (Target)
1100             */
1101            ByteLength = ((ACPI_CAST_PTR (char, Table) +
1102                            (ACPI_CAST_PTR (ACPI_PMTT_HEADER, Table)->Length)) -
1103                            ACPI_CAST_PTR (char, Target));
1104            break;
1105
1106        case ACPI_DMT_STRING:
1107
1108            ByteLength = strlen (ACPI_CAST_PTR (char, Target)) + 1;
1109            break;
1110
1111        case ACPI_DMT_IVRS_UNTERMINATED_STRING:
1112
1113            ByteLength = ((ACPI_CAST_PTR (ACPI_IVRS_DEVICE_HID, Target) -1)->UidLength);
1114            break;
1115
1116        case ACPI_DMT_GAS:
1117
1118            if (!LastOutputBlankLine)
1119            {
1120                AcpiOsPrintf ("\n");
1121                LastOutputBlankLine = TRUE;
1122            }
1123
1124            ByteLength = sizeof (ACPI_GENERIC_ADDRESS);
1125            break;
1126
1127        case ACPI_DMT_HESTNTFY:
1128
1129            if (!LastOutputBlankLine)
1130            {
1131                AcpiOsPrintf ("\n");
1132                LastOutputBlankLine = TRUE;
1133            }
1134
1135            ByteLength = sizeof (ACPI_HEST_NOTIFY);
1136            break;
1137
1138        case ACPI_DMT_IORTMEM:
1139
1140            if (!LastOutputBlankLine)
1141            {
1142                LastOutputBlankLine = FALSE;
1143            }
1144
1145            ByteLength = sizeof (ACPI_IORT_MEMORY_ACCESS);
1146            break;
1147
1148        default:
1149
1150            ByteLength = 0;
1151            break;
1152        }
1153
1154        /* Check if we are beyond a subtable, or (worse) beyond EOT */
1155

    8. Condition CurrentOffset + ByteLength > TableLength, taking false branch.
    24. Condition CurrentOffset + ByteLength > TableLength, taking false branch.
    36. Condition CurrentOffset + ByteLength > TableLength, taking false branch.

1156        if (CurrentOffset + ByteLength > TableLength)
1157        {
1158            if (SubtableLength)
1159            {
1160                AcpiOsPrintf (
1161                    "/**** ACPI subtable terminates early - "
1162                    "may be older version (dump table) */\n");
1163
1164                /* Move on to next subtable */
1165
1166                return (AE_OK);
1167            }
1168
1169            AcpiOsPrintf (
1170                "/**** ACPI table terminates "
1171                "in the middle of a data structure! */\n");
1172            return (AE_BAD_DATA);
1173        }
1174

    9. Condition Info->Opcode == ACPI_DMT_EXTRA_TEXT, taking false branch.
    25. Condition Info->Opcode == ACPI_DMT_EXTRA_TEXT, taking false branch.
    37. Condition Info->Opcode == ACPI_DMT_EXTRA_TEXT, taking false branch.

1175        if (Info->Opcode == ACPI_DMT_EXTRA_TEXT)
1176        {
1177            AcpiOsPrintf ("%s", Info->Name);
1178            continue;
1179        }
1180
1181        /* Start a new line and decode the opcode */
1182
1183        AcpiDmLineHeader (CurrentOffset, ByteLength, Info->Name);
1184

    10. Switch case value ACPI_DMT_UINT8.
    26. Switch case value ACPI_DMT_CHKSUM.
    38. Switch case value ACPI_DMT_CEDT.

1185        switch (Info->Opcode)
1186        {
1187        /* Single-bit Flag fields. Note: Opcode is the bit position */
1188
1189        case ACPI_DMT_FLAG0:
1190        case ACPI_DMT_FLAG1:
1191        case ACPI_DMT_FLAG2:
1192        case ACPI_DMT_FLAG3:
1193        case ACPI_DMT_FLAG4:
1194        case ACPI_DMT_FLAG5:
1195        case ACPI_DMT_FLAG6:
1196        case ACPI_DMT_FLAG7:
1197
1198            AcpiOsPrintf ("%1.1X\n", (*Target >> Info->Opcode) & 0x01);
1199            break;
1200
1201        /* 2-bit Flag fields */
1202
1203        case ACPI_DMT_FLAGS0:
1204
1205            AcpiOsPrintf ("%1.1X\n", *Target & 0x03);
1206            break;
1207
1208        case ACPI_DMT_FLAGS1:
1209
1210            AcpiOsPrintf ("%1.1X\n", (*Target >> 1) & 0x03);
1211            break;
1212
1213        case ACPI_DMT_FLAGS2:
1214
1215            AcpiOsPrintf ("%1.1X\n", (*Target >> 2) & 0x03);
1216            break;
1217
1218        case ACPI_DMT_FLAGS4:
1219
1220            AcpiOsPrintf ("%1.1X\n", (*Target >> 4) & 0x03);
1221            break;
1222
1223        case ACPI_DMT_FLAGS4_0:
1224
1225            AcpiOsPrintf ("%1.1X\n", (*(UINT32 *)Target) & 0x0F);
1226            break;
1227
1228        case ACPI_DMT_FLAGS4_4:
1229
1230            AcpiOsPrintf ("%1.1X\n", (*(UINT32 *)Target >> 4) & 0x0F);
1231            break;
1232
1233        case ACPI_DMT_FLAGS4_8:
1234
1235            AcpiOsPrintf ("%1.1X\n", (*(UINT32 *)Target >> 8) & 0x0F);
1236            break;
1237
1238        case ACPI_DMT_FLAGS4_12:
1239
1240            AcpiOsPrintf ("%1.1X\n", (*(UINT32 *)Target >> 12) & 0x0F);
1241            break;
1242
1243        case ACPI_DMT_FLAGS16_16:
1244
1245            AcpiOsPrintf ("%4.4X\n", (*(UINT32 *)Target >> 16) & 0xFFFF);
1246            break;
1247
1248        /* Integer Data Types */
1249
1250        case ACPI_DMT_UINT8:
1251        case ACPI_DMT_UINT16:
1252        case ACPI_DMT_UINT24:
1253        case ACPI_DMT_UINT32:
1254        case ACPI_DMT_UINT40:
1255        case ACPI_DMT_UINT48:
1256        case ACPI_DMT_UINT56:
1257        case ACPI_DMT_UINT64:
1258            /*
1259             * Dump bytes - high byte first, low byte last.
1260             * Note: All ACPI tables are little-endian.
1261             */
1262            Value = 0;

    11. Condition Temp8 > 0, taking true branch.
    13. Condition Temp8 > 0, taking false branch.

1263            for (Temp8 = (UINT8) ByteLength; Temp8 > 0; Temp8--)
1264            {
1265                AcpiOsPrintf ("%2.2X", Target[Temp8 - 1]);
1266                Value |= Target[Temp8 - 1];
1267                Value <<= 8;

    12. Jumping back to the beginning of the loop.

1268            }
1269

    14. Condition !Value, taking true branch.
    15. Condition Info->Flags & 0x10, taking true branch.

1270            if (!Value && (Info->Flags & DT_DESCRIBES_OPTIONAL))
1271            {
1272                AcpiOsPrintf (" [Optional field not present]");
1273            }
1274
1275            AcpiOsPrintf ("\n");

    16. Breaking from switch.

1276            break;
1277
1278        case ACPI_DMT_BUF7:
1279        case ACPI_DMT_BUF10:
1280        case ACPI_DMT_BUF12:
1281        case ACPI_DMT_BUF16:
1282        case ACPI_DMT_BUF128:
1283            /*
1284             * Buffer: Size depends on the opcode and was set above.
1285             * Each hex byte is separated with a space.
1286             * Multiple lines are separated by line continuation char.
1287             */
1288            for (Temp16 = 0; Temp16 < ByteLength; Temp16++)
1289            {
1290                AcpiOsPrintf ("%2.2X", Target[Temp16]);
1291                if ((UINT32) (Temp16 + 1) < ByteLength)
1292                {
1293                    if ((Temp16 > 0) && (!((Temp16+1) % 16)))
1294                    {
1295                        AcpiOsPrintf (" \\\n"); /* Line continuation */
1296                        AcpiDmLineHeader (0, 0, NULL);
1297                    }
1298                    else
1299                    {
1300                        AcpiOsPrintf (" ");
1301                    }
1302                }
1303            }
1304
1305            AcpiOsPrintf ("\n");
1306            break;
1307
1308        case ACPI_DMT_UUID:
1309
1310            /* Convert 16-byte UUID buffer to 36-byte formatted UUID string */
1311
1312            (void) AcpiUtConvertUuidToString ((char *) Target, AslGbl_MsgBuffer);
1313
1314            AcpiOsPrintf ("%s\n", AslGbl_MsgBuffer);
1315            break;
1316
1317        case ACPI_DMT_STRING:
1318
1319            AcpiOsPrintf ("\"%s\"\n", ACPI_CAST_PTR (char, Target));
1320            break;
1321
1322        case ACPI_DMT_IVRS_UNTERMINATED_STRING:
1323
1324            AcpiOsPrintf ("\"%.*s\"\n", ByteLength, ACPI_CAST_PTR (char, Target));
1325            break;
1326
1327        /* Fixed length ASCII name fields */
1328
1329        case ACPI_DMT_SIG:
1330
1331            AcpiUtCheckAndRepairAscii (Target, RepairedName, 4);
1332            AcpiOsPrintf ("\"%.4s\"    ", RepairedName);
1333
1334            TableData = AcpiAhGetTableInfo (ACPI_CAST_PTR (char, Target));
1335            if (TableData)
1336            {
1337                AcpiOsPrintf (STRING_FORMAT, TableData->Description);
1338            }
1339            else
1340            {
1341                AcpiOsPrintf ("\n");
1342            }
1343            break;
1344
1345        case ACPI_DMT_NAME4:
1346
1347            AcpiUtCheckAndRepairAscii (Target, RepairedName, 4);
1348            AcpiOsPrintf ("\"%.4s\"\n", RepairedName);
1349            break;
1350
1351        case ACPI_DMT_NAME6:
1352
1353            AcpiUtCheckAndRepairAscii (Target, RepairedName, 6);
1354            AcpiOsPrintf ("\"%.6s\"\n", RepairedName);
1355            break;
1356
1357        case ACPI_DMT_NAME8:
1358
1359            AcpiUtCheckAndRepairAscii (Target, RepairedName, 8);
1360            AcpiOsPrintf ("\"%.8s\"\n", RepairedName);
1361            break;
1362
1363        /* Special Data Types */
1364
1365        case ACPI_DMT_CHKSUM:
1366
1367            /* Checksum, display and validate */
1368
1369            AcpiOsPrintf ("%2.2X", *Target);
1370            Temp8 = AcpiDmGenerateChecksum (Table,
1371                ACPI_CAST_PTR (ACPI_TABLE_HEADER, Table)->Length,
1372                ACPI_CAST_PTR (ACPI_TABLE_HEADER, Table)->Checksum);
1373

    27. Condition Temp8 != ((ACPI_TABLE_HEADER *)(void *)Table)->Checksum, taking true branch.

1374            if (Temp8 != ACPI_CAST_PTR (ACPI_TABLE_HEADER, Table)->Checksum)
1375            {
1376                AcpiOsPrintf (
1377                    "     /* Incorrect checksum, should be %2.2X */", Temp8);
1378            }
1379
1380            AcpiOsPrintf ("\n");

    28. Breaking from switch.

1381            break;
1382
1383        case ACPI_DMT_SPACEID:
1384
1385            /* Address Space ID */
1386
1387            AcpiOsPrintf (UINT8_FORMAT, *Target, AcpiUtGetRegionName (*Target));
1388            break;
1389
1390        case ACPI_DMT_ACCWIDTH:
1391
1392            /* Encoded Access Width */
1393
1394            Temp8 = *Target;
1395            if (Temp8 > ACPI_GAS_WIDTH_RESERVED)
1396            {
1397                Temp8 = ACPI_GAS_WIDTH_RESERVED;
1398            }
1399
1400            AcpiOsPrintf (UINT8_FORMAT, *Target, AcpiDmGasAccessWidth[Temp8]);
1401            break;
1402
1403        case ACPI_DMT_GAS:
1404
1405            /* Generic Address Structure */
1406
1407            AcpiOsPrintf (STRING_FORMAT, "Generic Address Structure");
1408            Status = AcpiDmDumpTable (TableLength, CurrentOffset, Target,
1409                sizeof (ACPI_GENERIC_ADDRESS), AcpiDmTableInfoGas);
1410            if (ACPI_FAILURE (Status))
1411            {
1412                return (Status);
1413            }
1414
1415            AcpiOsPrintf ("\n");
1416            LastOutputBlankLine = TRUE;
1417            break;
1418
1419        case ACPI_DMT_ASF:
1420
1421            /* ASF subtable types */
1422
1423            Temp16 = (UINT16) ((*Target) & 0x7F);  /* Top bit can be zero or one */
1424            if (Temp16 > ACPI_ASF_TYPE_RESERVED)
1425            {
1426                Temp16 = ACPI_ASF_TYPE_RESERVED;
1427            }
1428
1429            AcpiOsPrintf (UINT8_FORMAT, *Target, AcpiDmAsfSubnames[Temp16]);
1430            break;
1431
1432        case ACPI_DMT_CEDT:
1433
1434            /* CEDT subtable types */
1435
1436            Temp8 = *Target;

    39. Condition Temp8 > ACPI_CEDT_TYPE_RESERVED, taking true branch.

1437            if (Temp8 > ACPI_CEDT_TYPE_RESERVED)
1438            {

    40. assignment: Assigning: Temp8 = ACPI_CEDT_TYPE_RESERVED.

1439                Temp8 = ACPI_CEDT_TYPE_RESERVED;
1440            }
1441

    CID (#1 of 1): Out-of-bounds read (OVERRUN)
    41. overrun-local: Overrunning array AcpiDmCedtSubnames of 2 8-byte elements at element index 2 (byte offset 23) using index Temp8 (which evaluates to 2).

1442            AcpiOsPrintf (UINT8_FORMAT, *Target,
1443                AcpiDmCedtSubnames[Temp8]);
1444            break;
1445
1446        case ACPI_DMT_DMAR:

[SchmErik]

Hi Colin thanks for hte report. I'll take a look tomorrow

[SchmErik]

hi Colin, does the following code change fix this?

diff --git a/source/common/dmtable.c b/source/common/dmtable.c
index 1221540db..0f8c78695 100644
--- a/source/common/dmtable.c
+++ b/source/common/dmtable.c
@@ -187,6 +187,7 @@ static const char           *AcpiDmAsfSubnames[] =
 static const char           *AcpiDmCedtSubnames[] =
 {
     "CXL Host Bridge Structure",
+    "CXL Fixed Memory Window Structure",
     "Unknown Subtable Type"         /* Reserved */
 };

[ColinIanKing]

Yes that fix that issue.

I also get:

1618        case ACPI_DMT_MADT:
1619
1620            /* MADT subtable types */
1621
1622            Temp8 = *Target;

    39. Condition Temp8 > ACPI_MADT_TYPE_RESERVED, taking true branch.

1623            if (Temp8 > ACPI_MADT_TYPE_RESERVED)
1624            {

    40. assignment: Assigning: Temp8 = ACPI_MADT_TYPE_RESERVED.

1625                Temp8 = ACPI_MADT_TYPE_RESERVED;
1626            }
1627

   CID: Out-of-bounds read (OVERRUN)
   41. overrun-local: Overrunning array AcpiDmMadtSubnames of 17 8-byte elements at element index 17 (byte offset 143) using index Temp8 (which evaluates to 17).

1628            AcpiOsPrintf (UINT8_FORMAT, *Target,
1629                AcpiDmMadtSubnames[Temp8]);
1630            break;
1631

[SchmErik]

Thanks. I'm starting to see a pattern here.

I've spent some time experimenting with compile-time assertions but it tends to add some boilerplate.. Is there any way in C to force for array initializers to require that all array elements are present during compilation?

[ColinIanKing]

I've used this technique in stress-ng:

#define STRESS_CONCAT(a, b) a ## b
#define STRESS_CONCAT_EXPAND(a, b) STRESS_CONCAT(a, b)
#define STRESS_ASSERT(expr) \
        enum { STRESS_CONCAT_EXPAND(STRESS_ASSERT_AT_LINE_, __LINE__) = \
                1 / !!(expr) };

#define SIZEOF_ARRAY(a)         (sizeof(a) / sizeof(a[0]))

STRESS_ASSERT(SIZEOF_ARRAY(stressors) != STRESS_MAX)

where stressors is an array, STRESS_MAX is the last item in the enum that maps to the array stressors[]