Hi Laszlo,
On 24/06/2021 20:49, Laszlo Ersek wrote:
> 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 <ler...@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>> Cc: James Bottomley <j...@linux.ibm.com>
>> Cc: Tobin Feldman-Fitzthum <to...@linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmu...@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 <ler...@redhat.com>
> Fixes: ddd2be6b0026abcd0f819b3915fc80c3de81dd62
>
> #2 OvmfPkg/X86QemuLoadImageLib: plug cmdline blob leak on success
> ...
> Reported-by: Laszlo Ersek <ler...@redhat.com>
> Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc
>
> Thank you in advance!
OK, I'll add these two patches.
>
> 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
>
OK, I'll add it.
>
> 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].
>
OK.
>
>> 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.
I'll make sure the #include section matches the [Protocols].
>
>
>> @@ -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.
>
Good idea, I'll rename.
>
>> + {
>> + {
>> + 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;
>
OK.
>
>> + 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.
>
You're right. I wrote correctly in the commit message but not accurately
in this code comment. I'll update.
>
>> + 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.
>
OK.
>
>> + }
>> +
>> + 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.
>
OK.
>
>> + }
>> +
>> + Status = FsProtocol->OpenVolume (FsVolumeHandle, &Root);
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>
> (10) Same leak as described in (8); please jump to the UnloadImage
> label.
>
OK.
>
>> + }
>> +
>> + 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".
>
Yes, missed that. Thanks for catching it. I'll fix.
>
>>
>> - 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.
OK, I'll modify this.
Thanks,
-Dov
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77141): https://edk2.groups.io/g/devel/message/77141
Mute This Topic: https://groups.io/mt/83602584/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
