Nice catch. Moving the check into the if conditional section would avoid the additional check when the status is EFI_SUCCESS.
Reviewed-by: Zhichao Gao <zhichao....@intel.com> Thanks, Zhichao > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Philippe Mathieu-Daudé > Sent: Tuesday, September 24, 2019 9:22 PM > To: devel@edk2.groups.io; Bi, Dandan <dandan...@intel.com> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; > Ni, Ray <ray...@intel.com>; Gao, Zhichao <zhichao....@intel.com>; Gao, > Liming <liming....@intel.com>; Laszlo Ersek <ler...@redhat.com> > Subject: Re: [edk2-devel] [patch v3 3/5] MdeModulePkg/UefiBootManager: > Unload image on EFI_SECURITY_VIOLATION > > On 9/24/19 3:16 PM, Dandan Bi wrote: > > 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 UefiBootManagerLib 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: Zhichao Gao <zhichao....@intel.com> > > Cc: Liming Gao <liming....@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Philippe Mathieu-Daude <phi...@redhat.com> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1992 > > Signed-off-by: Dandan Bi <dandan...@intel.com> > > --- > > V3: Enahnce the error handling logic in BmLoadOption.c and BmMisc.c. > > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 9 +++++++++ > > .../Library/UefiBootManagerLib/BmLoadOption.c | 14 ++++++++++++-- > > MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c | 14 > ++++++++++++-- > > 3 files changed, 33 insertions(+), 4 deletions(-) > > > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > > b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c > > index 952033fc82..760d7647b8 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..89372b3b97 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,11 +1409,21 @@ EfiBootManagerProcessLoadOption ( > > FileSize, > > &ImageHandle > > ); > > FreePool (FileBuffer); > > > > - if (!EFI_ERROR (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); > > + } > > + } else { > > Thanks for changing this Dandan! > > > Status = gBS->HandleProtocol (ImageHandle, > &gEfiLoadedImageProtocolGuid, (VOID **)&ImageInfo); > > ASSERT_EFI_ERROR (Status); > > > > ImageInfo->LoadOptionsSize = LoadOption->OptionalDataSize; > > ImageInfo->LoadOptions = LoadOption->OptionalData; diff --git > > a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c > > b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c > > index 6b8fb4d924..89595747af 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,11 +491,21 @@ EfiBootManagerDispatchDeferredImages ( > > ImageDevicePath, > > NULL, > > 0, > > &ImageHandle > > ); > > - if (!EFI_ERROR (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); > > + } > > + } else { > > Reviewed-by: Philippe Mathieu-Daude <phi...@redhat.com> > > > LoadCount++; > > // > > // Before calling the image, enable the Watchdog Timer for > > // a 5 Minute period > > // > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47951): https://edk2.groups.io/g/devel/message/47951 Mute This Topic: https://groups.io/mt/34275944/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-