Smbios: improves robostness; adds maiformed SMBIOS data verification and more debug log

src/common/impl/smbiosHelper.c

@@ -53,6 +53,9 @@ bool ffIsSmbiosValueSet(FFstrbuf* value)
     ;
 }
 
+static bool smbiosTableInitialized = false;
+static FFSmbiosHeaderTable smbiosTable;
+
 const FFSmbiosHeader* ffSmbiosNextEntry(const FFSmbiosHeader* header)
 {
     const char* p = ((const char*) header) + header->Length;
@@ -68,6 +71,74 @@ const FFSmbiosHeader* ffSmbiosNextEntry(const FFSmbiosHeader* header)
     return (const FFSmbiosHeader*) (p + 1);
 }
 
+static bool parseSmbiosTable(const uint8_t* data, uint32_t length)
+{
+    const FFSmbiosHeader* endOfTable = NULL;
+
+    FF_DEBUG("Parsing SMBIOS table structures with length %u bytes", length);
+    FF_MAYBE_UNUSED int structureCount = 0, totalCount = 0;
+    for (
+        const FFSmbiosHeader* header = (const FFSmbiosHeader*) data;
+        (const uint8_t*) header + sizeof(FFSmbiosHeader) < (const uint8_t*) data + length;
+        header = ffSmbiosNextEntry(header)
+    )
+    {
+        ++totalCount;
+        endOfTable = header;
+
+        // This doesn't verify the entire structure (e.g. string section can still be truncated),
+        // but at least ensures the formatted section is valid and prevents infinite loops
+        // when the table is severely malformed.
+        if (__builtin_expect((const uint8_t*) header + header->Length > (const uint8_t*) data + length, false))
+        {
+            FF_DEBUG("Truncated SMBIOS structure at offset 0x%lx: length %u is too small",
+                (unsigned long)((const uint8_t*) header - data), header->Length);
+            break;
+        }
+
+        if (header->Type < FF_SMBIOS_TYPE_END_OF_TABLE)
+        {
+            if (!smbiosTable[header->Type])
+            {
+                smbiosTable[header->Type] = header;
+                FF_DEBUG("Found SMBIOS structure type %u, handle 0x%04X, length %u",
+                    header->Type, header->Handle, header->Length);
+                structureCount++;
+            }
+            else
+            {
+                FF_DEBUG("Duplicate SMBIOS structure type %u, handle 0x%04X, length %u",
+                    header->Type, header->Handle, header->Length);
+            }
+        }
+        else if (header->Type == FF_SMBIOS_TYPE_END_OF_TABLE)
+        {
+            FF_DEBUG("Reached SMBIOS end of type %u, handle 0x%04X, length %u",
+                header->Type, header->Handle, header->Length);
+            break;
+        }
+        else
+        {
+            FF_DEBUG("Found custom SMBIOS structure type %u, handle 0x%04X, length %u; ignoring",
+                header->Type, header->Handle, header->Length);
+        }
+    }
+
+    if (!endOfTable)
+    {
+        FF_DEBUG("No SMBIOS structures found in table");
+        return false;
+    }
+
+    FF_DEBUG("Parsed %d/%d SMBIOS structures, end-of-table (Type 127) %s",
+        structureCount,
+        totalCount,
+        endOfTable->Type == FF_SMBIOS_TYPE_END_OF_TABLE ? "found." : "not found! SMBIOS data may be malformed.");
+    smbiosTable[FF_SMBIOS_TYPE_END_OF_TABLE] = endOfTable;
+
+    return true;
+}
+
 #if defined(__linux__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__sun) || defined(__HAIKU__) || defined(__OpenBSD__) || defined(__GNU__)
 #include <fcntl.h>
 #include <sys/stat.h>
