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

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

On Wed, 17 Apr 2019 at 18:00, Wu, Hao A <hao.a.wu@intel.com> wrote:
>
> > -----Original Message-----
> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > Sent: Thursday, April 18, 2019 12:45 AM
> > To: Laszlo Ersek
> > Cc: edk2-devel-groups-io; Wang, Jian J; Wu, Hao A; Ni, Ray; Gary Ching-Pang
> > Lin
> > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/UefiBootManagerLib: fix
> > crash on uninitialized ExitData
> >
> > 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"
> > >
>
> Agree with Laszlo.
> With the refining the message,
> Reviewed-by: Hao Wu <hao.a.wu@intel.com>
>

Thanks all

Pushed as cfb29d2bda57..d43056888790