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

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Wed, 17 Apr 2019 at 06:09, Laszlo Ersek <lersek@redhat.com> wrote:
>
> 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"
>

Good point.

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

Thanks Laszlo. I'll fix this before pushing the patch (assuming Hao is
ok with this v2)