Re: [edk2-devel] [PATCH 05/10] OvmfPkg/Sec: fix out-of-bounds reads

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 4/13/19 1:31 AM, Laszlo Ersek wrote:
> RH covscan justifiedly reports that accessing "EFI_FFS_FILE_HEADER.Size"
> and "EFI_COMMON_SECTION_HEADER.Size", which both are of type UINT8[3],
> through (UINT32*), is undefined behavior:
> 
>> Error: OVERRUN (CWE-119):
>> edk2-89910a39dcfd/OvmfPkg/Sec/SecMain.c:283: overrun-local: Overrunning
>> array of 3 bytes at byte offset 3 by dereferencing pointer
>> "(UINT32 *)File->Size".
>> #  281|
>> #  282|       File = (EFI_FFS_FILE_HEADER*)(UINTN) CurrentAddress;
>> #  283|->     Size = *(UINT32*) File->Size & 0xffffff;
>> #  284|       if (Size < (sizeof (*File) + sizeof (EFI_COMMON_SECTION_HEADER))) {
>> #  285|         return EFI_VOLUME_CORRUPTED;
>>
>> Error: OVERRUN (CWE-119):
>> edk2-89910a39dcfd/OvmfPkg/Sec/SecMain.c:614: overrun-local: Overrunning
>> array of 3 bytes at byte offset 3 by dereferencing pointer
>> "(UINT32 *)File->Size".
>> #  612|
>> #  613|       File = (EFI_FFS_FILE_HEADER*)(UINTN) CurrentAddress;
>> #  614|->     Size = *(UINT32*) File->Size & 0xffffff;
>> #  615|       if (Size < sizeof (*File)) {
>> #  616|         return EFI_NOT_FOUND;
>>
>> Error: OVERRUN (CWE-119):
>> edk2-89910a39dcfd/OvmfPkg/Sec/SecMain.c:639: overrun-local: Overrunning
>> array of 3 bytes at byte offset 3 by dereferencing pointer
>> "(UINT32 *)Section->Size".
>> #  637|         Section = (EFI_COMMON_SECTION_HEADER*)(UINTN) CurrentAddress;
>> #  638|
>> #  639|->       Size = *(UINT32*) Section->Size & 0xffffff;
>> #  640|         if (Size < sizeof (*Section)) {
>> #  641|           return EFI_NOT_FOUND;
> 
> Fix these by invoking the FFS_FILE_SIZE() and SECTION_SIZE() macros, which
> by now have been fixed too.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710
> Issue: scan-1008.txt
> Issue: scan-1009.txt
> Issue: scan-1010.txt
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/Sec/SecMain.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index 18a89c649fd4..3914355cd17b 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -269,17 +269,17 @@ FindFfsFileAndSection (
>    for (EndOfFile = CurrentAddress + Fv->HeaderLength; ; ) {
>  
>      CurrentAddress = (EndOfFile + 7) & ~(7ULL);
>      if (CurrentAddress > EndOfFirmwareVolume) {
>        return EFI_VOLUME_CORRUPTED;
>      }
>  
>      File = (EFI_FFS_FILE_HEADER*)(UINTN) CurrentAddress;
> -    Size = *(UINT32*) File->Size & 0xffffff;
> +    Size = FFS_FILE_SIZE (File);
>      if (Size < (sizeof (*File) + sizeof (EFI_COMMON_SECTION_HEADER))) {
>        return EFI_VOLUME_CORRUPTED;
>      }
>  
>      EndOfFile = CurrentAddress + Size;
>      if (EndOfFile > EndOfFirmwareVolume) {
>        return EFI_VOLUME_CORRUPTED;
>      }
> @@ -600,17 +600,17 @@ FindImageBase (
>    for (EndOfFile = CurrentAddress + BootFirmwareVolumePtr->HeaderLength; ; ) {
>  
>      CurrentAddress = (EndOfFile + 7) & 0xfffffffffffffff8ULL;
>      if (CurrentAddress > EndOfFirmwareVolume) {
>        return EFI_NOT_FOUND;
>      }
>  
>      File = (EFI_FFS_FILE_HEADER*)(UINTN) CurrentAddress;
> -    Size = *(UINT32*) File->Size & 0xffffff;
> +    Size = FFS_FILE_SIZE (File);
>      if (Size < sizeof (*File)) {
>        return EFI_NOT_FOUND;
>      }
>  
>      EndOfFile = CurrentAddress + Size;
>      if (EndOfFile > EndOfFirmwareVolume) {
>        return EFI_NOT_FOUND;
>      }
> @@ -625,17 +625,17 @@ FindImageBase (
>      //
>      // Loop through the FFS file sections within the FFS file
>      //
>      EndOfSection = (EFI_PHYSICAL_ADDRESS)(UINTN) (File + 1);
>      for (;;) {
>        CurrentAddress = (EndOfSection + 3) & 0xfffffffffffffffcULL;
>        Section = (EFI_COMMON_SECTION_HEADER*)(UINTN) CurrentAddress;
>  
> -      Size = *(UINT32*) Section->Size & 0xffffff;
> +      Size = SECTION_SIZE (Section);
>        if (Size < sizeof (*Section)) {
>          return EFI_NOT_FOUND;
>        }
>  
>        EndOfSection = CurrentAddress + Size;
>        if (EndOfSection > EndOfFile) {
>          return EFI_NOT_FOUND;
>        }
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>