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; Wed, 04 Sep 2019 07:16:45 -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 4574E81F25 for ; Wed, 4 Sep 2019 14:16:44 +0000 (UTC) Received: by mail-wr1-f69.google.com with SMTP id b9so8354510wrt.5 for ; Wed, 04 Sep 2019 07:16:44 -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=Fy4sigIHHuW5OIayG6E83eCLFTnXbV/ti5BPuGL2vsE=; b=HfSgoGc2TJiRWMmBYkuZvkA4T4dWZG8HSnRR4hLqx4Omq1PjUBpAvdiQzIf7MB/nj+ pT8Vs9YL/En0lKUju54J3hE1pEJcdKm2LSoYN/1MdoMeOlSX5Dzk3k52oCkko6unyvZL /hXF3zHehmICad0lU/5z7RgBBhsKo6ESBKOezLT73mmjZezL20VUpIuxRYPQ0J2wyrGk EtLv5SNDE7Sov2sg3kJLmRBN6dX45aFrWoKJwK6ktW3g2/ulhTKBXLauYVwip3xiNzJo 7OV0n1jX0z6zHI8Fqu/xG0TmevcriVN0NAo2Bawf9r3f2B1NJdGA4+qoTpJpfJD8Z/yG dOtw== X-Gm-Message-State: APjAAAW4i/8gbY4B9dj6knOFFiarOXOnklbZyf/RMEzbZzcRj108lFep aebR373nyT1kurDPdDtO+cTd5xDy7gw1Rrf00pGlxTSREbwE221p4G1QA7twcxUlUX7u9d/4srC 1xnQEAavrrHucfg== X-Received: by 2002:adf:dc89:: with SMTP id r9mr1139650wrj.139.1567606603030; Wed, 04 Sep 2019 07:16:43 -0700 (PDT) X-Google-Smtp-Source: APXvYqyyFqvey1Zlg3uK9mweisiqBXefmqKPuGgERJjVm/lp6+FzaXGczVh/oaBPy+DjH7NepowmKw== X-Received: by 2002:adf:dc89:: with SMTP id r9mr1139615wrj.139.1567606602819; Wed, 04 Sep 2019 07:16:42 -0700 (PDT) Received: from [10.201.33.84] ([195.166.127.210]) by smtp.gmail.com with ESMTPSA id f15sm4453159wml.8.2019.09.04.07.16.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 04 Sep 2019 07:16:42 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH] ArmVirtPkg/PlatformBootManagerLib: unload image on EFI_SECURITY_VIOLATION To: devel@edk2.groups.io, lersek@redhat.com Cc: Ard Biesheuvel , Dandan Bi , Leif Lindholm References: <20190903163801.28652-1-lersek@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Openpgp: id=89C1E78F601EE86C867495CBA2A3FD6EDEADC0DE; url=http://pgp.mit.edu/pks/lookup?op=get&search=0xA2A3FD6EDEADC0DE Message-ID: Date: Wed, 4 Sep 2019 16:16:41 +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: <20190903163801.28652-1-lersek@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 9/3/19 6:38 PM, Laszlo Ersek wrote: > The LoadImage() boot service is a bit unusual in that it allocates > resources in a particular failure case; namely, it produces a valid > "ImageHandle" when it returns EFI_SECURITY_VIOLATION. This is supposed to > happen e.g. when Secure Boot verification fails for the image, but the > platform policy for the particular image origin (such as "fixed media" or > "removable media") is DEFER_EXECUTE_ON_SECURITY_VIOLATION. The return code > allows platform logic to selectively override the verification failure, > and launch the image nonetheless. > > ArmVirtPkg/PlatformBootManagerLib does not override EFI_SECURITY_VIOLATION > for the kernel image loaded from fw_cfg -- any LoadImage() error is > considered fatal. When we simply treat EFI_SECURITY_VIOLATION like any > other LoadImage() error, we leak the resources associated with > "KernelImageHandle". From a resource usage perspective, > EFI_SECURITY_VIOLATION must be considered "success", and rolled back. > > Implement this rollback, without breaking the proper "nesting" of error > handling jumps and labels. > > Cc: Ard Biesheuvel > Cc: Dandan Bi > Cc: Leif Lindholm > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1992 > Fixes: 23d04b58e27b382bbd3f9b16ba9adb1cb203dad5 > Signed-off-by: Laszlo Ersek Reviewed-by: Philippe Mathieu-Daude > --- > > Notes: > Repo: https://github.com/lersek/edk2.git > Branch: ldimg_armvirt_bz1992 > > ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c b/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c > index 3cc83e3b7b95..d3851fd75fa5 100644 > --- a/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c > +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c > @@ -968,53 +968,60 @@ TryRunningQemuKernel ( > > // > // Create a device path for the kernel image to be loaded from that will call > // back into our file system. > // > KernelDevicePath = FileDevicePath (FileSystemHandle, KernelBlob->Name); > if (KernelDevicePath == NULL) { > DEBUG ((EFI_D_ERROR, "%a: failed to allocate kernel device path\n", > __FUNCTION__)); > Status = EFI_OUT_OF_RESOURCES; > goto UninstallProtocols; > } > > // > // Load the image. This should call back into our file system. > // > Status = gBS->LoadImage ( > FALSE, // BootPolicy: exact match required > gImageHandle, // ParentImageHandle > KernelDevicePath, > NULL, // SourceBuffer > 0, // SourceSize > &KernelImageHandle > ); > if (EFI_ERROR (Status)) { > DEBUG ((EFI_D_ERROR, "%a: LoadImage(): %r\n", __FUNCTION__, Status)); > - goto FreeKernelDevicePath; > + if (Status != EFI_SECURITY_VIOLATION) { > + goto FreeKernelDevicePath; > + } > + // > + // From the resource allocation perspective, EFI_SECURITY_VIOLATION means > + // "success", so we must roll back the image loading. > + // > + goto UnloadKernelImage; > } > > // > // Construct the kernel command line. > // > Status = gBS->OpenProtocol ( > KernelImageHandle, > &gEfiLoadedImageProtocolGuid, > (VOID **)&KernelLoadedImage, > gImageHandle, // AgentHandle > NULL, // ControllerHandle > EFI_OPEN_PROTOCOL_GET_PROTOCOL > ); > ASSERT_EFI_ERROR (Status); > > if (CommandLineBlob->Data == NULL) { > KernelLoadedImage->LoadOptionsSize = 0; > } else { > // > // Verify NUL-termination of the command line. > // > if (CommandLineBlob->Data[CommandLineBlob->Size - 1] != '\0') { > DEBUG ((EFI_D_ERROR, "%a: kernel command line is not NUL-terminated\n", > __FUNCTION__)); > Status = EFI_PROTOCOL_ERROR; > goto UnloadKernelImage; >