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

["Wu, Hao A" <hao.a.wu@intel.com>]

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