From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.120, mailfrom: hao.a.wu@intel.com) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by groups.io with SMTP; Wed, 04 Sep 2019 22:37:56 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Sep 2019 22:37:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,469,1559545200"; d="scan'208";a="187862705" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga006.jf.intel.com with ESMTP; 04 Sep 2019 22:37:55 -0700 Received: from fmsmsx118.amr.corp.intel.com (10.18.116.18) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 4 Sep 2019 22:37:55 -0700 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by fmsmsx118.amr.corp.intel.com (10.18.116.18) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 4 Sep 2019 22:37:55 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.32]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.23]) with mapi id 14.03.0439.000; Thu, 5 Sep 2019 13:37:53 +0800 From: "Wu, Hao A" 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 Thread-Topic: [patch 2/3] MdeModulePkg: Unload image on EFI_SECURITY_VIOLATION Thread-Index: AQHVYvptyH6Ffthe4EeiYwW7e4Mznqccj+eQ Date: Thu, 5 Sep 2019 05:37:52 +0000 Message-ID: References: <20190904082555.35424-1-dandan.bi@intel.com> <20190904082555.35424-3-dandan.bi@intel.com> In-Reply-To: <20190904082555.35424-3-dandan.bi@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----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 >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. 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(-) >=20 > 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 l= eak. > + // > + if (Status =3D=3D 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 u= sing 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 a= n image, > we should > + // unload image for the EFI_SECURITY_VIOLATION to avoid resource lea= k. > + // > + if (Status =3D=3D EFI_SECURITY_VIOLATION) { > + gBS->UnloadImage (ImageHandle); > + } > FreePool(DriverDevicePath); > return Status; > } >=20 > 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); > } >=20 > 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 l= eak. > + // > + 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 processin= g load > options. >=20 > -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 >=20 > **/ >=20 > @@ -1409,10 +1409,19 @@ EfiBootManagerProcessLoadOption ( > FileSize, > &ImageHandle > ); > FreePool (FileBuffer); >=20 > + // > + // 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 a= n image, > we should > + // unload image for the EFI_SECURITY_VIOLATION to avoid resource lea= k. > + // > + 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); >=20 > 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. >=20 > -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 >=20 > **/ >=20 > @@ -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 l= eak. > + // > + 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 vverr= ide > mapping. >=20 > - 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 >=20 > **/ >=20 > #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 loade= d and > an ImageHandle was created > + // with a valid EFI_LOADED_IMAGE_PROTOCOL, but the image c= an > not be started right now. > + // If the caller doesn't have the option to defer the exec= ution of an > image, we should > + // unload image for the EFI_SECURITY_VIOLATION to avoid re= source > leak. > + // > + if (Status =3D=3D EFI_SECURITY_VIOLATION) { > + gBS->UnloadImage (ImageHandle); > + } > DriverImageInfo->UnLoadable =3D TRUE; > DriverImageInfo->ImageHandle =3D NULL; > } > } > } > -- > 2.18.0.windows.1