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

[PATCH v2] 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.

Given that ExitData is never actually used, let's just get rid of it
entirely. While we're at it, drop the pointless assignment of Status
as well.

Tested-by: Gary Lin <glin@suse.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
v2: drop ExitData entirely instead of initializing it to NULL

 MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
index fc8775dfa419..6b8fb4d92461 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
@@ -439,8 +439,6 @@ EfiBootManagerDispatchDeferredImages (
   UINTN                              ImageSize;
   BOOLEAN                            BootOption;
   EFI_HANDLE                         ImageHandle;
-  UINTN                              ExitDataSize;
-  CHAR16                             *ExitData;
   UINTN                              ImageCount;
   UINTN                              LoadCount;
 
@@ -502,10 +500,7 @@ EfiBootManagerDispatchDeferredImages (
         // a 5 Minute period
         //
         gBS->SetWatchdogTimer (5 * 60, 0x0000, 0x00, NULL);
-        Status = gBS->StartImage (ImageHandle, &ExitDataSize, &ExitData);
-        if (ExitData != NULL) {
-          FreePool (ExitData);
-        }
+        gBS->StartImage (ImageHandle, NULL, NULL);
 
         //
         // Clear the Watchdog Timer after the image returns.
-- 
2.17.1

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

[Laszlo Ersek]

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"

With that:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

> 
> Given that ExitData is never actually used, let's just get rid of it
> entirely. While we're at it, drop the pointless assignment of Status
> as well.
> 
> Tested-by: Gary Lin <glin@suse.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: drop ExitData entirely instead of initializing it to NULL
> 
>  MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> index fc8775dfa419..6b8fb4d92461 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> @@ -439,8 +439,6 @@ EfiBootManagerDispatchDeferredImages (
>    UINTN                              ImageSize;
>    BOOLEAN                            BootOption;
>    EFI_HANDLE                         ImageHandle;
> -  UINTN                              ExitDataSize;
> -  CHAR16                             *ExitData;
>    UINTN                              ImageCount;
>    UINTN                              LoadCount;
>  
> @@ -502,10 +500,7 @@ EfiBootManagerDispatchDeferredImages (
>          // a 5 Minute period
>          //
>          gBS->SetWatchdogTimer (5 * 60, 0x0000, 0x00, NULL);
> -        Status = gBS->StartImage (ImageHandle, &ExitDataSize, &ExitData);
> -        if (ExitData != NULL) {
> -          FreePool (ExitData);
> -        }
> +        gBS->StartImage (ImageHandle, NULL, NULL);
>  
>          //
>          // Clear the Watchdog Timer after the image returns.
> 

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

[Ard Biesheuvel]

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

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)

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

[Wu, Hao A]

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

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

[Ard Biesheuvel]

On Wed, 17 Apr 2019 at 18:00, Wu, Hao A <hao.a.wu@intel.com> wrote:
>
> > -----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>
>

Thanks all

Pushed as cfb29d2bda57..d43056888790