@@ -155,10 +226,10 @@ typedef union FFSmbiosEntryPoint
 const FFSmbiosHeaderTable* ffGetSmbiosHeaderTable()
 {
     static FFstrbuf buffer;
-    static FFSmbiosHeaderTable table;
 
-    if (buffer.chars == NULL)
+    if (!smbiosTableInitialized)
     {
+        smbiosTableInitialized = true;
         FF_DEBUG("Initializing SMBIOS buffer");
         ffStrbufInit(&buffer);
         #if !__HAIKU__ && !__OpenBSD__ && !__DragonFly__ && !__GNU__
@@ -419,29 +490,8 @@ const FFSmbiosHeaderTable* ffGetSmbiosHeaderTable()
         }
         #endif
 
-        FF_DEBUG("Parsing SMBIOS table structures");
-        FF_MAYBE_UNUSED int structureCount = 0;
-        for (
-            const FFSmbiosHeader* header = (const FFSmbiosHeader*) buffer.chars;
-            (const uint8_t*) header < (const uint8_t*) buffer.chars + buffer.length;
-            header = ffSmbiosNextEntry(header)
-        )
-        {
-            if (header->Type < FF_SMBIOS_TYPE_END_OF_TABLE)
-            {
-                if (!table[header->Type]) {
-                    table[header->Type] = header;
-                    FF_DEBUG("Found SMBIOS structure type %u, handle 0x%04X, length %u",
-                        header->Type, header->Handle, header->Length);
-                    structureCount++;
-                }
-            }
-            else if (header->Type == FF_SMBIOS_TYPE_END_OF_TABLE) {
-                FF_DEBUG("Reached end-of-table marker");
-                break;
-            }
-        }
-        FF_DEBUG("Parsed %d SMBIOS structures", structureCount);
+        if (!parseSmbiosTable((const uint8_t*) buffer.chars, buffer.length))
+            ffStrbufClear(&buffer);
     }
 
     if (buffer.length == 0) {
@@ -449,7 +499,7 @@ const FFSmbiosHeaderTable* ffGetSmbiosHeaderTable()
         return NULL;
     }
 
-    return &table;
+    return &smbiosTable;
 }
 #elif defined(_WIN32)
 #include "common/windows/nt.h"
@@ -469,10 +519,10 @@ typedef struct FFRawSmbiosData
 const FFSmbiosHeaderTable* ffGetSmbiosHeaderTable()
 {
     static SYSTEM_FIRMWARE_TABLE_INFORMATION* buffer;
-    static FFSmbiosHeaderTable table;
 
-    if (!buffer)
+    if (!smbiosTableInitialized)
     {
+        smbiosTableInitialized = true;
         FF_DEBUG("Initializing Windows SMBIOS buffer");
 
         FF_DEBUG("Querying system firmware table size with signature 'RSMB'");
@@ -509,32 +559,19 @@ const FFSmbiosHeaderTable* ffGetSmbiosHeaderTable()
         FF_DEBUG("Successfully retrieved SMBIOS data: version %u.%u, length %u bytes",
             rawData->SMBIOSMajorVersion, rawData->SMBIOSMinorVersion, rawData->Length);
 
-        FF_DEBUG("Parsing SMBIOS table structures");
-        FF_MAYBE_UNUSED int structureCount = 0;
-        for (
-            const FFSmbiosHeader* header = (const FFSmbiosHeader*) rawData->SMBIOSTableData;
-            (const uint8_t*) header < rawData->SMBIOSTableData + rawData->Length;
-            header = ffSmbiosNextEntry(header)
-        )
+        if (!parseSmbiosTable(rawData->SMBIOSTableData, rawData->Length))
         {
-            if (header->Type < FF_SMBIOS_TYPE_END_OF_TABLE)
-            {
-                if (!table[header->Type]) {
-                    table[header->Type] = header;
-                    FF_DEBUG("Found SMBIOS structure type %u, handle 0x%04X, length %u",
-                        header->Type, header->Handle, header->Length);
-                    structureCount++;
-                }
-            }
-            else if (header->Type == FF_SMBIOS_TYPE_END_OF_TABLE) {
-                FF_DEBUG("Reached end-of-table marker");
-                break;
-            }
+            free(buffer);
+            buffer = NULL;
+            return NULL;
         }
-        FF_DEBUG("Parsed %d SMBIOS structures", structureCount);
     }
 
-    return &table;
+    if (!buffer) {
+        FF_DEBUG("No valid SMBIOS data available");
+        return NULL;
+    }
+    return &smbiosTable;
 }
 #elif defined(__APPLE__)
 #include "common/apple/cf_helpers.h"
