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

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----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>

Best Regards,
Hao Wu

> 
> 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)