Re: [edk2-devel] [PATCH v3 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts

["Michael Kubacki" <mikuback@linux.microsoft.com>]

Either approach works for me.

I understand the desire to avoid code bloat that comes with the library.

The most common classes of issues I see at the moment are asserts being 
misused for error handling (which is significant), general issues with 
integer conversion/evaluation, and unsafe arithmetic operations.

I suppose this is in the spirit of Gerd's comment as I thought 
additional library usage might increase awareness to help with the latter.

Do the MdeModulePkg / SMBIOS maintainers have a preference?

Thanks,
Michael

On 2/14/2023 8:01 AM, Gerd Hoffmann wrote:
> On Mon, Feb 13, 2023 at 04:15:30PM +0000, Michael Brown wrote:
>> On 13/02/2023 15:48, Michael Kubacki wrote:
>>> @@ -1608,9 +1610,12 @@ ParseAndAddExistingSmbiosTable (
>>>        //
>>>        // Make sure not to access memory beyond SmbiosEnd
>>>        //
>>> -    if ((Smbios.Raw + sizeof (SMBIOS_STRUCTURE) > SmbiosEnd.Raw) ||
>>> -        (Smbios.Raw + sizeof (SMBIOS_STRUCTURE) < Smbios.Raw))
>>> -    {
>>> +    Status = SafeUintnAdd ((UINTN)Smbios.Raw, sizeof (SMBIOS_STRUCTURE), &SafeIntResult);
>>> +    if (EFI_ERROR (Status)) {
>>> +      return EFI_INVALID_PARAMETER;
>>> +    }
>>> +
>>> +    if ((SafeIntResult > (UINTN)SmbiosEnd.Raw) || (SafeIntResult < (UINTN)Smbios.Raw)) {
>>>          return EFI_INVALID_PARAMETER;
>>>        }
>>
>> Could this not be expressed much more cleanly as just:
>>
>>    if ((UINTN)(SmbiosEnd.Raw - Smbios.Raw) < sizeof (SMBIOS_STRUCTURE)) {
>>      return EFI_INVALID_PARAMETER;
>>    }
>>
>> without the need to hide a basic arithmetic operation behind an ugly wrapper
>> function and drag in an external library?
> 
> Well, the advantage of using SafeIntLib is that (a) it is obvious even
> to untrained code readers what is going on here, and (b) it is hard to
> get it wrong.
> 
> When looking at the quality some of the patches being posted to the list
> have I'd say that is important to consider even if you personally have
> no problems avoiding integer overflows without the help of SafeIntLib.
> 
> take care,
>    Gerd