Re: [edk2-devel] [PATCH v2] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData

["Laszlo Ersek" <lersek@redhat.com>]

On 04/17/19 08:40, Ard Biesheuvel wrote:
> As reported by Gary, the recent LoadImage/StartImage changes to
> accommodate dispatching PE/COFF images built for foreign architectures
> may result in a crash when loading an IA32 option ROM into a X64 VM
> running OVMF:
> 
>   Loading driver at 0x0007E537000 EntryPoint=0x0007E53C06D 8086100e.efi
>   InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 7F003B98
>   ProtectUefiImageCommon - 0x7F002BC0
>     - 0x000000007E537000 - 0x000000000009F900
>   Image type IA32 can't be started on X64 UEFI system.
>   ASSERT MdeModulePkg/Core/Dxe/Mem/Pool.c(698): Head->Signature == ((('p') |
>               ('h' << 8)) | ((('d') | ('0' << 8)) << 16)) || Head->Signature
>               == ((('p') | ('h' << 8)) | ((('d') | ('1' << 8)) << 16))
> 
> This turns out to be caused by the deferred image loading code in BDS,
> which doesn't check the result code of gBS->StartImage(), and ends up
> trying to free an uninitialized pointer.

StartImage() can return an error status for one of two reasons:
- StartImage() itself fails, or
- StartImage() actually transfers control to the image, but then the
image exits with an error status.

In the latter case, we have two further branches:
- the image produces the error status by returning from its entry point
function, or
- the image calls gBS->Exit().

In the last case, it is possible that the image exits with an error
*and* returns some ExitData that needs freeing. (In fact gBS->Exit() can
produce ExitData only if it returns an error.)

Therefore, a failure status returned by gBS->StartImage() must not
prevent an attempt to free a non-NULL ExitData.

My point being... the patch is correct, IMO, but the above paragraph of
the commit message has not been updated from v1, and it still suggests
(to me anyway) that things could be improved by adding a check on
"Status". That's not the case, IMO.

So here's what I suggest: before pushing the patch, please simply remove
the fragment

  "doesn't check the result code of gBS->StartImage(), and"

With that:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

> 
> Given that ExitData is never actually used, let's just get rid of it
> entirely. While we're at it, drop the pointless assignment of Status
> as well.
> 
> Tested-by: Gary Lin <glin@suse.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: drop ExitData entirely instead of initializing it to NULL
> 
>  MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> index fc8775dfa419..6b8fb4d92461 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> @@ -439,8 +439,6 @@ EfiBootManagerDispatchDeferredImages (
>    UINTN                              ImageSize;
>    BOOLEAN                            BootOption;
>    EFI_HANDLE                         ImageHandle;
> -  UINTN                              ExitDataSize;
> -  CHAR16                             *ExitData;
>    UINTN                              ImageCount;
>    UINTN                              LoadCount;
>  
> @@ -502,10 +500,7 @@ EfiBootManagerDispatchDeferredImages (
>          // a 5 Minute period
>          //
>          gBS->SetWatchdogTimer (5 * 60, 0x0000, 0x00, NULL);
> -        Status = gBS->StartImage (ImageHandle, &ExitDataSize, &ExitData);
> -        if (ExitData != NULL) {
> -          FreePool (ExitData);
> -        }
> +        gBS->StartImage (ImageHandle, NULL, NULL);
>  
>          //
>          // Clear the Watchdog Timer after the image returns.
>