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

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

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


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