[PATCH] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData

[PATCH] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData

[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

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

[Gary Lin]

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

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

[Wu, Hao A]

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

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

[Ard Biesheuvel]

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

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

[Ni, Ray]

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