> -----Original Message-----
> From: Bi, Dandan
> Sent: Thursday, September 05, 2019 2:24 PM
> To: Wu, Hao A; [email protected]
> 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 <[email protected]>; [email protected]
> > Cc: Wang, Jian J <[email protected]>; Ni, Ray <[email protected]>; Gao,
> > Liming <[email protected]>; Laszlo Ersek <[email protected]>
> > 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: [email protected]
> > > 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 <[email protected]>
> > > Cc: Hao A Wu <[email protected]>
> > > Cc: Ray Ni <[email protected]>
> > > Cc: Liming Gao <[email protected]>
> > > Cc: Laszlo Ersek <[email protected]>
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1992
> > > Signed-off-by: Dandan Bi <[email protected]>
> > > ---
> > > 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/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
