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

["Ni, Ray" <ray.ni@intel.com>]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Ard Biesheuvel
> Sent: Tuesday, April 16, 2019 11:23 PM
> To: Wu, Hao A <hao.a.wu@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Wang, Jian J
> <jian.j.wang@intel.com>; glin@suse.com
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UefiBootManagerLib: fix
> crash on uninitialized ExitData
> 
> On Tue, 16 Apr 2019 at 23:07, Wu, Hao A <hao.a.wu@intel.com> wrote:
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > > Sent: Wednesday, April 17, 2019 4:30 AM
> > > To: devel@edk2.groups.io
> > > Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; glin@suse.com; Ard Biesheuvel
> > > Subject: [PATCH] MdeModulePkg/UefiBootManagerLib: fix crash on
> > > uninitialized ExitData
> > >
> > > 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. So ensure ExitData
> > > is initialized before the call.
> > >
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> > > b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> > > index fc8775dfa419..cf99de5b924a 100644
> > > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> > > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> > > @@ -502,6 +502,7 @@ EfiBootManagerDispatchDeferredImages (
> > >          // a 5 Minute period
> > >          //
> > >          gBS->SetWatchdogTimer (5 * 60, 0x0000, 0x00, NULL);
> > > +        ExitData = NULL;
> > >          Status = gBS->StartImage (ImageHandle, &ExitDataSize, &ExitData);
> > >          if (ExitData != NULL) {
> > >            FreePool (ExitData);
> >
> > Looks like the 'ExitData' is not being used at all here.
> >
> > Ray and Ard,
> >
> > Do you see any concern to just pass 'NULL' as the 3rd parameter
> > (eliminates 'ExitData') here?
> >
> 
> Yes, that would be even better, actually, I did not realize it was optional

I agree.
> 
>