> -----Original Message----- > From: Wu, Hao A > Sent: Thursday, September 5, 2019 1:38 PM > To: Bi, Dandan <dandan...@intel.com>; devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.w...@intel.com>; Ni, Ray <ray...@intel.com>; Gao, > Liming <liming....@intel.com>; Laszlo Ersek <ler...@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.w...@intel.com> > > Cc: Hao A Wu <hao.a...@intel.com> > > Cc: Ray Ni <ray...@intel.com> > > Cc: Liming Gao <liming....@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1992 > > Signed-off-by: Dandan Bi <dandan...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-