[edk2-devel] [patch 2/3] MdeModulePkg: Unload image on EFI_SECURITY_VIOLATION
Dandan Bi
dandan.bi at intel.com
Thu Sep 5 06:23:48 UTC 2019
> -----Original Message-----
> From: Wu, Hao A
> Sent: Thursday, September 5, 2019 1:38 PM
> To: Bi, Dandan <dandan.bi at intel.com>; devel at edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang at intel.com>; Ni, Ray <ray.ni at intel.com>; Gao,
> Liming <liming.gao at intel.com>; Laszlo Ersek <lersek at 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 at 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 at intel.com>
> > Cc: Hao A Wu <hao.a.wu at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Liming Gao <liming.gao at intel.com>
> > Cc: Laszlo Ersek <lersek at redhat.com>
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1992
> > Signed-off-by: Dandan Bi <dandan.bi at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list