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 06:21:51 -0700 Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) (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 62D7DC058CA4 for ; Tue, 24 Sep 2019 13:21:50 +0000 (UTC) Received: by mail-wm1-f71.google.com with SMTP id 190so7183wme.4 for ; Tue, 24 Sep 2019 06:21:50 -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=XQz2UZFz2BK4hU/HaRY0y/zlU//t6XAFzJg6X/llhgM=; b=C7Nsp2lpyS5HXEjmjw1tLMVZrTbnBN00bOnKoXsYknFXVbfv49YTbMojIb9eEbHm9G Mejx35Ng4ZjDgKnOp1CgQvw1cv+BtzG/OPhfitPGqWNQrb5wh2pNTN3P2R9plb1Wbnjp /KgHGepYZQmbxnpR/SnYSWTxde6Kq8i6MQXjoaT5vG/5hOc7phKC9fdkP/ZQr7ikabq+ PD6Mlr+nQ9wRLS2F9KID0WcUm+ZWSlTG825cwlLIzKn7Cj+tNqUgTNfX22cfw+XK985z gpvUYIqgVQnflslhHTml2tGIalDgbP1Em0J0DXbjdGYJqzhvnq2ZtjllTNdqK/Vm7OQg TZcw== X-Gm-Message-State: APjAAAWNroNKRefLby9RRRyoc9zPLpKIr9L+uQHyn+Cn1RNPU2hb4y9N HsjeP8O7meU5fGsP87LRf2YOWWiottOgqRPRLp3/1SGMANxweNhHfzzP34eeN7ISIVx6mifXiWk 37Ni2u5cOtZ7/Dg== X-Received: by 2002:a1c:7fcc:: with SMTP id a195mr2788946wmd.27.1569331309068; Tue, 24 Sep 2019 06:21:49 -0700 (PDT) X-Google-Smtp-Source: APXvYqwfPtgxsc9qQfzXeYCXLuwFREX7H4LVLyaU/KOCW1sG28GtYKtJwvDpUA39bLihGZKpZvVGEw== X-Received: by 2002:a1c:7fcc:: with SMTP id a195mr2788854wmd.27.1569331307971; Tue, 24 Sep 2019 06:21: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 p85sm3797650wme.23.2019.09.24.06.21.46 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 24 Sep 2019 06:21:47 -0700 (PDT) Subject: Re: [edk2-devel] [patch v3 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: <20190924131644.10412-1-dandan.bi@intel.com> <20190924131644.10412-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: <82949612-609a-27cf-2800-04ce8033ab9d@redhat.com> Date: Tue, 24 Sep 2019 15:21: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: <20190924131644.10412-4-dandan.bi@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Hao A Wu > Cc: Ray Ni > Cc: Zhichao Gao > Cc: Liming Gao > Cc: Laszlo Ersek > Cc: Philippe Mathieu-Daude > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1992 > Signed-off-by: Dandan Bi > --- > 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.
> +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,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.
> +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,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 > LoadCount++; > // > // Before calling the image, enable the Watchdog Timer for > // a 5 Minute period > // >