Re: [patch 2/3] MdeModulePkg: Unload image on EFI_SECURITY_VIOLATION

["Dandan Bi" <dandan.bi@intel.com>]

Hi Ray,

Background:
This patch series is to review the callers of LoadImage() in edk2, if the LoadImage() with EFI_SECURITY_VIOLATION returned and 
caller doesn't have the attempt to defer the execution of an image, we should treat EFI_SECURITY_VIOLATION as other normal errors, but we should do unloadimage()  to free the resource since with EFI_SECURITY_VIOLATION returned, the image has been loaded.( https://bugzilla.tianocore.org/show_bug.cgi?id=1992)

Questions:
One caller of  LoadImage() in function ProcessOpRomImage in edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c,
Hao has mentioned as below that with EFI_SECURITY_VIOLATION returned, the image may be used later, we cannot unload the image right now.
So here we want to double confirm with you that 1) whether we cannot unload the image right now for this case since it may be used later 2) whether we should unload the image after finish the usage and where is the right place to do unload in this case.
Could you help take a look?

> For the PciBusDxe change, I think 'PciOptionRomImageDevicePath', which
> should be the loaded image device path, will still be used by AddDriver()
> when EFI_SECURITY_VIOLATION is returned by LoadImage():
> 
>   //
>   // Record the Option ROM Image device path when LoadImage fails.
>   // PciOverride.GetDriver() will try to look for the Image Handle using the
> device path later.
>   //
>   AddDriver (PciDevice, NULL, PciOptionRomImageDevicePath);
> 
> Later in GetDriver(), the device path will be used to locate the image handle:
> 
>   if (Override->DriverImageHandle == NULL) {
>     Override->DriverImageHandle = LocateImageHandle (Override-
> >DriverImagePath);
>   }
> 
> Ray, could you help to share your thoughts on this one? Thanks.
>

Thanks,
Dandan

