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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#46767): https://edk2.groups.io/g/devel/message/46767
Mute This Topic: https://groups.io/mt/33136046/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#46868): https://edk2.groups.io/g/devel/message/46868
Mute This Topic: https://groups.io/mt/33136046/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#46875): https://edk2.groups.io/g/devel/message/46875
Mute This Topic: https://groups.io/mt/33136046/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#46887): https://edk2.groups.io/g/devel/message/46887
Mute This Topic: https://groups.io/mt/33136046/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47062): https://edk2.groups.io/g/devel/message/47062
Mute This Topic: https://groups.io/mt/33136046/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#46937): https://edk2.groups.io/g/devel/message/46937
Mute This Topic: https://groups.io/mt/33136046/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.