[patch 0/3] Unload image on EFI_SECURITY_VIOLATION

[patch 0/3] Unload image on EFI_SECURITY_VIOLATION

[Dandan Bi]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1992

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 edk2 which don't have the policy to defer the
execution of the image.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
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: Zhichao Gao <zhichao.gao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Dandan Bi (3):
  EmbeddedPkg: Unload image on EFI_SECURITY_VIOLATION
  MdeModulePkg: Unload image on EFI_SECURITY_VIOLATION
  ShellPkg: Unload image on EFI_SECURITY_VIOLATION

 .../AndroidFastboot/Arm/BootAndroidBootImg.c         |  9 +++++++++
 .../Library/AndroidBootImgLib/AndroidBootImgLib.c    | 12 ++++++++++++
 MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c |  9 +++++++++
 .../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 ++++++++++-
 ShellPkg/Application/Shell/ShellManParser.c          |  9 +++++++++
 .../Library/UefiShellDebug1CommandsLib/LoadPciRom.c  | 11 ++++++++++-
 ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c   | 11 ++++++++++-
 11 files changed, 107 insertions(+), 5 deletions(-)

-- 
2.18.0.windows.1

[patch 1/3] EmbeddedPkg: Unload image on EFI_SECURITY_VIOLATION

[Dandan Bi]

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 EmbeddedPkg which don't have the policy to defer the
execution of the image.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
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>
---
 .../AndroidFastboot/Arm/BootAndroidBootImg.c         |  9 +++++++++
 .../Library/AndroidBootImgLib/AndroidBootImgLib.c    | 12 ++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
index 591afbe7cc..9fa28e3390 100644
--- a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
+++ b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
@@ -71,10 +71,19 @@ StartEfiApplication (
 
   // Load the image from the device path with Boot Services function
   Status = gBS->LoadImage (TRUE, ParentImageHandle, DevicePath, NULL, 0,
                   &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);
+    }
     return Status;
   }
 
   // Passed LoadOptions to the EFI Application
   if (LoadOptionsSize != 0) {
diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
index d9e7aa7d2b..2e9e74db1d 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -439,10 +439,22 @@ AndroidBootImgBoot (
                                            + KernelSize;
 
   Status = gBS->LoadImage (TRUE, gImageHandle,
                            (EFI_DEVICE_PATH *)&KernelDevicePath,
                            (VOID*)(UINTN)Kernel, KernelSize, &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);
+    }
+    return Status;
+  }
 
   // Set kernel arguments
   Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid,
                                 (VOID **) &ImageInfo);
   ImageInfo->LoadOptions = NewKernelArg;
-- 
2.18.0.windows.1

Re: [edk2-devel] [patch 1/3] EmbeddedPkg: Unload image on EFI_SECURITY_VIOLATION

[Ard Biesheuvel]

On Wed, 4 Sep 2019 at 01:26, Dandan Bi <dandan.bi@intel.com> wrote:
>
> 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 EmbeddedPkg which don't have the policy to defer the
> execution of the image.
>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 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>

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  .../AndroidFastboot/Arm/BootAndroidBootImg.c         |  9 +++++++++
>  .../Library/AndroidBootImgLib/AndroidBootImgLib.c    | 12 ++++++++++++
>  2 files changed, 21 insertions(+)
>
> diff --git a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
> index 591afbe7cc..9fa28e3390 100644
> --- a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
> +++ b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
> @@ -71,10 +71,19 @@ StartEfiApplication (
>
>    // Load the image from the device path with Boot Services function
>    Status = gBS->LoadImage (TRUE, ParentImageHandle, DevicePath, NULL, 0,
>                    &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);
> +    }
>      return Status;
>    }
>
>    // Passed LoadOptions to the EFI Application
>    if (LoadOptionsSize != 0) {
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> index d9e7aa7d2b..2e9e74db1d 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> @@ -439,10 +439,22 @@ AndroidBootImgBoot (
>                                             + KernelSize;
>
>    Status = gBS->LoadImage (TRUE, gImageHandle,
>                             (EFI_DEVICE_PATH *)&KernelDevicePath,
>                             (VOID*)(UINTN)Kernel, KernelSize, &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);
> +    }
> +    return Status;
> +  }
>
>    // Set kernel arguments
>    Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid,
>                                  (VOID **) &ImageInfo);
>    ImageInfo->LoadOptions = NewKernelArg;
> --
> 2.18.0.windows.1
>
>
> 
>

Re: [edk2-devel] [patch 1/3] EmbeddedPkg: Unload image on EFI_SECURITY_VIOLATION

[Laszlo Ersek]

