From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: dandan.bi@intel.com) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by groups.io with SMTP; Mon, 09 Sep 2019 20:37:57 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Sep 2019 20:37:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,487,1559545200"; d="scan'208";a="268277360" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga001.jf.intel.com with ESMTP; 09 Sep 2019 20:37:56 -0700 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 9 Sep 2019 20:37:55 -0700 Received: from shsmsx106.ccr.corp.intel.com (10.239.4.159) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 9 Sep 2019 20:37:54 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.32]) by SHSMSX106.ccr.corp.intel.com ([169.254.10.86]) with mapi id 14.03.0439.000; Tue, 10 Sep 2019 11:37:52 +0800 From: "Dandan Bi" To: "Ni, Ray" , "Wu, Hao A" , "devel@edk2.groups.io" , "Gao, Zhichao" CC: "Wang, Jian J" , "Ni, Ray" , "Gao, Liming" , Laszlo Ersek , "Bi, Dandan" Subject: Re: [patch 2/3] MdeModulePkg: Unload image on EFI_SECURITY_VIOLATION Thread-Topic: [patch 2/3] MdeModulePkg: Unload image on EFI_SECURITY_VIOLATION Thread-Index: AQHVYvptyH6Ffthe4EeiYwW7e4Mznqccj+eQgAAJeDCAACQvkIAHjEpw Date: Tue, 10 Sep 2019 03:37:52 +0000 Message-ID: <3C0D5C461C9E904E8F62152F6274C0BB40C5894D@SHSMSX104.ccr.corp.intel.com> References: <20190904082555.35424-1-dandan.bi@intel.com> <20190904082555.35424-3-dandan.bi@intel.com> <3C0D5C461C9E904E8F62152F6274C0BB40C57431@SHSMSX104.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: dandan.bi@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Ray, Background: This patch series is to review the callers of LoadImage() in edk2, if the L= oadImage() with EFI_SECURITY_VIOLATION returned and=20 caller doesn't have the attempt to defer the execution of an image, we shou= ld treat EFI_SECURITY_VIOLATION as other normal errors, but we should do un= loadimage() to free the resource since with EFI_SECURITY_VIOLATION returne= d, the image has been loaded.( https://bugzilla.tianocore.org/show_bug.cgi?= id=3D1992) Questions: One caller of LoadImage() in function ProcessOpRomImage in edk2/MdeModuleP= kg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c, Hao has mentioned as below that with EFI_SECURITY_VIOLATION returned, the i= mage 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 plac= e 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(): >=20 > // > // 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); >=20 > Later in GetDriver(), the device path will be used to locate the image ha= ndle: >=20 > if (Override->DriverImageHandle =3D=3D NULL) { > Override->DriverImageHandle =3D LocateImageHandle (Override- > >DriverImagePath); > } >=20 > 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 ; devel@edk2.groups.io; Ni, Ray > ; Gao, Zhichao > Cc: Wang, Jian J ; Ni, Ray ; Gao= , > Liming ; Laszlo Ersek > Subject: RE: [patch 2/3] MdeModulePkg: Unload image on > EFI_SECURITY_VIOLATION >=20 > > -----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 ; devel@edk2.groups.io > > > Cc: Wang, Jian J ; Ni, Ray > > > ; Gao, Liming ; Laszlo Ersek > > > > > > 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 > > > > Cc: Hao A Wu > > > > Cc: Ray Ni > > > > Cc: Liming Gao > > > > Cc: Laszlo Ersek > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1992 > > > > Signed-off-by: Dandan Bi > > > > --- > > > > 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? >=20 > Hello, >=20 > 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(): >=20 > // > // 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); >=20 > Later in GetDriver(), the device path will be used to locate the image ha= ndle: >=20 > if (Override->DriverImageHandle =3D=3D NULL) { > Override->DriverImageHandle =3D LocateImageHandle (Override- > >DriverImagePath); > } >=20 > Ray, could you help to share your thoughts on this one? Thanks. >=20 >=20 > 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. >=20 > For the UefiBootManagerLib changes, I will leave it to Ray and Zhichao. >=20 > Best Regards, > Hao Wu >=20 >=20 > > > > > > > > > > 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 fo= r each > change. > > > > > I will separate the patch into module level and send the new patch seri= es. > > > > > > 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 =3D=3D EFI_SECURITY_VIOLATION) { > > > > + gBS->UnloadImage (ImageHandle); > > > > + } > > > > // > > > > // Record the Option ROM Image device path when LoadImage fa= ils. > > > > // 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 =3D=3D 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 !=3D 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 =3D=3D 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 =3D 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.
> > > > +Copyright (c) 2011 - 2019, Intel Corporation. All rights > > > > +reserved.
> > > > (C) Copyright 2015-2018 Hewlett Packard Enterprise Development > > > > LP
> > > > 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 =3D=3D EFI_SECURITY_VIOLATION) { > > > > + gBS->UnloadImage (ImageHandle); > > > > + } > > > > if (!EFI_ERROR (Status)) { > > > > Status =3D gBS->HandleProtocol (ImageHandle, > > > > &gEfiLoadedImageProtocolGuid, (VOID **)&ImageInfo); > > > > ASSERT_EFI_ERROR (Status); > > > > > > > > ImageInfo->LoadOptionsSize =3D 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.
> > > > +Copyright (c) 2011 - 2019, Intel Corporation. All rights > > > > +reserved.
> > > > (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> > > > 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 =3D=3D 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.
> > > > + Copyright (c) 2007 - 2019, Intel Corporation. All rights > > > > + reserved.
> > > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > > > > > **/ > > > > > > > > #include "InternalPlatDriOverrideDxe.h" > > > > @@ -1484,10 +1484,19 @@ GetDriverFromMapping ( > > > > ); > > > > ASSERT (DriverBinding !=3D NULL); > > > > DriverImageInfo->ImageHandle =3D 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 =3D=3D EFI_SECURITY_VIOLATION) { > > > > + gBS->UnloadImage (ImageHandle); > > > > + } > > > > DriverImageInfo->UnLoadable =3D TRUE; > > > > DriverImageInfo->ImageHandle =3D NULL; > > > > } > > > > } > > > > } > > > > -- > > > > 2.18.0.windows.1