From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web11.17775.1583169180138250660 for ; Mon, 02 Mar 2020 09:13:00 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=LKDPCBDO; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583169179; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=I0iI4ihf1GzyCDsnHy62GmeVdhYnpDM+zRV6ZI9qR6w=; b=LKDPCBDOpE0ZuvKQnt6XQ9UJsqFPjeVOuS+KKYMDS0n3g3t6LHGlGWGUH9KNS7ndnVR/yn yXSZJ3ali31crBEyYmFXcv5EBF265QCA8FwTTuaj73G1/4BEiXCIgikgxq2/FCv7Mz/uo1 ozMf+U/NNXQ6qxaIFJwy+0+2jtpgeqk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-80-CIID_VEvNhSHg8CYHye1TA-1; Mon, 02 Mar 2020 12:12:54 -0500 X-MC-Unique: CIID_VEvNhSHg8CYHye1TA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7640E1084430; Mon, 2 Mar 2020 17:12:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-197.ams2.redhat.com [10.36.117.197]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7932187B1A; Mon, 2 Mar 2020 17:12:52 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 04/13] OvmfPkg: provide a generic implementation of QemuLoadImageLib To: devel@edk2.groups.io, ard.biesheuvel@linaro.org References: <20200302072936.29221-1-ard.biesheuvel@linaro.org> <20200302072936.29221-5-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: <0be0b190-c32b-66b7-1dcb-ecd2105406f5@redhat.com> Date: Mon, 2 Mar 2020 18:12:51 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200302072936.29221-5-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/02/20 08:29, Ard Biesheuvel wrote: > Implement QemuLoadImageLib, and make it load the image provided by the > QEMU_EFI_LOADER_FS_MEDIA_GUID/kernel device path that we implemented > in a preceding patch in a separate DXE driver, using only the standard > LoadImage and StartImage boot services. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2566 > Signed-off-by: Ard Biesheuvel > --- > OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 253 ++++++++++++++++++++ > OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf | 39 +++ > 2 files changed, 292 insertions(+) > > diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c > new file mode 100644 > index 000000000000..c48c7a88dd91 > --- /dev/null > +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c > @@ -0,0 +1,253 @@ > +/** @file > + Generic implementation of QemuLoadImageLib library class interface. > + > + Copyright (c) 2020, ARM Ltd. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#include > + > +#include > +#include > +#include > +#include > +#include (1) I think it would be nicer if, in this patch, we didn't access fw_cfg at all. The filesystem already exposes "cmdline", we could use that. ... Except, I can see you are removing that in patch #7... OK, I guess. > +#include > +#include > +#include > + > +#pragma pack (1) > +typedef struct { > + EFI_DEVICE_PATH_PROTOCOL FilePathHeader; > + CHAR16 FilePath[sizeof (L"kernel")]; > +} KERNEL_FILE_DEVPATH; (2) The idea is very nice, but the size of the FilePath array is larger than necessary. L"kernel" is an array of CHAR16, sizeof returns number of bytes, and the element type of FilePath is CHAR16; so we end up allocating doubly. Please use instead: CHAR16 FilePath[ARRAY_SIZE (L"kernel")]; (Please #include for this, too.) > + > +typedef struct { > + VENDOR_DEVICE_PATH VenMediaNode; > + KERNEL_FILE_DEVPATH FileNode; > + EFI_DEVICE_PATH_PROTOCOL EndNode; > +} KERNEL_VENMEDIA_FILE_DEVPATH; > +#pragma pack () > + > +STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = { > + { > + { > + MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP, > + { sizeof (VENDOR_DEVICE_PATH) } > + }, > + QEMU_KERNEL_LOADER_FS_MEDIA_GUID > + }, { > + { > + MEDIA_DEVICE_PATH, MEDIA_FILEPATH_DP, > + { sizeof (KERNEL_FILE_DEVPATH) } > + }, > + L"kernel", > + }, { > + END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE, > + { sizeof (EFI_DEVICE_PATH_PROTOCOL) } > + } > +}; > + > +/** > + Download the kernel, the initial ramdisk, and the kernel command line from > + QEMU's fw_cfg. The kernel will be instructed via its command line to load > + the initrd from the same Simple FileSystem. (3) Please sync the comments here with the lib class header, according to my requests for the lib class header (two updates). > + > + @param[out] ImageHandle The image handle that was allocated for > + loading the image > + @param[out] LoadedImage The loaded image protocol that was installed > + on ImageHandle by the LoadImage boot service. > + > + @retval EFI_SUCCESS The image was loaded successfully. > + @retval EFI_NOT_FOUND Kernel image was not found. > + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. > + @retval EFI_PROTOCOL_ERROR Unterminated kernel command line. > + > + @return Error codes from any of the underlying > + functions. > +**/ > +EFI_STATUS > +EFIAPI > +QemuLoadKernelImage ( > + OUT EFI_HANDLE *ImageHandle > + ) > +{ > + EFI_STATUS Status; > + EFI_HANDLE KernelImageHandle; > + EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage; > + UINTN CommandLineSize; > + CHAR8 *CommandLine; > + UINTN InitrdSize; > + > + // > + // Load the image. This should call back into the QEMU EFI loader file system. > + // > + Status = gBS->LoadImage ( > + FALSE, // BootPolicy: exact match required > + gImageHandle, // ParentImageHandle > + (EFI_DEVICE_PATH_PROTOCOL *)&mKernelDevicePath, > + NULL, // SourceBuffer > + 0, // SourceSize > + &KernelImageHandle > + ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: LoadImage(): %r\n", __FUNCTION__, Status)); > + return Status; > + } (4) OK, so we got a bit of a complication here, with regard to EFI_SECURITY_VIOLATION. Per spec, when gBS->LoadImage() returns EFI_SECURITY_VIOLATION, the image will have been loaded. If we don't release it with gBS->UnloadImage(), then it will be leaked. Now, there are two ways to treat this issue, given this library class: (4a) Handle EFI_SECURITY_VIOLATION internally to the library; that is, do not allow the caller to explicitly override the security violation (IOW don't allow the caller to do something with the output ImageHandle *despite* the security violation). In this case: - Catch EFI_SECURITY_VIOLATION here explicitly, - set status to EFI_ACCESS_DENIED, - set (*ImageHandle) to NULL, - introduce an additional label just above gBS->UnloadImage(), - jump to that label from here, - update the leading comments in both the lib class header and in this lib instance to state that EFI_SECURITY_VIOLATION is caught internally, and EFI_ACCESS_DENIED is returned instead, always. (4b) Expose the generality of gBS->LoadImage() through this library interface: - document the special behavior of EFI_SECURITY_VIOLATION in the lib class header (necessitating the caller to recognize that error code, and then to unload the kernel image with QemuUnloadKernelImage()), - actually implement the special handling for EFI_SECURITY_VIOLATION here. I would strongly suggest (4a). Now, I realize that (4a) requires the removal of the special handling of EFI_SECURITY_VIOLATION in patch #6. I still think that's preferable, because even if we propagate EFI_SECURITY_VIOLATION out of this function, the bad security status of the ImageHandle that we output, will not be the *only* thing amiss. Namely, we will not have set up the command line. And so the caller will be left with two choices: - release the image at once, - "trust" the image nonetheless... but what about the command line, then? In other words, the way the patch set looks right now, we allow EFI_SECURITY_VIOLATION to get out of the function, but it's not really useful to the caller. Given that we're turning this boundary into an actual lib class API, I think we should either keep EFI_SECURITY_VIOLATION internal -- (4a) -- or pass it out full-featured -- (4b) --. And then I think (4a) is much better here (for simplicity). > + > + // > + // 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); > + > + QemuFwCfgSelectItem (QemuFwCfgItemCommandLineSize); > + CommandLineSize = (UINTN) QemuFwCfgRead64 (); (5) This should be QemuFwCfgRead32(). See FetchBlob() the original code: Blob->Size = QemuFwCfgRead32 (); > + > + if (CommandLineSize == 0) { > + KernelLoadedImage->LoadOptionsSize = 0; > + } else { > + CommandLine = AllocatePool (CommandLineSize); > + ASSERT (CommandLine != NULL); (6) It's not very difficult to jump to the end of the function here. PLease use an explicit check and a goto, for releasing KernelImageHandle. > + > + QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData); > + QemuFwCfgReadBytes (CommandLineSize, CommandLine); > + > + // > + // Verify NUL-termination of the command line. > + // > + if (CommandLine[CommandLineSize - 1] != '\0') { > + DEBUG ((DEBUG_ERROR, "%a: kernel command line is not NUL-terminated\n", > + __FUNCTION__)); > + Status = EFI_PROTOCOL_ERROR; > + goto FreeCommandLine; > + } > + > + // > + // Drop the terminating NUL, convert to UTF-16. > + // > + KernelLoadedImage->LoadOptionsSize = (CommandLineSize - 1) * 2; > + } > + > + QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize); > + InitrdSize = (UINTN) QemuFwCfgRead64 (); (7) Should be QemuFwCfgRead32(). > + > + if (InitrdSize > 0) { > + // > + // Append ' initrd=initrd' in UTF-16. > + // > + KernelLoadedImage->LoadOptionsSize += sizeof (L" initrd=initrd") - 2; > + } > + > + if (KernelLoadedImage->LoadOptionsSize == 0) { > + KernelLoadedImage->LoadOptions = NULL; > + } else { > + // > + // NUL-terminate in UTF-16. > + // > + KernelLoadedImage->LoadOptionsSize += 2; > + > + KernelLoadedImage->LoadOptions = AllocatePool ( > + KernelLoadedImage->LoadOptionsSize); > + if (KernelLoadedImage->LoadOptions == NULL) { > + KernelLoadedImage->LoadOptionsSize = 0; > + Status = EFI_OUT_OF_RESOURCES; > + goto FreeCommandLine; It's possible that we don't have a kernel commandline via fw_cfg, but we're still appending " initrd=initrd" because we do have an initial ramdisk. In that case, CommandLine will not have been assigned, and the FreePool() in the end will blow up. (8) So this "goto" is correct, but the final FreePool() should be conditional on (CommandLineSize > 0). > + } > + > + UnicodeSPrintAsciiFormat ( > + KernelLoadedImage->LoadOptions, > + KernelLoadedImage->LoadOptionsSize, > + "%a%a", > + (CommandLineSize == 0) ? "" : CommandLine, > + (InitrdSize == 0) ? "" : " initrd=initrd" > + ); > + DEBUG ((DEBUG_INFO, "%a: command line: \"%s\"\n", __FUNCTION__, > + (CHAR16 *)KernelLoadedImage->LoadOptions)); > + } > + > + *ImageHandle = KernelImageHandle; > + return EFI_SUCCESS; > + > +FreeCommandLine: > + FreePool (CommandLine); > + gBS->UnloadImage (KernelImageHandle); > + > + return Status; > +} > + > +/** > + Transfer control to a kernel image loaded with QemuLoadKernelImage () > + > + @param[in] ImageHandle Handle of image to be started. > + > + @retval EFI_INVALID_PARAMETER ImageHandle is either an invalid image handle > + or the image has already been initialized with > + StartImage > + @retval EFI_SECURITY_VIOLATION The current platform policy specifies that the > + image should not be started. > + > + @return Error codes returned by the started image > +**/ > +EFI_STATUS > +EFIAPI > +QemuStartKernelImage ( > + IN EFI_HANDLE ImageHandle > + ) > +{ > + return gBS->StartImage ( > + ImageHandle, > + NULL, // ExitDataSize > + NULL // ExitData > + ); > +} > + > +/** > + Unloads an image loaded with QemuLoadKernelImage (). > + > + @param ImageHandle Handle that identifies the image to be > + unloaded. > + > + @retval EFI_SUCCESS The image has been unloaded. > + @retval EFI_UNSUPPORTED The image has been started, and does not > + support unload. > + @retval EFI_INVALID_PARAMETER ImageHandle is not a valid image handle. > + > +**/ > +EFI_STATUS > +EFIAPI > +QemuUnloadKernelImage ( > + IN EFI_HANDLE ImageHandle > + ) > +{ > + EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage; > + EFI_STATUS Status; > + > + Status = gBS->OpenProtocol ( > + ImageHandle, > + &gEfiLoadedImageProtocolGuid, > + (VOID **)&KernelLoadedImage, > + gImageHandle, // AgentHandle > + NULL, // ControllerHandle > + EFI_OPEN_PROTOCOL_GET_PROTOCOL > + ); > + if (EFI_ERROR (Status)) { > + return EFI_INVALID_PARAMETER; > + } > + > + if (KernelLoadedImage->LoadOptions != NULL) { > + FreePool (KernelLoadedImage->LoadOptions); > + KernelLoadedImage->LoadOptions = NULL; > + } > + KernelLoadedImage->LoadOptionsSize = 0; > + > + return gBS->UnloadImage (ImageHandle); > +} Seems OK. > diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf > new file mode 100644 > index 000000000000..cbd40e6290e0 > --- /dev/null > +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf > @@ -0,0 +1,39 @@ > +## @file > +# Generic implementation of QemuLoadImageLib library class interface. > +# > +# Copyright (c) 2020, ARM Ltd. All rights reserved.
> +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +## > + > +[Defines] > + INF_VERSION = 1.27 > + BASE_NAME = GenericQemuLoadImageLib > + FILE_GUID = 9e3e28da-c7b5-4f85-841a-84e6a9a1f1a0 > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = QemuLoadImageLib|DXE_DRIVER > + > +[Sources] > + GenericQemuLoadImageLib.c > + > +[Packages] > + MdeModulePkg/MdeModulePkg.dec > + MdePkg/MdePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + DebugLib > + MemoryAllocationLib > + PrintLib > + QemuFwCfgLib > + ReportStatusCodeLib OK, so from patch #6, it's clear that EfiSignalEventReadyToBoot() and REPORT_STATUS_CODE() remain in the original spot. (9) That looks good; but then we don't need ReportStatusCodeLib here (the header file is not included either, anyway). > + UefiBootServicesTableLib > + > +[Protocols] > + gEfiDevicePathProtocolGuid Not strictly needed by this patch, but in order to match the #include directives, it seems OK. > + gEfiLoadedImageProtocolGuid > + > +[Guids] > + gQemuKernelLoaderFsMediaGuid > Same -- OK. Thanks, Laszlo