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

["Gerd Hoffmann" <kraxel@redhat.com>]

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