[vbox-dev] [PATCH] Fix broken SMBios checksum calculation
Michal Necasek
michal.necasek at oracle.com
Mon Feb 11 11:09:29 GMT 2019
Hi Martin,
I think I now finally understand what's happening. The checksum
calculation code in DevPcBios.cpp is wrong... *but* it actually delivers
the correct result, as long as the BIOS image was run through 'biossums'
first!
Here's why: biossums calculates the checksum of the _DMI_ header and
the _SM_ header (at that point not containing final values), and then
calculates and updates the checksum of the entire BIOS image. Now, the
8-bit checksum of the _DMI_ header is 0, and the checksum of the entire
BIOS image is also 0. Which means that when you calculate the checksum
of the 15 _DMI_ header bytes, you'll get some value, and when you add
the checksum of the rest of the BIOS image, the result won't change.
You probably do not have the correct checksum of the entire BIOS
image, and that is why the _DMI_ checksum calculation produces a bogus
result for you. I have the checksum right, so the obviously wrong code
still gets the right answer.
I decided to throw the checksumming code in DevPcBios.cpp out
completely and replace it with the already existing
FwCommonPlantSmbiosAndDmiHdrs() function. That should produce the right
checksums regardless of whether the existing checksums in the BIOS image
are correct or not. Patch is attached, please let me know if it works
for you.
Cheers,
Michal
On 2/5/2019 5:57 PM, Martin Fleisz wrote:
> Hi Michal,
>
> when you look at the source of dmidecode that is not really the case.
> You are right that dmidecode first looks for the _SM_ header. But then
> it will check the _SM_ checksum, the _DMI_ signature and the _DMI_
> checksum (see
> https://github.com/mirror/dmidecode/blob/master/dmidecode.c#L5379). If
> any of these is wrong/missing it will fail with the error message from
> my previous mail.
> In both our cases the _DMI_ checksum is wrong (as you stated the code
> obviously calculates the wrong checksum).
> The _SM_ checksum is just wrong because of the incorrect value for the
> _DMI_ checksum. If the _DMI_ checksum was calculated correctly, the _SM_
> checksum is correct as it is now.
> I hope that also makes sense ... maybe we even mean the same thing
> anyways ... :-)
>
> Cheers, Martin
>
> On 05.02.2019 17:37, Michal Necasek wrote:
>>
>> Hi Martin,
>>
>> That's not what I'm saying. What I'm saying is that because my BIOS
>> image was run through biossums, I didn't see the dmidecode problem
>> because although the _DMI_ header checksum is wrong (incorrect
>> calculation in DevPcBios.cpp), it does not matter in practice because
>> dmidecode finds the _SM_ header first.
>>
>> In other words, for you both _SM_ and _DMI_ headers had bad
>> checksums, but for me only _DMI_ did. After your fix (which is
>> correct), the _SM_ header you have probably still has a bad checksum
>> but dmidecode does not care because it finds the valid _DMI_ header.
>>
>> I hope that made sense.
>>
>> - Michal
>>
>> On 2/5/2019 5:27 PM, Martin Fleisz wrote:
>>> Hi Michal,
>>>
>>> I don't think that biossums is the way you can fix this issue because in
>>> DevPcBios.cpp the _DMI_ table is patched dynamically (see
>>> https://www.virtualbox.org/browser/vbox/trunk/src/VBox/Devices/PC/DevPcBios.cpp#L1594
>>>
>>> where the number and size of the DMI tables is updated) and therefore
>>> the checksum must be re-calculated. The _SM_ table checksum is not
>>> affected by these changes (as long as the _DMI_ checksum is correctly).
>>>
>>> Cheers, Martin
>>>
>>> On 05.02.2019 17:02, Michal Necasek wrote:
>>>>
>>>> Hi Martin,
>>>>
>>>> Now it's making sense. The '_DMI_' table header checksum code is
>>>> wrong in DevPCBios.cpp. But dmidecode normally does not get that far,
>>>> because it finds the '_SM_' marker first, and that one has the right
>>>> checksum if the BIOS image got run through the 'biossums' tool. I
>>>> suspect that's not happening for you, and that is something which is
>>>> probably missing from our makefiles.
>>>>
>>>> I can't see into your repository on github. But you're probably right
>>>> and it's not doing any harm. Anyway, now I see how to reproduce the
>>>> problem, thanks.
>>>>
>>>> - Michal
>>>>
>>>> On 2/5/2019 4:09 PM, Martin Fleisz wrote:
>>>>> Hi Michal,
>>>>>
>>>>> I compile the OSE version (with some very small modifications in
>>>>> VBoxBiosAlternative386.asm
>>>>> <https://github.com/thincast/VirtualBox-RDP-Server/commit/98e7959e484a0145e39619dc6dbc40d3b9282520#diff-9c7472aa47554e70acbb1a4298b673e4>
>>>>>
>>>>>
>>>>> but that shouldn't do any harm to my understanding) and when running
>>>>> dmidecode on Ubuntu 18.04 it reports "# No SMBIOS nor DMI entry point
>>>>> found, sorry.". After applying the patch everything worked just fine.
>>>>> I checked with VirtualBox 6.0.4 and it works just fine out of the box
>>>>> ... so I guess either the checksum is somehow valid by accident or
>>>>> there
>>>>> is some code missing/different in the OSE repository.
>>>>>
>>>>> Best regards, Martin
>>>>>
>>>>> On 05.02.2019 15:48, Michal Necasek wrote:
>>>>>>
>>>>>> Hi Martin,
>>>>>>
>>>>>> The patch looks good, but I'm struggling to reproduce the problem.
>>>>>> The dmidecode utility in Ubuntu 18.04.1 and 18.10 (dmidecode version
>>>>>> 3.1 in both) does not generate any complaints. So what exactly does?
>>>>>>
>>>>>> Regards,
>>>>>> Michal
>>>>>>
>>>>>> On 2/4/2019 12:16 PM, Martin Fleisz wrote:
>>>>>>> When using legacy BIOS the Intermediate Checksum of the SMBIOS Entry
>>>>>>> Point Structure is calculated incorrectly.
>>>>>>>
>>>>>>> The calculation starts at the wrong offset (at the beginning of
>>>>>>> pu8PcBios rather than pu8PcBios[i]) and creates the sum over the
>>>>>>> whole pu8PcBios data instead of just the next 15 bytes.
>>>>>>>
>>>>>>> The patch below fixes checksum calculation and makes applications
>>>>>>> like dmidecode work again in the guest.
>>>>>>>
>>>>>>> The patch is contributed under the terms of the MIT license.
>>>>>>>
>>>>>>> Cheers, Martin
>>>>>>>
>>>>>>> diff --git src/VBox/Devices/PC/DevPcBios.cpp
>>>>>>> src/VBox/Devices/PC/DevPcBios.cpp
>>>>>>> index e967adff2f..7e56cc18fe 100644
>>>>>>> --- src/VBox/Devices/PC/DevPcBios.cpp
>>>>>>> +++ src/VBox/Devices/PC/DevPcBios.cpp
>>>>>>> @@ -1594,9 +1594,9 @@ static DECLCALLBACK(int)
>>>>>>> pcbiosConstruct(PPDMDEVINS pDevIns, int iInstance, PCF
>>>>>>> *(uint16_t*)&pThis->pu8PcBios[i + 0x06] =
>>>>>>> RT_H2LE_U16(cbDmiTables);
>>>>>>> *(uint16_t*)&pThis->pu8PcBios[i + 0x0C] =
>>>>>>> RT_H2LE_U16(cNumDmiTables);
>>>>>>> uint8_t u8Sum = 0;
>>>>>>> - for (unsigned j = 0; j < pThis->cbPcBios; j++)
>>>>>>> - if (j != i + 0x05)
>>>>>>> - u8Sum += pThis->pu8PcBios[j];
>>>>>>> + for (unsigned j = 0; j < 0x0F; j++)
>>>>>>> + if (j != 0x05)
>>>>>>> + u8Sum += pThis->pu8PcBios[i + j];
>>>>>>> pThis->pu8PcBios[i + 0x05] = -u8Sum;
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> vbox-dev mailing list
>>>>>>> vbox-dev at virtualbox.org
>>>>>>> https://www.virtualbox.org/mailman/listinfo/vbox-dev
>>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> vbox-dev mailing list
>>>>>> vbox-dev at virtualbox.org
>>>>>> https://www.virtualbox.org/mailman/listinfo/vbox-dev
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> vbox-dev mailing list
>>>>> vbox-dev at virtualbox.org
>>>>> https://www.virtualbox.org/mailman/listinfo/vbox-dev
>>>>>
>>>>
>>>> _______________________________________________
>>>> vbox-dev mailing list
>>>> vbox-dev at virtualbox.org
>>>> https://www.virtualbox.org/mailman/listinfo/vbox-dev
>>>
>>>
>>> _______________________________________________
>>> vbox-dev mailing list
>>> vbox-dev at virtualbox.org
>>> https://www.virtualbox.org/mailman/listinfo/vbox-dev
>>>
>>
>> _______________________________________________
>> vbox-dev mailing list
>> vbox-dev at virtualbox.org
>> https://www.virtualbox.org/mailman/listinfo/vbox-dev
>
>
> _______________________________________________
> vbox-dev mailing list
> vbox-dev at virtualbox.org
> https://www.virtualbox.org/mailman/listinfo/vbox-dev
>
-------------- next part --------------
Index: DevPcBios.cpp
===================================================================
--- DevPcBios.cpp (revision 128759)
+++ DevPcBios.cpp (working copy)
@@ -1581,23 +1581,20 @@
if (RT_FAILURE(rc))
return rc;
- for (unsigned i = 0; i < pThis->cbPcBios; i += 16)
+ /* Look for _SM_/_DMI_ anchor strings within the BIOS and replace the table headers. */
+ for (unsigned i = 0; i < (pThis->cbPcBios - 16); i += 16)
{
- /* If the DMI table is located at the expected place, patch the DMI table length and the checksum. */
if ( pThis->pu8PcBios[i + 0x00] == '_'
- && pThis->pu8PcBios[i + 0x01] == 'D'
+ && pThis->pu8PcBios[i + 0x01] == 'S'
&& pThis->pu8PcBios[i + 0x02] == 'M'
- && pThis->pu8PcBios[i + 0x03] == 'I'
- && pThis->pu8PcBios[i + 0x04] == '_'
- && *(uint16_t*)&pThis->pu8PcBios[i + 0x06] == 0)
+ && pThis->pu8PcBios[i + 0x03] == '_'
+ && pThis->pu8PcBios[i + 0x10] == '_'
+ && pThis->pu8PcBios[i + 0x11] == 'D'
+ && pThis->pu8PcBios[i + 0x12] == 'M'
+ && pThis->pu8PcBios[i + 0x13] == 'I'
+ && pThis->pu8PcBios[i + 0x14] == '_')
{
- *(uint16_t*)&pThis->pu8PcBios[i + 0x06] = RT_H2LE_U16(cbDmiTables);
- *(uint16_t*)&pThis->pu8PcBios[i + 0x0C] = RT_H2LE_U16(cNumDmiTables);
- uint8_t u8Sum = 0;
- for (unsigned j = 0; j < pThis->cbPcBios; j++)
- if (j != i + 0x05)
- u8Sum += pThis->pu8PcBios[j];
- pThis->pu8PcBios[i + 0x05] = -u8Sum;
+ FwCommonPlantSmbiosAndDmiHdrs(pDevIns, pThis->pu8PcBios + i, cbDmiTables, cNumDmiTables);
break;
}
}
More information about the vbox-dev
mailing list