From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: philmd@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 24 Sep 2019 03:34:50 -0700 Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5AE6537E79 for ; Tue, 24 Sep 2019 10:34:49 +0000 (UTC) Received: by mail-wm1-f69.google.com with SMTP id o8so744881wmc.2 for ; Tue, 24 Sep 2019 03:34:49 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:openpgp:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=jinVFFJ2hhrtt0b7tJk7K6oQOdSHWBKEEF7awYRJfIE=; b=d9xgwEF32ZPYY1BaVkeb4ofB6jItYrMis3ntnJFiYtpa3xojDGn1yvqFnx9ShlHt6P xr9VeFpA3ZTD6GwNSVq+ZBBXxOsy0H/h1PXHE4eIkLndRBJ6f7Y+FHuJNlkF3y7PoZIF E1+gsbQUYS5BWwKufr+EPKxGFCL59VyyYJQBrTDAK8H/ndqPxHZAUkvYulO4RqwH2w5U xsTEA0YJMFLqmWafX3jlwKtQ92WB8O+4HTmAYqHIoPYcjhS90Srxy/5iZaxtusNyyYQK zk0pGccYw/C6o3LAZc5BeupikJnf39CqVtqTy5W+nhQ3uSlyWZA3go3i3FREPyXLyvcN VLWw== X-Gm-Message-State: APjAAAUrjVxNAbGXiXWyqXDuyeqsZXeu7xbTZUgkjvgmvJuZ3dDC6UUC hPLvZM/Ch5H19m9/WLEbXqRniu5R+T0yh2pM+B5JAMwfkP/v8s/i26/EID66yPbstJV8AcnVyFo MSJzmy/ITp2Y/WQ== X-Received: by 2002:a1c:9ad1:: with SMTP id c200mr1858318wme.165.1569321288073; Tue, 24 Sep 2019 03:34:48 -0700 (PDT) X-Google-Smtp-Source: APXvYqwM+OVmV4vi1XwvitUhf9JcQrjlggR4KqxMU3Q29ee+PO9TuRhT3Jn0Ik5uvdzc1uy64fqAyw== X-Received: by 2002:a1c:9ad1:: with SMTP id c200mr1858301wme.165.1569321287878; Tue, 24 Sep 2019 03:34:47 -0700 (PDT) Received: from [192.168.1.115] (240.red-88-21-68.staticip.rima-tde.net. [88.21.68.240]) by smtp.gmail.com with ESMTPSA id y186sm3724794wmd.26.2019.09.24.03.34.46 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 24 Sep 2019 03:34:47 -0700 (PDT) Subject: Re: [edk2-devel] [patch v2 3/5] MdeModulePkg/UefiBootManager: Unload image on EFI_SECURITY_VIOLATION To: devel@edk2.groups.io, dandan.bi@intel.com Cc: Jian J Wang , Hao A Wu , Ray Ni , Zhichao Gao , Liming Gao , Laszlo Ersek References: <20190918030557.55256-1-dandan.bi@intel.com> <20190918030557.55256-4-dandan.bi@intel.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Openpgp: id=89C1E78F601EE86C867495CBA2A3FD6EDEADC0DE; url=http://pgp.mit.edu/pks/lookup?op=get&search=0xA2A3FD6EDEADC0DE Message-ID: Date: Tue, 24 Sep 2019 12:34:46 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: <20190918030557.55256-4-dandan.bi@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Dandan, On 9/18/19 5:05 AM, 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 > Cc: Hao A Wu > Cc: Ray Ni > Cc: Zhichao Gao > Cc: Liming Gao > Cc: Laszlo Ersek > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1992 > Signed-off-by: Dandan Bi > --- > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 9 +++++++++ > .../Library/UefiBootManagerLib/BmLoadOption.c | 11 ++++++++++- > MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c | 11 ++++++++++- > 3 files changed, 29 insertions(+), 2 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); OK > + } > // > // 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..af47b787d1 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); > What about: 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); > + } - if (!EFI_ERROR (Status)) { } else { > 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..833e38c6fe 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 == EFI_SECURITY_VIOLATION) { > + gBS->UnloadImage (ImageHandle); > + } > if (!EFI_ERROR (Status)) { Ditto. The logic is correct, but the code workflow is now odd. > LoadCount++; > // > // Before calling the image, enable the Watchdog Timer for > // a 5 Minute period >