Searching over uninitialized data

[gabonator]

I have following scenario: I prepend smbios snapshot with proper MDRSMBIOSHeader structure at the beginning of /var/lib/smbios/smbios2 with following script on bmc/container side:

mkdir -p /var/lib/smbios
size="$(wc -c < smbios_snapshot.bin)"
printf "%.2x" 1 | xxd -r -p > /var/lib/smbios/smbios2
printf "%.2x" 2 | xxd -r -p >> /var/lib/smbios/smbios2
printf "%.8x" 0 | xxd -r -p >> /var/lib/smbios/smbios2
printf "%.8x" ${size} | sed -E 's/(..)(..)(..)(..)/\4\3\2\1/' | xxd -r -p >> /var/lib/smbios/smbios2
cat smbios_snapshot.bin >> /var/lib/smbios/smbios2

My first problem was that smbios-mdr requires smbios tables of version 3.2 or higher, whereas my snapshot is just 3.1. I was able to add support for this version, but noticed that MDRV2 constructor does not clear the smbiosTableStorage buffer which is used to hold the smbios data from /var/lib/smbios2 and MDRV2::checkSMBIOSVersion scans whole buffer:

https://github.com/openbmc/smbios-mdr/blob/e05a542324fcac40c9ff100287302ee92ed03621/src/mdrv2.cpp#L657-L665

Either the real smbios table buffer size should be used in std::string buffer(dataIn, dataIn + smbiosTableStorageSize) or the buffer should be cleared in MDRV2 constructor:

diff --git a/include/mdrv2.hpp b/include/mdrv2.hpp
index c2cf038..671660e 100644
--- a/include/mdrv2.hpp
+++ b/include/mdrv2.hpp
@@ -87,7 +87,8 @@ class MDRV2 :

         std::copy(smbiosTableId.begin(), smbiosTableId.end(),
                   smbiosDir.dir[smbiosDirIndex].common.id.dataInfo);
-
+        // prevent searching over uninitialized data in checkSMBIOSVersion
+        std::memset(smbiosTableStorage, 0, smbiosTableStorageSize);
         smbiosDir.dir[smbiosDirIndex].dataStorage = smbiosTableStorage;

         agentSynchronizeData();

Even after this change the checkSMBIOSVersion is not safe in cases where the _SM_ token is part of the strings section or is present somewhere in the dump.

Anyway, if anyone is struggling with smbios 3.1 support, here is a patch to enable it - it is just a workaround which skips the header, it works only for dmidecode dumps where Structure table address (qword attribute @+0x10) holds the size of the header:

diff --git a/include/smbios_mdrv2.hpp b/include/smbios_mdrv2.hpp
index e77abf4..9aedd96 100644
--- a/include/smbios_mdrv2.hpp
+++ b/include/smbios_mdrv2.hpp
@@ -169,8 +169,8 @@ static constexpr const char* pciePath =
 static constexpr const char* systemPath =
     "/xyz/openbmc_project/inventory/system/chassis/motherboard/bios";

-constexpr std::array<SMBIOSVersion, 3> supportedSMBIOSVersions{
-    SMBIOSVersion{3, 2}, SMBIOSVersion{3, 3}, SMBIOSVersion{3, 5}};
+constexpr std::array<SMBIOSVersion, 4> supportedSMBIOSVersions{
+    SMBIOSVersion{3, 1}, SMBIOSVersion{3, 2}, SMBIOSVersion{3, 3}, SMBIOSVersion{3, 5}};

 typedef enum
 {
@@ -225,6 +226,15 @@ static inline uint8_t* getSMBIOSTypePtr(uint8_t* smbiosDataIn, uint8_t typeId,
     {
         return nullptr;
     }
+    // skip smbios header
+    if (smbiosDataIn[0] == '_' && smbiosDataIn[1] == 'S' && smbiosDataIn[2] == 'M' &&
+        smbiosDataIn[3] == '3' && smbiosDataIn[4] == '_')
+    {
+      uint64_t structureTableAddress = *reinterpret_cast<uint64_t*>(smbiosDataIn + 0x10);
+      if (structureTableAddress < 128)
+        smbiosDataIn += structureTableAddress;
+    }
+
     char* smbiosData = reinterpret_cast<char*>(smbiosDataIn);
     while ((*smbiosData != '\0') || (*(smbiosData + 1) != '\0'))
     {

[Krellan]

While investigating some corruption, I noticed something similar, and made a fix: https://gerrit.openbmc.org/c/openbmc/smbios-mdr/+/69322

[Krellan]

I also made https://gerrit.openbmc.org/c/openbmc/smbios-mdr/+/69457 for adding SMBIOS version 3.4 support. I would add 3.1 at the same time, but I have no 3.1 machine to test on.