On 09/04/19 10:25, Dandan Bi wrote:
> 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 EmbeddedPkg which don't have the policy to defer the
> execution of the image.
> 
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 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>
> ---
>  .../AndroidFastboot/Arm/BootAndroidBootImg.c         |  9 +++++++++
>  .../Library/AndroidBootImgLib/AndroidBootImgLib.c    | 12 ++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
> index 591afbe7cc..9fa28e3390 100644
> --- a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
> +++ b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
> @@ -71,10 +71,19 @@ StartEfiApplication (
>  
>    // Load the image from the device path with Boot Services function
>    Status = gBS->LoadImage (TRUE, ParentImageHandle, DevicePath, NULL, 0,
>                    &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);
> +    }
>      return Status;
>    }
>  
>    // Passed LoadOptions to the EFI Application
>    if (LoadOptionsSize != 0) {

The return status exists here originally, so this change is strictly an
improvement.

But:

> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> index d9e7aa7d2b..2e9e74db1d 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> @@ -439,10 +439,22 @@ AndroidBootImgBoot (
>                                             + KernelSize;
>  
>    Status = gBS->LoadImage (TRUE, gImageHandle,
>                             (EFI_DEVICE_PATH *)&KernelDevicePath,
>                             (VOID*)(UINTN)Kernel, KernelSize, &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);
> +    }
> +    return Status;
> +  }
>  
>    // Set kernel arguments
>    Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid,
>                                  (VOID **) &ImageInfo);
>    ImageInfo->LoadOptions = NewKernelArg;
> 

The return status is introduced afresh here. And, at this point in the
function, we have allocated at least "NewKernelArg". (And probably more
resources.)

If we take an early return here, then we're going to leak NewKernelArg.

Hm... Hmm. Well, even without this patch, if the loaded and started
image returns, then the AndroidBootImgBoot() function leaks
"NewKernelArg" anyway.

Sigh. I guess the error handling in this function should be cleaned up
first, to solve the present issue cleanly. On the other hand, I'm not
sure how much this function is used at all. And I guess the patch
doesn't make things worse than they are.

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

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

[Dandan Bi]

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

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

[Wu, Hao A]

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

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.

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

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

[Dandan Bi]

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

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

[Wu, Hao A]

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

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

[Dandan Bi]

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

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

[Laszlo Ersek]

just a meta comment:

On 09/05/19 08:23, Dandan Bi wrote:
>> -----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

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

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

Yes, do that please, especially because now we have fine-grained
maintainership assignments in "Maintainers.txt" (not for all packages,
but certainly for MdeModulePkg), and a python script to assign reviewers.

Let's say, you have three patches in the series, on your topic branch.
You can do:

$ python BaseTools/Scripts/GetMaintainer.py HEAD~2
$ python BaseTools/Scripts/GetMaintainer.py HEAD~1
$ python BaseTools/Scripts/GetMaintainer.py HEAD

Each invocation will give you the list of people that you should CC on
that particular patch.

You can also invoke the script with the "-l" option, to get reviewers
for a specific file:

$ python BaseTools/Scripts/GetMaintainer.py \
    -l MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c

$ python BaseTools/Scripts/GetMaintainer.py \
    -l MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c

Other than this, I'll skip the review of patches #2 and #3 in this
series; I've got too many things to do, and I'm not a designated
reviewer / maintainer on either module being modified. I hope that's
acceptable.

Thanks,
Laszlo

[patch 3/3] ShellPkg: Unload image on EFI_SECURITY_VIOLATION

[Dandan Bi]

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 ShellPkg which don't have the policy to defer the
execution of the image.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.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>
---
 ShellPkg/Application/Shell/ShellManParser.c           |  9 +++++++++
 .../Library/UefiShellDebug1CommandsLib/LoadPciRom.c   | 11 ++++++++++-
 ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c    | 11 ++++++++++-
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/ShellPkg/Application/Shell/ShellManParser.c b/ShellPkg/Application/Shell/ShellManParser.c
index 6909f29441..e5f97bbb11 100644
--- a/ShellPkg/Application/Shell/ShellManParser.c
+++ b/ShellPkg/Application/Shell/ShellManParser.c
@@ -643,10 +643,19 @@ ProcessManFile(
       goto Done;
     }
     DevPath = ShellInfoObject.NewEfiShellProtocol->GetDevicePathFromFilePath(CmdFilePathName);
     Status      = gBS->LoadImage(FALSE, gImageHandle, DevPath, NULL, 0, &CmdFileImgHandle);
     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 the resource leak.
