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

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

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?

The almost-identical check performed in the code immediately below (that 
takes Smbios.Hdr->Length into account) should also be updated to e.g.:

   if ((UINTN)(SmbiosEnd.Raw - Smbios.Raw) < (Smbios.Hdr->Length + 2)) {
     ...
   }

Thanks,

Michael