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

["Michael Brown" <mcb30@ipxe.org>]

On 14/02/2023 13:01, 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.

Fair enough.  But in that case it should be used in a way that makes it 
clear what it's actually doing.  Specifically, the check for

   if (... || (SafeIntResult < (UINTN)Smbios.Raw)) {
     ...
   }

then becomes meaningless, since the whole point of SafeUintnAdd() is 
that it cannot result in this kind of unsigned integer wraparound.  The 
code as modified by this patch is *harder* to understand, because the 
reader has to dig through to figure out why this check still exists, 
look at the implementation of SafeUintnAdd() to see what is meant by 
"safe" in this context, and finally come to the conclusion that the 
remaining underflow check is redundant code that should have been removed.

The reader is also going to be confused by the fact that the code as 
modified by this patch then contains two separate methods for checking 
for integer overflows, since the patch does not similarly update the 
very similar code found almost immediately afterwards:

   if ((Smbios.Raw + Smbios.Hdr->Length + 2 * sizeof (UINT8) > 
SmbiosEnd.Raw) ||
         (Smbios.Raw + Smbios.Hdr->Length + 2 * sizeof (UINT8) < 
Smbios.Raw))
     {
       return EFI_INVALID_PARAMETER;
     }


Lastly: the actual operation being made safe here is "check that buffer 
contains at least this much data remaining".  This is most obviously 
done by calculating the remaining buffer space (i.e. 
(UINTN)(SmbiosEnd.Raw - Smbios.Raw)) and comparing against that.  That 
logic is clear, simple, and obviously correct at first glance.  Using 
the SafeUintnAdd() call does something that *is* mathematically 
equivalent to this check, but where the logic is much harder to parse.


I'd prefer it if the code were updated to avoid SafeUintnAdd() 
altogether.  But if not, then at a minimum the redundant check should be 
removed, and the calculation involving Smbios.Hdr->Length should also be 
updated to use SafeUintnAdd().

Thanks,

Michael