@@ -542,10 +579,10 @@ const FFSmbiosHeaderTable* ffGetSmbiosHeaderTable()
 const FFSmbiosHeaderTable* ffGetSmbiosHeaderTable()
 {
     static CFDataRef smbiosDataBuffer;
-    static FFSmbiosHeaderTable table;
 
-    if (smbiosDataBuffer == NULL)
+    if (!smbiosTableInitialized)
     {
+        smbiosTableInitialized = true;
         FF_DEBUG("Initializing SMBIOS buffer on Apple platform");
 
         FF_IOOBJECT_AUTO_RELEASE io_registry_entry_t registryEntry = IOServiceGetMatchingService(MACH_PORT_NULL, IOServiceMatching("AppleSMBIOS"));
@@ -553,7 +590,6 @@ const FFSmbiosHeaderTable* ffGetSmbiosHeaderTable()
         if (!registryEntry)
         {
             FF_DEBUG("IOServiceGetMatchingService() failed to find AppleSMBIOS");
-            smbiosDataBuffer = CFDataCreate(NULL, NULL, 0);
             return NULL;
         }
 
@@ -562,44 +598,31 @@ const FFSmbiosHeaderTable* ffGetSmbiosHeaderTable()
         if (!smbiosDataBuffer)
         {
             FF_DEBUG("IORegistryEntryCreateCFProperty() failed to get SMBIOS data");
-            smbiosDataBuffer = CFDataCreate(NULL, NULL, 0);
+            return NULL;
+        }
+        if (CFGetTypeID(smbiosDataBuffer) != CFDataGetTypeID())
+        {
+            FF_DEBUG("Unexpected SMBIOS data type: expected CFData");
+            CFRelease(smbiosDataBuffer);
+            smbiosDataBuffer = NULL;
             return NULL;
         }
 
         FF_DEBUG("Successfully retrieved SMBIOS data: %lu bytes", CFDataGetLength(smbiosDataBuffer));
-
-        FF_DEBUG("Parsing SMBIOS table structures");
-        FF_MAYBE_UNUSED int structureCount = 0;
-        for (
-            const FFSmbiosHeader* header = (const FFSmbiosHeader*) CFDataGetBytePtr(smbiosDataBuffer),
-            *end = (const FFSmbiosHeader*) ((const uint8_t*) header + CFDataGetLength(smbiosDataBuffer));
-            header < end;
-            header = ffSmbiosNextEntry(header)
-        )
+        if (!parseSmbiosTable((const uint8_t*) CFDataGetBytePtr(smbiosDataBuffer), (uint32_t) CFDataGetLength(smbiosDataBuffer)))
         {
-            if (header->Type < FF_SMBIOS_TYPE_END_OF_TABLE)
-            {
-                if (!table[header->Type]) {
-                    table[header->Type] = header;
-                    FF_DEBUG("Found SMBIOS structure type %u, handle 0x%04X, length %u",
-                        header->Type, header->Handle, header->Length);
-                    structureCount++;
-                }
-            }
-            else if (header->Type == FF_SMBIOS_TYPE_END_OF_TABLE) {
-                FF_DEBUG("Reached end-of-table marker");
-                break;
-            }
+            CFRelease(smbiosDataBuffer);
+            smbiosDataBuffer = NULL;
+            return NULL;
         }
-        FF_DEBUG("Parsed %d SMBIOS structures", structureCount);
     }
 
-    if (CFDataGetLength(smbiosDataBuffer) == 0)
+    if (!smbiosDataBuffer)
     {
         FF_DEBUG("No valid SMBIOS data available");
         return NULL;
     }
 
-    return &table;
+    return &smbiosTable;
 }
 #endif

src/common/smbiosHelper.h

