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:27:21 -0700 Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.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 094C686662 for ; Tue, 24 Sep 2019 10:27:21 +0000 (UTC) Received: by mail-wr1-f71.google.com with SMTP id w8so404754wrm.3 for ; Tue, 24 Sep 2019 03:27:20 -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=bFRz7xd9vtJtq9RXlwHhXumkCp5qM1ZPLWB4yNeZPcM=; b=e7T5+5IbPM1GnwPwj+B4A8UhHWU9PdePRBBL6hfg5Wnv8V4cQ9DSjrq0l+87lX3nGk g/nROwOf2iN/bjQKuWlw74SpIgNM37nC+tclI4n94GuOW+jwceS3wOYZfo6bezuqo5ow swKVDWqIRoTXaj19nMH1hG0Y1FMUKq3s0rWvkBAAvm379IMqce/c+gUGnlyDvMDx3zwA 5wh1zkhTJ3Pzc39WiLuSn13iRa9CmPweMoF5P4vKajJnRE/HTWOMfogbfyxuhSxBHyMl XxJ95CAcSvLJY8Mwzsz7+Gr7Du3vajPPDs14GkF0JP7/PhQM+TGvTkPiiM23ZYjf9mdB Tk4g== X-Gm-Message-State: APjAAAUc0lLaLWJctXx74T7WFVvEq5L7k43v9Xok7CYc5DHqLpKeMB2U fg1y6RTeOQzCf5lmXKJpRMT6MYe7pLdL8NOK/nKDLpyX+xLmt4ln2FrpBa9/5XJGqt/qq79CH7s 3OhfqGFeEZ5NS5A== X-Received: by 2002:a1c:968b:: with SMTP id y133mr2028006wmd.56.1569320839829; Tue, 24 Sep 2019 03:27:19 -0700 (PDT) X-Google-Smtp-Source: APXvYqyrLZR8iuasQ/cpMHkp3s3AQ9HGkrgFjhBYIDWooU5CGvfbjzOF7XHOmgf5cR5qlGYgzR+YDQ== X-Received: by 2002:a1c:968b:: with SMTP id y133mr2027986wmd.56.1569320839650; Tue, 24 Sep 2019 03:27:19 -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 f18sm1095758wrv.38.2019.09.24.03.27.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 24 Sep 2019 03:27:19 -0700 (PDT) Subject: Re: [edk2-devel] [patch v2 1/5] EmbeddedPkg: Unload image on EFI_SECURITY_VIOLATION To: devel@edk2.groups.io, dandan.bi@intel.com Cc: Leif Lindholm , Ard Biesheuvel , Laszlo Ersek References: <20190918030557.55256-1-dandan.bi@intel.com> <20190918030557.55256-2-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:27:18 +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-2-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 EmbeddedPkg which don't have the policy to defer the > execution of the image. > > Cc: Leif Lindholm > Cc: Ard Biesheuvel > Cc: Laszlo Ersek > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1992 > Signed-off-by: Dandan Bi > Acked-by: Ard Biesheuvel > Acked-by: Laszlo Ersek > --- > .../AndroidFastboot/Arm/BootAndroidBootImg.c | 9 +++++++++ > .../Library/AndroidBootImgLib/AndroidBootImgLib.c | 12 ++++++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c > index 591afbe7cc..fe05878b4b 100644 > --- a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c > +++ b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c > @@ -71,10 +71,19 @@ StartEfiApplication ( > > // Load the image from the device path with Boot Services function > Status = gBS->LoadImage (TRUE, ParentImageHandle, DevicePath, NULL, 0, > &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); > + } > return Status; > } > > // Passed LoadOptions to the EFI Application > if (LoadOptionsSize != 0) { > diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c > index d9e7aa7d2b..e1036954ee 100644 > --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c > +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c > @@ -439,10 +439,22 @@ AndroidBootImgBoot ( > + KernelSize; > > Status = gBS->LoadImage (TRUE, gImageHandle, > (EFI_DEVICE_PATH *)&KernelDevicePath, > (VOID*)(UINTN)Kernel, KernelSize, &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); > + } > + return Status; > + } > > // Set kernel arguments > Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid, > (VOID **) &ImageInfo); > ImageInfo->LoadOptions = NewKernelArg; > Reviewed-by: Philippe Mathieu-Daude