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:30:17 -0700 Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.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 442B13B708 for ; Tue, 24 Sep 2019 10:30:16 +0000 (UTC) Received: by mail-wr1-f69.google.com with SMTP id f11so395565wrt.18 for ; Tue, 24 Sep 2019 03:30:16 -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=nJ7onvpq/rHqBWuZemVpKY1SYdk4EKT0L6nIUwCz3SE=; b=C5Vr0tALJH03As0OMFYFt69tCQdXqNcMOGkR5ENizKzKCOcq2Qe+zPDyvxQ3FmOXhb f+zgHaR0rqSsYUTOtBZve90ovGXPDc579xlMl4xqG28ZECbhPOAiL/EON0dnxFmOJLWK 75jFz3ENT7k+U7y49fk0VdaIjKm6wl8pI45OHMbmlRSEMfpPqu2g7HkqPvLRWrB72Q5d U3c+X+1tjki6v8ejq4MONnCzXo9MtZWrIOusqnd1BwIY36/DE/xuuU6ZOXGhkBHyFv7h LsuqSwSdAwGgBQJXCiXG9/pBVZbeJl5xg7TvtJCBGh2az5jCTAGOulb5fvO3F6xBQHeT L5QQ== X-Gm-Message-State: APjAAAVaa37IdcaEoYhKyPEfPJOhIKJ9h3iFcLM/FK7h/96W1upEEvOX GK2AN85zd1gGZrqiZ5Kt9WusloJKCp1AQyki7H3hXvzEJVDFficTQZKRy/cRJPhWOpBVVauqHGD E92F/he4tvsYgrA== X-Received: by 2002:a05:600c:238a:: with SMTP id m10mr2245342wma.51.1569321015029; Tue, 24 Sep 2019 03:30:15 -0700 (PDT) X-Google-Smtp-Source: APXvYqwOKzKJxb07w3/UD6jqSs1U0M2rFeWCaJruKa+WWmtzb2LYxdf5NLVQBJY6RRUKcMpBcUnerQ== X-Received: by 2002:a05:600c:238a:: with SMTP id m10mr2245329wma.51.1569321014784; Tue, 24 Sep 2019 03:30:14 -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 u22sm2495547wru.72.2019.09.24.03.30.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 24 Sep 2019 03:30:13 -0700 (PDT) Subject: Re: [edk2-devel] [patch v2 5/5] ShellPkg: Unload image on EFI_SECURITY_VIOLATION To: devel@edk2.groups.io, dandan.bi@intel.com Cc: Ray Ni , Zhichao Gao , Laszlo Ersek References: <20190918030557.55256-1-dandan.bi@intel.com> <20190918030557.55256-6-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:30:12 +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-6-dandan.bi@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 ShellPkg which don't have the policy to defer the > execution of the image. > > Cc: Ray Ni > Cc: Zhichao Gao > Cc: Laszlo Ersek > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1992 > Signed-off-by: Dandan Bi > Reviewed-by: Zhichao Gao > --- > ShellPkg/Application/Shell/ShellManParser.c | 9 +++++++++ > .../Library/UefiShellDebug1CommandsLib/LoadPciRom.c | 11 ++++++++++- > ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c | 11 ++++++++++- > 3 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/ShellPkg/Application/Shell/ShellManParser.c b/ShellPkg/Application/Shell/ShellManParser.c > index 6909f29441..4d5a5668aa 100644 > --- a/ShellPkg/Application/Shell/ShellManParser.c > +++ b/ShellPkg/Application/Shell/ShellManParser.c > @@ -643,10 +643,19 @@ ProcessManFile( > goto Done; > } > DevPath = ShellInfoObject.NewEfiShellProtocol->GetDevicePathFromFilePath(CmdFilePathName); > Status = gBS->LoadImage(FALSE, gImageHandle, DevPath, NULL, 0, &CmdFileImgHandle); > 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 the resource leak. > + // > + if (Status == EFI_SECURITY_VIOLATION) { > + gBS->UnloadImage (CmdFileImgHandle); > + } OK > *HelpText = NULL; > goto Done; > } > Status = gBS->OpenProtocol( > CmdFileImgHandle, > diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c > index 1b169d0d3c..5b6cba17f3 100644 > --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c > +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/LoadPciRom.c > @@ -1,10 +1,10 @@ > /** @file > Main file for LoadPciRom shell Debug1 function. > > (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
> - Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.
> + Copyright (c) 2005 - 2019, Intel Corporation. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > > #include "UefiShellDebug1CommandsLib.h" > @@ -332,10 +332,19 @@ LoadEfiDriversFromRomImage ( > ImageBuffer, > ImageLength, > &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 == EFI_SECURITY_VIOLATION) { > + gBS->UnloadImage (ImageHandle); > + } OK > ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_LOADPCIROM_LOAD_FAIL), gShellDebug1HiiHandle, L"loadpcirom", FileName, ImageIndex); > // PrintToken (STRING_TOKEN (STR_LOADPCIROM_LOAD_IMAGE_ERROR), HiiHandle, ImageIndex, Status); > } else { > Status = gBS->StartImage (ImageHandle, NULL, NULL); > if (EFI_ERROR (Status)) { > diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c > index 6a94b48c86..b6e7c952fa 100644 > --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c > +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Load.c > @@ -1,10 +1,10 @@ > /** @file > Main file for attrib shell level 2 function. > > (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
> - Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
> + Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > > #include "UefiShellLevel2CommandsLib.h" > @@ -110,10 +110,19 @@ LoadDriver( > NULL, > 0, > &LoadedDriverHandle); > > 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 (LoadedDriverHandle); > + } OK. Reviewed-by: Philippe Mathieu-Daude > ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_LOAD_NOT_IMAGE), gShellLevel2HiiHandle, FileName, Status); > } else { > // > // Make sure it is a driver image > // >