> -----Original Message----- > From: [email protected] [mailto:[email protected]] On Behalf Of > Ard Biesheuvel > Sent: Tuesday, April 16, 2019 11:23 PM > To: Wu, Hao A <[email protected]> > Cc: Ni, Ray <[email protected]>; [email protected]; Wang, Jian J > <[email protected]>; [email protected] > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/UefiBootManagerLib: fix > crash on uninitialized ExitData > > On Tue, 16 Apr 2019 at 23:07, Wu, Hao A <[email protected]> wrote: > > > > > -----Original Message----- > > > From: Ard Biesheuvel [mailto:[email protected]] > > > Sent: Wednesday, April 17, 2019 4:30 AM > > > To: [email protected] > > > Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; [email protected]; 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 <[email protected]> > > > --- > > > 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. > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39231): https://edk2.groups.io/g/devel/message/39231 Mute This Topic: https://groups.io/mt/31204960/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