> -----Original Message-----
> From: Wu, Hao A
> Sent: Thursday, September 5, 2019 4:35 PM
> To: Bi, Dandan <dandan.bi@intel.com>; devel@edk2.groups.io; Ni, Ray
> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Gao,
> Liming <liming.gao@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [patch 2/3] MdeModulePkg: Unload image on
> EFI_SECURITY_VIOLATION
> 
> > -----Original Message-----
> > From: Bi, Dandan
> > Sent: Thursday, September 05, 2019 2:24 PM
> > To: Wu, Hao A; devel@edk2.groups.io
> > Cc: Wang, Jian J; Ni, Ray; Gao, Liming; Laszlo Ersek; Bi, Dandan
> > Subject: RE: [patch 2/3] MdeModulePkg: Unload image on
> > EFI_SECURITY_VIOLATION
> >
> > > -----Original Message-----
> > > From: Wu, Hao A
> > > Sent: Thursday, September 5, 2019 1:38 PM
> > > To: Bi, Dandan <dandan.bi@intel.com>; devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray
> > > <ray.ni@intel.com>; Gao, Liming <liming.gao@intel.com>; Laszlo Ersek
> > > <lersek@redhat.com>
> > > Subject: RE: [patch 2/3] MdeModulePkg: Unload image on
> > > EFI_SECURITY_VIOLATION
> > >
> > > > -----Original Message-----
> > > > From: Bi, Dandan
> > > > Sent: Wednesday, September 04, 2019 4:26 PM
> > > > To: devel@edk2.groups.io
> > > > Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; Gao, Liming; Laszlo Ersek
> > > > Subject: [patch 2/3] MdeModulePkg: Unload image on
> > > > EFI_SECURITY_VIOLATION
> > > >
> > > > For the LoadImage() boot service, with EFI_SECURITY_VIOLATION
> > > > retval, the Image was loaded and an ImageHandle was created with a
> > > > valid EFI_LOADED_IMAGE_PROTOCOL, but the image can not be
> started
> > > > right
> > > now.
> > > > This follows UEFI Spec.
> > > >
> > > > But if the caller of LoadImage() doesn't have the option to defer
> > > > the execution of an image, we can not treat EFI_SECURITY_VIOLATION
> > > > like any other LoadImage() error, we should unload image for the
> > > > EFI_SECURITY_VIOLATION to avoid resource leak.
> > > >
> > > > This patch is to do error handling for EFI_SECURITY_VIOLATION
> > > > explicitly for the callers in MdeModulePkg which don't have the
> > > > policy to defer the execution of the image.
> > > >
> > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > > Cc: Ray Ni <ray.ni@intel.com>
> > > > Cc: Liming Gao <liming.gao@intel.com>
> > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1992
> > > > Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> > > > ---
> > > >  MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c  |  9
> > > > +++++++++
> > > >  MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c |  9
> > > > +++++++++
> > > >  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c      |  9
> > > +++++++++
> > > >  .../Library/UefiBootManagerLib/BmLoadOption.c         | 11
> ++++++++++-
> > > >  MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c      | 11
> > > > ++++++++++-
> > > >  .../PlatformDriOverrideDxe/PlatDriOverrideLib.c       | 11 ++++++++++-
> > >
> > >
> > > Hello,
> > >
> > > Could you help to provide the information on what tests have been
> > > performed for this patch? Thanks.
> >
> > Previously I only did the VS build since I think these are just the
> > enhancement for error handling.
> > For these callers,  they don't have the real use case to defer the
> > execution of the image.
> > EFI_SECURITY_VIOLATION  for them just like other errors, the only
> > difference is that with EFI_SECURITY_VIOLATION  retval, we need to
> > call UnloadImage () to free resource.
> >
> > Hao and other feature owners, do you have any suggestion for the tests?
> 
> Hello,
> 
> For the PciBusDxe change, I think 'PciOptionRomImageDevicePath', which
> should be the loaded image device path, will still be used by AddDriver()
> when EFI_SECURITY_VIOLATION is returned by LoadImage():
> 
>   //
>   // Record the Option ROM Image device path when LoadImage fails.
>   // PciOverride.GetDriver() will try to look for the Image Handle using the
> device path later.
>   //
>   AddDriver (PciDevice, NULL, PciOptionRomImageDevicePath);
> 
> Later in GetDriver(), the device path will be used to locate the image handle:
> 
>   if (Override->DriverImageHandle == NULL) {
>     Override->DriverImageHandle = LocateImageHandle (Override-
> >DriverImagePath);
>   }
> 
> Ray, could you help to share your thoughts on this one? Thanks.
> 
> 
> For the DxeCapsuleLibFmp & PlatformDriOverrideDxe changes, I am okay
> with only the build test. It looks to me that both of the cases will not attempt
> to consume the loaded image later if EFI_SECURITY_VIOLATION is returned.
> 
> For the UefiBootManagerLib changes, I will leave it to Ray and Zhichao.
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> >
> > >
> > > Also, since the patch is touching multiple features (PCI, Capsule,
> > > BM and driver override), I would suggest to break this patch into
> > > multiple ones so that it will be more clear to evaluate the impact for each
> change.
> > >
> > I will separate the patch into module level and send the new patch series.
> >
> >
> > Thanks,
> > Dandan
> >
> > > Best Regards,
> > > Hao Wu
> > >
> > >
> > > >  6 files changed, 57 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> > > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> > > > index c994ed5fe3..1a8d9811b0 100644
> > > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> > > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> > > > @@ -726,10 +726,19 @@ ProcessOpRomImage (
> > > >                      Buffer,
> > > >                      BufferSize,
> > > >                      &ImageHandle
> > > >                      );
> > > >      if (EFI_ERROR (Status)) {
> > > > +      //
> > > > +      // With EFI_SECURITY_VIOLATION retval, the Image was loaded
> > > > + and an
> > > > ImageHandle was created
> > > > +      // with a valid EFI_LOADED_IMAGE_PROTOCOL, but the image
> > > > + can not
> > > > be started right now.
> > > > +      // If the caller doesn't have the option to defer the
> > > > + execution of an
> > > > image, we should
> > > > +      // unload image for the EFI_SECURITY_VIOLATION to avoid
> > > > + resource
> > > leak.
> > > > +      //
> > > > +      if (Status == EFI_SECURITY_VIOLATION) {
> > > > +        gBS->UnloadImage (ImageHandle);
> > > > +      }
> > > >        //
> > > >        // Record the Option ROM Image device path when LoadImage fails.
> > > >        // PciOverride.GetDriver() will try to look for the Image
> > > > Handle using the device path later.
> > > >        //
> > > >        AddDriver (PciDevice, NULL, PciOptionRomImageDevicePath);
> > > > diff --git
> a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
> > > > b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
> > > > index 95aa9de087..74c00ecf9e 100644
> > > > --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
> > > > +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
> > > > @@ -1028,10 +1028,19 @@ StartFmpImage (
> > > >                    ImageSize,
> > > >                    &ImageHandle
> > > >                    );
> > > >    DEBUG((DEBUG_INFO, "FmpCapsule: LoadImage - %r\n", Status));
> > > >    if (EFI_ERROR(Status)) {
> > > > +    //
> > > > +    // With EFI_SECURITY_VIOLATION retval, the Image was loaded
> > > > + and an
> > > > ImageHandle was created
> > > > +    // with a valid EFI_LOADED_IMAGE_PROTOCOL, but the image can
> > > > + not be
> > > > started right now.
> > > > +    // If the caller doesn't have the option to defer the
> > > > + execution of an image,
> > > > we should
> > > > +    // unload image for the EFI_SECURITY_VIOLATION to avoid
> > > > + resource
> > > leak.
> > > > +    //
> > > > +    if (Status == EFI_SECURITY_VIOLATION) {
> > > > +      gBS->UnloadImage (ImageHandle);
> > > > +    }
> > > >      FreePool(DriverDevicePath);
> > > >      return Status;
> > > >    }
> > > >
> > > >    DEBUG((DEBUG_INFO, "FmpCapsule: StartImage ...\n")); diff --git
> > > > a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > > > b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > > > index 952033fc82..c8de7eec03 100644
> > > > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > > > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
> > > > @@ -1859,10 +1859,19 @@ EfiBootManagerBoot (
> > > >      if (FilePath != NULL) {
> > > >        FreePool (FilePath);
> > > >      }
> > > >
> > > >      if (EFI_ERROR (Status)) {
> > > > +      //
> > > > +      // With EFI_SECURITY_VIOLATION retval, the Image was loaded
> > > > + and an
> > > > ImageHandle was created
> > > > +      // with a valid EFI_LOADED_IMAGE_PROTOCOL, but the image
> > > > + can not
> > > > be started right now.
> > > > +      // If the caller doesn't have the option to defer the
> > > > + execution of an
> > > > image, we should
> > > > +      // unload image for the EFI_SECURITY_VIOLATION to avoid
> > > > + resource
> > > leak.
> > > > +      //
> > > > +      if (Status == EFI_SECURITY_VIOLATION) {
> > > > +        gBS->UnloadImage (ImageHandle);
> > > > +      }
> > > >        //
> > > >        // Report Status Code with the failure status to indicate
> > > > that the failure to load boot option
> > > >        //
> > > >        BmReportLoadFailure
> > > > (EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR, Status);
> > > >        BootOption->Status = Status; diff --git
> > a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> > > > b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> > > > index 07592f8ebd..233fb43c27 100644
> > > > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> > > > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> > > > @@ -1,9 +1,9 @@
> > > >  /** @file
> > > >    Load option library functions which relate with creating and
> > > > processing load options.
> > > >
> > > > -Copyright (c) 2011 - 2018, Intel Corporation. All rights
> > > > reserved.<BR>
> > > > +Copyright (c) 2011 - 2019, Intel Corporation. All rights
> > > > +reserved.<BR>
> > > >  (C) Copyright 2015-2018 Hewlett Packard Enterprise Development
> > > > LP<BR>
> > > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > >
> > > >  **/
> > > >
> > > > @@ -1409,10 +1409,19 @@ EfiBootManagerProcessLoadOption (
> > > >                      FileSize,
> > > >                      &ImageHandle
> > > >                      );
> > > >      FreePool (FileBuffer);
> > > >
> > > > +    //
> > > > +    // With EFI_SECURITY_VIOLATION retval, the Image was loaded
> > > > + and an
> > > > ImageHandle was created
> > > > +    // with a valid EFI_LOADED_IMAGE_PROTOCOL, but the image can
> > > > + not be
> > > > started right now.
> > > > +    // If the caller doesn't have the option to defer the
> > > > + execution of an image,
> > > > we should
> > > > +    // unload image for the EFI_SECURITY_VIOLATION to avoid
> > > > + resource
> > > leak.
> > > > +    //
> > > > +    if (Status == EFI_SECURITY_VIOLATION) {
> > > > +      gBS->UnloadImage (ImageHandle);
> > > > +    }
> > > >      if (!EFI_ERROR (Status)) {
> > > >        Status = gBS->HandleProtocol (ImageHandle,
> > > > &gEfiLoadedImageProtocolGuid, (VOID **)&ImageInfo);
> > > >        ASSERT_EFI_ERROR (Status);
> > > >
> > > >        ImageInfo->LoadOptionsSize = LoadOption->OptionalDataSize;
> > > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> > > > b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> > > > index 6b8fb4d924..cdfc57741b 100644
> > > > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> > > > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c
> > > > @@ -1,9 +1,9 @@
> > > >  /** @file
> > > >    Misc library functions.
> > > >
> > > > -Copyright (c) 2011 - 2018, Intel Corporation. All rights
> > > > reserved.<BR>
> > > > +Copyright (c) 2011 - 2019, Intel Corporation. All rights
> > > > +reserved.<BR>
> > > >  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> > > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > > >
> > > >  **/
> > > >
> > > > @@ -491,10 +491,19 @@ EfiBootManagerDispatchDeferredImages (
> > > >          ImageDevicePath,
> > > >          NULL,
> > > >          0,
> > > >          &ImageHandle
> > > >        );
> > > > +      //
> > > > +      // With EFI_SECURITY_VIOLATION retval, the Image was loaded
> > > > + and an
> > > > ImageHandle was created
> > > > +      // with a valid EFI_LOADED_IMAGE_PROTOCOL, but the image
> > > > + can not
> > > > be started right now.
> > > > +      // If the caller doesn't have the option to defer the
> > > > + execution of an
> > > > image, we should
> > > > +      // unload image for the EFI_SECURITY_VIOLATION to avoid
> > > > + resource
> > > leak.
> > > > +      //
> > > > +      if (Status == EFI_SECURITY_VIOLATION) {
> > > > +        gBS->UnloadImage (ImageHandle);
> > > > +      }
> > > >        if (!EFI_ERROR (Status)) {
> > > >          LoadCount++;
> > > >          //
> > > >          // Before calling the image, enable the Watchdog Timer for
> > > >          // a 5 Minute period
> > > > diff --git
> > > >
> > a/MdeModulePkg/Universal/PlatformDriOverrideDxe/PlatDriOverrideLib.c
> > > >
> > b/MdeModulePkg/Universal/PlatformDriOverrideDxe/PlatDriOverrideLib.c
> > > > index 2d3736b468..e4b6b26330 100644
> > > > ---
> > > >
> > a/MdeModulePkg/Universal/PlatformDriOverrideDxe/PlatDriOverrideLib.c
> > > > +++
> > > >
> > b/MdeModulePkg/Universal/PlatformDriOverrideDxe/PlatDriOverrideLib.c
> > > > @@ -1,9 +1,9 @@
> > > >  /** @file
> > > >    Implementation of the shared functions to do the platform
> > > > driver vverride mapping.
> > > >
> > > > -  Copyright (c) 2007 - 2018, Intel Corporation. All rights
> > > > reserved.<BR>
> > > > +  Copyright (c) 2007 - 2019, Intel Corporation. All rights
> > > > + reserved.<BR>
> > > >    SPDX-License-Identifier: BSD-2-Clause-Patent
> > > >
> > > >  **/
> > > >
> > > >  #include "InternalPlatDriOverrideDxe.h"
> > > > @@ -1484,10 +1484,19 @@ GetDriverFromMapping (
> > > >                                     );
> > > >                  ASSERT (DriverBinding != NULL);
> > > >                  DriverImageInfo->ImageHandle = ImageHandle;
> > > >                }
> > > >              } else {
> > > > +              //
> > > > +              // With EFI_SECURITY_VIOLATION retval, the Image
> > > > + was loaded and
> > > > an ImageHandle was created
> > > > +              // with a valid EFI_LOADED_IMAGE_PROTOCOL, but the
> > > > + image can
> > > > not be started right now.
> > > > +              // If the caller doesn't have the option to defer
> > > > + the execution of an
> > > > image, we should
> > > > +              // unload image for the EFI_SECURITY_VIOLATION to
> > > > + avoid resource
> > > > leak.
> > > > +              //
> > > > +              if (Status == EFI_SECURITY_VIOLATION) {
> > > > +                gBS->UnloadImage (ImageHandle);
> > > > +              }
> > > >                DriverImageInfo->UnLoadable = TRUE;
> > > >                DriverImageInfo->ImageHandle = NULL;
> > > >              }
> > > >            }
> > > >          }
> > > > --
> > > > 2.18.0.windows.1