@@ -63,14 +63,22 @@ typedef enum __attribute__((__packed__)) FFSmbiosType // : uint8_t
     FF_SMBIOS_TYPE_STRING_PROPERTY = 46,
     FF_SMBIOS_TYPE_INACTIVE = 126,
     FF_SMBIOS_TYPE_END_OF_TABLE = 127,
+    FF_SMBIOS_TYPE__MAX,
     // system- and OEM-specific information 128~256
 } FFSmbiosType;
 static_assert(sizeof(FFSmbiosType) == 1, "FFSmbiosType should be 1 byte");
 
 typedef struct FFSmbiosHeader
 {
+    // Type of SMBIOS structure. Do NOT test `Type == FF_SMBIOS_END_OF_TABLE` to determine the end of the table,
+    // as malformed tables may be missing the end-of-table marker.
+    // **Use FFSmbiosHeaderTable[FF_SMBIOS_TYPE_END_OF_TABLE] pointer instead.**
     FFSmbiosType Type;
+    // Length of formatted section, excluding unformatted string section
+    // Must be at least 4 (sizeof(FFSmbiosHeader)) to be valid
     uint8_t Length;
+    // Unique handle, used to reference this structure from other structures.
+    // Not guaranteed to be consistent across reboots or even multiple reads of the same table.
     uint16_t Handle;
 } __attribute__((__packed__)) FFSmbiosHeader;
 static_assert(sizeof(FFSmbiosHeader) == 4, "FFSmbiosHeader should be 4 bytes");
@@ -84,7 +92,7 @@ static inline const char* ffSmbiosLocateString(const char* start, uint8_t index
     return start;
 }
 
-typedef const FFSmbiosHeader* FFSmbiosHeaderTable[FF_SMBIOS_TYPE_END_OF_TABLE];
+typedef const FFSmbiosHeader* FFSmbiosHeaderTable[FF_SMBIOS_TYPE__MAX];
 
 const FFSmbiosHeader* ffSmbiosNextEntry(const FFSmbiosHeader* header);
 const FFSmbiosHeaderTable* ffGetSmbiosHeaderTable();

src/detection/cpucache/cpucache_shared.c

@@ -37,9 +37,12 @@ const char* ffDetectCPUCache(FFCPUCacheResult* result)
     if (!data)
         return "Cache information is not found in SMBIOS data";
 
-    for (; data->Header.Type == FF_SMBIOS_TYPE_CACHE_INFO;
-           data = (const FFSmbiosCacheInfo*) ffSmbiosNextEntry(&data->Header))
+    const FFSmbiosCacheInfo* endOfTable = (const FFSmbiosCacheInfo*) (*smbiosTable)[FF_SMBIOS_TYPE_END_OF_TABLE];
+    for (; data != endOfTable; data = (const FFSmbiosCacheInfo*) ffSmbiosNextEntry(&data->Header))
     {
+        if (data->Header.Type != FF_SMBIOS_TYPE_CACHE_INFO)
+            continue;
+
         bool enabled = !!(data->CacheConfiguration & (1 << 7));
         if (!enabled)
             continue;

src/detection/physicalmemory/physicalmemory_linux.c

@@ -75,8 +75,8 @@ const char* ffDetectPhysicalMemory(FFlist* result)
     if (!data)
         return "Memory device is not found in SMBIOS data";
 
-    for (; data->Header.Type != FF_SMBIOS_TYPE_END_OF_TABLE; // Ignores custom types (which is larger than 127)
-        data = (const FFSmbiosMemoryDevice*) ffSmbiosNextEntry(&data->Header))
+    const FFSmbiosMemoryDevice* endOfTable = (const FFSmbiosMemoryDevice*) (*smbiosTable)[FF_SMBIOS_TYPE_END_OF_TABLE];
+    for (; data != endOfTable; data = (const FFSmbiosMemoryDevice*) ffSmbiosNextEntry(&data->Header))
     {
         if (data->Header.Type != FF_SMBIOS_TYPE_MEMORY_DEVICE) continue;