* [PATCH] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData
@ 2019-04-16 20:29 Ard Biesheuvel
2019-04-17 2:10 ` [edk2-devel] " Gary Lin
2019-04-17 6:07 ` Wu, Hao A
0 siblings, 2 replies; 5+ messages in thread
From: Ard Biesheuvel @ 2019-04-16 20:29 UTC (permalink / raw)
To: devel; +Cc: jian.j.wang, hao.a.wu, ray.ni, glin, Ard Biesheuvel
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);
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData
2019-04-16 20:29 [PATCH] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData Ard Biesheuvel
@ 2019-04-17 2:10 ` Gary Lin
2019-04-17 6:07 ` Wu, Hao A
1 sibling, 0 replies; 5+ messages in thread
From: Gary Lin @ 2019-04-17 2:10 UTC (permalink / raw)
To: devel, ard.biesheuvel; +Cc: jian.j.wang, hao.a.wu, ray.ni
On Tue, Apr 16, 2019 at 01:29:35PM -0700, 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. So ensure ExitData is initialized
> before the call.
>
The fix works for me.
Tested-by: Gary Lin <glin@suse.com>
> 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);
> --
> 2.17.1
>
>
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData
2019-04-16 20:29 [PATCH] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData Ard Biesheuvel
2019-04-17 2:10 ` [edk2-devel] " Gary Lin
@ 2019-04-17 6:07 ` Wu, Hao A
2019-04-17 6:23 ` Ard Biesheuvel
1 sibling, 1 reply; 5+ messages in thread
From: Wu, Hao A @ 2019-04-17 6:07 UTC (permalink / raw)
To: Ni, Ray, Ard Biesheuvel, devel@edk2.groups.io; +Cc: Wang, Jian J, glin@suse.com
> -----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?
Best Regards,
Hao Wu
> --
> 2.17.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData
2019-04-17 6:07 ` Wu, Hao A
@ 2019-04-17 6:23 ` Ard Biesheuvel
2019-04-17 6:30 ` [edk2-devel] " Ni, Ray
0 siblings, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2019-04-17 6:23 UTC (permalink / raw)
To: Wu, Hao A; +Cc: Ni, Ray, devel@edk2.groups.io, Wang, Jian J, glin@suse.com
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData
2019-04-17 6:23 ` Ard Biesheuvel
@ 2019-04-17 6:30 ` Ni, Ray
0 siblings, 0 replies; 5+ messages in thread
From: Ni, Ray @ 2019-04-17 6:30 UTC (permalink / raw)
To: devel@edk2.groups.io, ard.biesheuvel@linaro.org, Wu, Hao A
Cc: Wang, Jian J, glin@suse.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.
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-04-17 6:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-04-16 20:29 [PATCH] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData Ard Biesheuvel
2019-04-17 2:10 ` [edk2-devel] " Gary Lin
2019-04-17 6:07 ` Wu, Hao A
2019-04-17 6:23 ` Ard Biesheuvel
2019-04-17 6:30 ` [edk2-devel] " Ni, Ray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox