[edk2-devel] [PATCH 04/13] OvmfPkg: provide a generic implementation of QemuLoadImageLib
Laszlo Ersek
lersek at redhat.com
Mon Mar 2 17:12:51 UTC 2020
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 <ard.biesheuvel at linaro.org>
> ---
> 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.<BR>
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Uefi.h>
> +
> +#include <Guid/QemuKernelLoaderFsMedia.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PrintLib.h>
> +#include <Library/QemuFwCfgLib.h>
(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 <Library/UefiBootServicesTableLib.h>
> +#include <Protocol/DevicePath.h>
> +#include <Protocol/LoadedImage.h>
> +
> +#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 <Base.h> 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.<BR>
> +#
> +# 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55219): https://edk2.groups.io/g/devel/message/55219
Mute This Topic: https://groups.io/mt/71669017/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list