[edk2-devel] [PATCH v2 2/3] OvmfPkg/GenericQemuLoadImageLib: Read cmdline from QemuKernelLoaderFs
Laszlo Ersek
lersek at redhat.com
Thu Jun 24 17:49:21 UTC 2021
Hi Dov,
On 06/17/21 14:16, Dov Murik wrote:
> Remove the QemuFwCfgLib interface used to read the QEMU cmdline
> (-append argument) and the initrd size. Instead, use the synthetic
> filesystem QemuKernelLoaderFs which has three files: "kernel", "initrd",
> and "cmdline".
>
> Cc: Laszlo Ersek <lersek at redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
> Cc: Jordan Justen <jordan.l.justen at intel.com>
> Cc: James Bottomley <jejb at linux.ibm.com>
> Cc: Tobin Feldman-Fitzthum <tobin at linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik at linux.ibm.com>
> ---
> OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf | 2 +-
> OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 145 ++++++++++++++++++--
> 2 files changed, 133 insertions(+), 14 deletions(-)
This update seems to address everything that Ard requested under v1;
thanks.
My comments:
(1) I spent a lot of time reviewing your patch. Unfortunately, I found a
preexistent bug in both QemuLoadImageLib instances, which we should fix
first, in two separate patches.
The bug was introduced in commit ddd2be6b0026 ("OvmfPkg: provide a
generic implementation of QemuLoadImageLib", 2020-03-05). Unfortunately
I missed the bug in my original review.
In said commit, the QemuLoadKernelImage() function
[OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c]
refactored / reimplemented the logic from the TryRunningQemuKernel()
function [ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c].
If we now check out the tree at ddd2be6b0026, and compare the above two
functions, we notice the following:
(1a) TryRunningQemuKernel() downloads all three blobs via fw_cfg in the
beginning, and *always* frees all successfully downloaded blobs at the
end, under the "FreeBlobs" label.
(1b) In QemuLoadKernelImage(), the kernel and initrd fw_cfg blobs are
owned by the QemuKernelLoaderFsDxe driver; only the command line blob is
downloaded from fw_cfg. Not freeing the former two blobs (kernel and
initrd) makes sense. *However*, the command line blob should *still* be
freed, even if QemuLoadKernelImage() succeeds! That's because we have no
use for the command line fw_cfg blob, after it is translated to
LoadOptions.
The bug is that QemuLoadKernelImage() leaks "CommandLine" on success.
The same issue was introduced in the other lib instance
[OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c], in commit
7c47d89003a6 ("OvmfPkg: implement QEMU loader library for X86 with
legacy fallback", 2020-03-05).
The fix is identical between both library instances:
> @@ -193,14 +193,16 @@ QemuLoadKernelImage (
> }
>
> *ImageHandle = KernelImageHandle;
> - return EFI_SUCCESS;
> + Status = EFI_SUCCESS;
>
> FreeCommandLine:
> if (CommandLineSize > 0) {
> FreePool (CommandLine);
> }
> UnloadImage:
> - gBS->UnloadImage (KernelImageHandle);
> + if (EFI_ERROR (Status)) {
> + gBS->UnloadImage (KernelImageHandle);
> + }
>
> return Status;
> }
Can you please submit this fix twice, in two separate patches at the
*very front* of this series, one patch for each lib instance? Something
like:
#1 OvmfPkg/GenericQemuLoadImageLib: plug cmdline blob leak on success
...
Reported-by: Laszlo Ersek <lersek at redhat.com>
Fixes: ddd2be6b0026abcd0f819b3915fc80c3de81dd62
#2 OvmfPkg/X86QemuLoadImageLib: plug cmdline blob leak on success
...
Reported-by: Laszlo Ersek <lersek at redhat.com>
Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
Thank you in advance!
Then, comments on your actual patch:
(2) The bugzilla ticket should be referenced in the commit message
please, above your signoff:
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
On 06/17/21 14:16, Dov Murik wrote:
>
> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
> index b262cb926a4d..f462fd6922cf 100644
> --- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
> @@ -27,12 +27,12 @@ [LibraryClasses]
> DebugLib
> MemoryAllocationLib
> PrintLib
> - QemuFwCfgLib
> UefiBootServicesTableLib
>
> [Protocols]
> gEfiDevicePathProtocolGuid
> gEfiLoadedImageProtocolGuid
> + gEfiSimpleFileSystemProtocolGuid
>
> [Guids]
> gQemuKernelLoaderFsMediaGuid
(3) The FileHandleLib class should be added, under [LibraryClasses].
> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> index 114db7e8441f..f520456e3b24 100644
> --- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
> @@ -11,9 +11,9 @@
> #include <Base.h>
> #include <Guid/QemuKernelLoaderFsMedia.h>
> #include <Library/DebugLib.h>
> +#include <Library/FileHandleLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/PrintLib.h>
> -#include <Library/QemuFwCfgLib.h>
> #include <Library/QemuLoadImageLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Protocol/DevicePath.h>
(4) The new "gEfiSimpleFileSystemProtocolGuid" dependency should be
reflected here too, by adding:
#include <Protocol/SimpleFileSystem.h>
(In general the [Protocols] section of the INF file should be matched by
#include <Protocol/...> directives.)
This was masked from you because <Library/FileHandleLib.h> pulled in
<Protocol/SimpleFileSystem.h>, but that's not enough justification for a
difference between the INF [Protocols] section and the #include
directive list.
> @@ -30,6 +30,11 @@ typedef struct {
> KERNEL_FILE_DEVPATH FileNode;
> EFI_DEVICE_PATH_PROTOCOL EndNode;
> } KERNEL_VENMEDIA_FILE_DEVPATH;
> +
> +typedef struct {
> + VENDOR_DEVICE_PATH VenMediaNode;
> + EFI_DEVICE_PATH_PROTOCOL EndNode;
> +} SINGLE_VENMEDIA_NODE_DEVPATH;
> #pragma pack ()
>
> STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
> @@ -51,6 +56,78 @@ STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
> }
> };
>
> +STATIC CONST SINGLE_VENMEDIA_NODE_DEVPATH mQemuKernelLoaderFileSystemDevicePath = {
(5) This variable name causes two overlong lines in the file; it should
be renamed to "mQemuKernelLoaderFsDevicePath" please.
> + {
> + {
> + MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP,
> + { sizeof (VENDOR_DEVICE_PATH) }
> + },
> + QEMU_KERNEL_LOADER_FS_MEDIA_GUID
> + }, {
> + END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
> + { sizeof (EFI_DEVICE_PATH_PROTOCOL) }
> + }
> +};
> +
> +STATIC
> +EFI_STATUS
> +GetQemuKernelLoaderBlobSize (
> + IN EFI_FILE_HANDLE Root,
> + IN CHAR16 *FileName,
> + OUT UINTN *Size
> + )
> +{
> + EFI_STATUS Status;
> + EFI_FILE_HANDLE FileHandle;
> + UINT64 FileSize;
> +
> + Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + Status = FileHandleGetSize (FileHandle, &FileSize);
> + if (EFI_ERROR (Status)) {
> + goto CloseFile;
> + }
> + *Size = FileSize;
(6) Silent truncation from UINT64 to UINTN, even if theoretical, is bad
practice. Please do this:
if (FileSize > MAX_UINTN) {
Status = EFI_UNSUPPORTED;
goto CloseFile;
}
*Size = (UINTN)FileSize;
> + Status = EFI_SUCCESS;
> +CloseFile:
> + FileHandle->Close (FileHandle);
> + return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +ReadWholeQemuKernelLoaderBlob (
> + IN EFI_FILE_HANDLE Root,
> + IN CHAR16 *FileName,
> + IN UINTN Size,
> + OUT VOID *Buffer
> + )
> +{
> + EFI_STATUS Status;
> + EFI_FILE_HANDLE FileHandle;
> + UINTN ReadSize;
> +
> + Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> + ReadSize = Size;
> + Status = FileHandle->Read (FileHandle, &ReadSize, Buffer);
> + if (EFI_ERROR (Status)) {
> + goto CloseFile;
> + }
> + if (ReadSize != Size) {
> + Status = EFI_PROTOCOL_ERROR;
> + goto CloseFile;
> + }
> + Status = EFI_SUCCESS;
> +CloseFile:
> + FileHandle->Close (FileHandle);
> + return Status;
> +}
> +
> /**
> 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
> @@ -76,12 +153,16 @@ QemuLoadKernelImage (
> OUT EFI_HANDLE *ImageHandle
> )
> {
> - EFI_STATUS Status;
> - EFI_HANDLE KernelImageHandle;
> - EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;
> - UINTN CommandLineSize;
> - CHAR8 *CommandLine;
> - UINTN InitrdSize;
> + EFI_STATUS Status;
> + EFI_HANDLE KernelImageHandle;
> + EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage;
> + EFI_DEVICE_PATH_PROTOCOL *DevicePathNode;
> + EFI_HANDLE FsVolumeHandle;
> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FsProtocol;
> + EFI_FILE_HANDLE Root;
> + UINTN CommandLineSize;
> + CHAR8 *CommandLine;
> + UINTN InitrdSize;
>
> //
> // Load the image. This should call back into the QEMU EFI loader file system.
> @@ -124,8 +205,38 @@ QemuLoadKernelImage (
> );
> ASSERT_EFI_ERROR (Status);
>
> - QemuFwCfgSelectItem (QemuFwCfgItemCommandLineSize);
> - CommandLineSize = (UINTN)QemuFwCfgRead32 ();
> + //
> + // Open the Qemu Kernel Loader abstract filesystem (volume) which will be
> + // used to read the "initrd" and "cmdline" synthetic files.
> + //
(7) This comment is welcome, but it is inexact.
We'll use the filesystem for reading the command line, yes, but
regarding the initrd, we use the filesystem only for learning the *size*
of the initrd. (And even the size of the initrd is only interesting
inasmuch a nonzero size means that an initrd is *present*.) The initrd
blob itself is not read by us.
I suggest:
used to query the "initrd" and to read the "cmdline" synthetic files.
> + DevicePathNode = (EFI_DEVICE_PATH_PROTOCOL *)&mQemuKernelLoaderFileSystemDevicePath;
> + Status = gBS->LocateDevicePath (
> + &gEfiSimpleFileSystemProtocolGuid,
> + &DevicePathNode,
> + &FsVolumeHandle
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
(8) This leaks "KernelImageHandle". At this point, gBS->LoadImage() at
the top of the function will have succeeded.
Please jump to the UnloadImage label, rather than returning.
> + }
> +
> + Status = gBS->HandleProtocol (
> + FsVolumeHandle,
> + &gEfiSimpleFileSystemProtocolGuid,
> + (VOID **)&FsProtocol
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
(9) Same leak as described in (8); please jump to the UnloadImage label.
> + }
> +
> + Status = FsProtocol->OpenVolume (FsVolumeHandle, &Root);
> + if (EFI_ERROR (Status)) {
> + return Status;
(10) Same leak as described in (8); please jump to the UnloadImage
label.
> + }
> +
> + Status = GetQemuKernelLoaderBlobSize (Root, L"cmdline", &CommandLineSize);
> + if (EFI_ERROR (Status)) {
> + goto CloseRoot;
> + }
>
> if (CommandLineSize == 0) {
> KernelLoadedImage->LoadOptionsSize = 0;
> @@ -136,8 +247,11 @@ QemuLoadKernelImage (
> goto UnloadImage;
> }
(11) Not fully shown in the context, but here we have:
if (CommandLineSize == 0) {
KernelLoadedImage->LoadOptionsSize = 0;
} else {
CommandLine = AllocatePool (CommandLineSize);
if (CommandLine == NULL) {
Status = EFI_OUT_OF_RESOURCES;
goto UnloadImage;
}
Note that we have a "goto UnloadImage" in it.
Please update that to "goto CloseRoot".
>
> - QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData);
> - QemuFwCfgReadBytes (CommandLineSize, CommandLine);
> + Status = ReadWholeQemuKernelLoaderBlob (Root, L"cmdline", CommandLineSize,
> + CommandLine);
> + if (EFI_ERROR (Status)) {
> + goto FreeCommandLine;
> + }
>
> //
> // Verify NUL-termination of the command line.
> @@ -155,8 +269,10 @@ QemuLoadKernelImage (
> KernelLoadedImage->LoadOptionsSize = (UINT32)((CommandLineSize - 1) * 2);
> }
>
> - QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize);
> - InitrdSize = (UINTN)QemuFwCfgRead32 ();
> + Status = GetQemuKernelLoaderBlobSize (Root, L"initrd", &InitrdSize);
> + if (EFI_ERROR (Status)) {
> + goto FreeCommandLine;
> + }
>
> if (InitrdSize > 0) {
> //
> @@ -193,6 +309,7 @@ QemuLoadKernelImage (
> }
>
> *ImageHandle = KernelImageHandle;
> + Root->Close (Root);
> return EFI_SUCCESS;
>
> FreeCommandLine:
> @@ -201,6 +318,8 @@ FreeCommandLine:
> }
> UnloadImage:
> gBS->UnloadImage (KernelImageHandle);
> +CloseRoot:
> + Root->Close (Root);
>
> return Status;
> }
>
(12) So, the order of handlers is incorrect here, and when I looked into
it, that was when I actually found preexistent issue (1).
The desired epilogue for the function is:
> *ImageHandle = KernelImageHandle;
> Status = EFI_SUCCESS;
>
> FreeCommandLine:
> if (CommandLineSize > 0) {
> FreePool (CommandLine);
> }
> CloseRoot:
> Root->Close (Root);
> UnloadImage:
> if (EFI_ERROR (Status)) {
> gBS->UnloadImage (KernelImageHandle);
> }
>
> return Status;
The idea is that CommandLine and Root are both temporaries, and as such
they need to be released on either success or failure. Whereas
KernelImageHandle must be released precisely on failure. Furthermore, in
either case, they must cascade as shown above -- in reverse order of
construction.
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77068): https://edk2.groups.io/g/devel/message/77068
Mute This Topic: https://groups.io/mt/83602584/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