+      //
+      if (Status == EFI_SECURITY_VIOLATION) {
+        gBS->UnloadImage (CmdFileImgHandle);
+      }
       *HelpText = NULL;
       goto Done;
     }
     Status = gBS->OpenProtocol(
                     CmdFileImgHandle,
diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c
index 1b169d0d3c..f91e3eb6e7 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c
@@ -1,10 +1,10 @@
 /** @file
   Main file for LoadPciRom shell Debug1 function.
 
   (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR>
-  Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2005 - 2019, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
 #include "UefiShellDebug1CommandsLib.h"
@@ -332,10 +332,19 @@ LoadEfiDriversFromRomImage (
                         ImageBuffer,
                         ImageLength,
                         &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);
+            }
             ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_LOADPCIROM_LOAD_FAIL), gShellDebug1HiiHandle, L"loadpcirom", FileName, ImageIndex);
 //            PrintToken (STRING_TOKEN (STR_LOADPCIROM_LOAD_IMAGE_ERROR), HiiHandle, ImageIndex, Status);
           } else {
             Status = gBS->StartImage (ImageHandle, NULL, NULL);
             if (EFI_ERROR (Status)) {
diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
index 6a94b48c86..a13e1bda2d 100644
--- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
+++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
@@ -1,10 +1,10 @@
 /** @file
   Main file for attrib shell level 2 function.
 
   (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR>
-  Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
 #include "UefiShellLevel2CommandsLib.h"
@@ -110,10 +110,19 @@ LoadDriver(
     NULL,
     0,
     &LoadedDriverHandle);
 
   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 (LoadedDriverHandle);
+    }
     ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_LOAD_NOT_IMAGE), gShellLevel2HiiHandle, FileName, Status);
   } else {
     //
     // Make sure it is a driver image
     //
-- 
2.18.0.windows.1

Re: [patch 3/3] ShellPkg: Unload image on EFI_SECURITY_VIOLATION

[Gao, Zhichao]

Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

> -----Original Message-----
> From: Bi, Dandan
> Sent: Wednesday, September 4, 2019 4:26 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> Laszlo Ersek <lersek@redhat.com>
> Subject: [patch 3/3] ShellPkg: 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 ShellPkg which don't have the policy to defer the execution of
> the image.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.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>
> ---
>  ShellPkg/Application/Shell/ShellManParser.c           |  9 +++++++++
>  .../Library/UefiShellDebug1CommandsLib/LoadPciRom.c   | 11 ++++++++++-
>  ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c    | 11 ++++++++++-
>  3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/ShellPkg/Application/Shell/ShellManParser.c
> b/ShellPkg/Application/Shell/ShellManParser.c
> index 6909f29441..e5f97bbb11 100644
> --- a/ShellPkg/Application/Shell/ShellManParser.c
> +++ b/ShellPkg/Application/Shell/ShellManParser.c
> @@ -643,10 +643,19 @@ ProcessManFile(
>        goto Done;
>      }
>      DevPath = ShellInfoObject.NewEfiShellProtocol-
> >GetDevicePathFromFilePath(CmdFilePathName);
>      Status      = gBS->LoadImage(FALSE, gImageHandle, DevPath, NULL, 0,
> &CmdFileImgHandle);
>      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 the resource
> leak.
> +      //
> +      if (Status == EFI_SECURITY_VIOLATION) {
> +        gBS->UnloadImage (CmdFileImgHandle);
> +      }
>        *HelpText = NULL;
>        goto Done;
>      }
>      Status = gBS->OpenProtocol(
>                      CmdFileImgHandle,
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c
> index 1b169d0d3c..f91e3eb6e7 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c
> @@ -1,10 +1,10 @@
>  /** @file
>    Main file for LoadPciRom shell Debug1 function.
> 
>    (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR>
> -  Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2005 - 2019, Intel Corporation. All rights
> + reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> 
>  #include "UefiShellDebug1CommandsLib.h"
> @@ -332,10 +332,19 @@ LoadEfiDriversFromRomImage (
>                          ImageBuffer,
>                          ImageLength,
>                          &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);
> +            }
>              ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN
> (STR_LOADPCIROM_LOAD_FAIL), gShellDebug1HiiHandle, L"loadpcirom",
> FileName, ImageIndex);
>  //            PrintToken (STRING_TOKEN
> (STR_LOADPCIROM_LOAD_IMAGE_ERROR), HiiHandle, ImageIndex, Status);
>            } else {
>              Status = gBS->StartImage (ImageHandle, NULL, NULL);
>              if (EFI_ERROR (Status)) {
> diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
> b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
> index 6a94b48c86..a13e1bda2d 100644
> --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
> +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c
> @@ -1,10 +1,10 @@
>  /** @file
>    Main file for attrib shell level 2 function.
> 
>    (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR>
> -  Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2009 - 2019, Intel Corporation. All rights
> + reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> 
>  #include "UefiShellLevel2CommandsLib.h"
> @@ -110,10 +110,19 @@ LoadDriver(
>      NULL,
>      0,
>      &LoadedDriverHandle);
> 
>    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 (LoadedDriverHandle);
> +    }
>      ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_LOAD_NOT_IMAGE),
> gShellLevel2HiiHandle, FileName, Status);
>    } else {
>      //
>      // Make sure it is a driver image
>      //
> --
> 2.18.0.windows.1