[RESEND] [PATCH v2 0/3] OvmfPkg: Use QemuKernelLoaderFs to read cmdline/initrd

[RESEND] [PATCH v2 0/3] OvmfPkg: Use QemuKernelLoaderFs to read cmdline/initrd

[Dov Murik]

[resending due to missing recipients]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3457

In order to support measured SEV boot with kernel/initrd/cmdline, we'd
like to have one place that reads those blobs; in the future we'll add
the measurement and verification in that place.

We already have a synthetic filesystem (QemuKernelLoaderFs) which holds
three files: "kernel", "initrd", and "cmdline".  The kernel is indeed
read from this filesystem in LoadImage; but the cmdline (and the length
of initrd) are read from QemuFwCfgLib items.

This patch series modifies GenericQemuLoadImageLib to read cmdline (and
the initrd size) from the QemuKernelLoaderFs synthetic filesystem, thus
removing the dependency on QemuFwCfgLib.

Note that X86QemuLoadImageLib is not modified, because it contains a
QemuLoadLegacyImage() which reads other items of the QemuFwCfg which are
not available in QemuKernelLoaderFs.  Since we don't want to support the
legacy boot path in the future measured SEV boot, we leave
X86QemuLoadImageLib as-is (except for a comment addition in patch 3) and
will force use for GenericQemuLoadImageLib in the measured SEV boot
implementation.

Relevant discussion threads start in:
https://edk2.groups.io/g/devel/message/76069

To test this on x86_64, I forced the use of GenericQemuLoadImageLib
using the following local patch:


diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 0a237a905866..46442b543bcf 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -404,7 +404,7 @@ [LibraryClasses.common.DXE_DRIVER]
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
-  QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
+  QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf # XXX don't commit this or someone will be mad
 !if $(TPM_ENABLE) == TRUE
   Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
   Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf


I tested boot with QEMU and OVMF with the following QEMU arguments:

  -kernel a
  -kernel a -initrd b
  -kernel a -cmdline c
  -kernel a -initrd b -cmdline c

(and also without -kernel)


Code is at
https://github.com/confidential-containers-demo/edk2/tree/use-synthetic-fs-for-cmdline-v2

v2 changes:

 - Add comment to header of X86QemuLoadImageLib.inf
 - Clearer function names in GenericQemuLoadImageLib.c
 - Fix coding style issues

v1: https://edk2.groups.io/g/devel/message/76265


Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>

Dov Murik (3):
  Revert "OvmfPkg/QemuKernelLoaderFsDxe: don't expose kernel command
    line"
  OvmfPkg/GenericQemuLoadImageLib: Read cmdline from QemuKernelLoaderFs
  OvmfPkg/X86QemuLoadImageLib: State fw_cfg dependency in file header

 OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf |   2 +-
 OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf         |   3 +
 OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c   | 145 ++++++++++++++++++--
 OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c           |   3 +
 OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c               |  11 +-
 5 files changed, 147 insertions(+), 17 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] Revert "OvmfPkg/QemuKernelLoaderFsDxe: don't expose kernel command line"

[Dov Murik]

This reverts commit efc52d67e1573ce174d301b52fa1577d552c8441.

Manually fixed conflicts in:
  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c

Note that besides re-exposing the kernel command line as a file in the
synthetic filesystem, we also revert back to AllocatePool instead of
AllocatePages.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
index b09ff6a3590d..c7ddd86f5c75 100644
--- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
+++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
@@ -33,6 +33,7 @@
 typedef enum {
   KernelBlobTypeKernel,
   KernelBlobTypeInitrd,
+  KernelBlobTypeCommandLine,
   KernelBlobTypeMax
 } KERNEL_BLOB_TYPE;
 
@@ -59,6 +60,11 @@ STATIC KERNEL_BLOB mKernelBlob[KernelBlobTypeMax] = {
     {
       { QemuFwCfgItemInitrdSize,      QemuFwCfgItemInitrdData,      },
     }
+  }, {
+    L"cmdline",
+    {
+      { QemuFwCfgItemCommandLineSize, QemuFwCfgItemCommandLineData, },
+    }
   }
 };
 
@@ -948,7 +954,7 @@ FetchBlob (
   //
   // Read blob.
   //
-  Blob->Data = AllocatePages (EFI_SIZE_TO_PAGES ((UINTN)Blob->Size));
+  Blob->Data = AllocatePool (Blob->Size);
   if (Blob->Data == NULL) {
     DEBUG ((DEBUG_ERROR, "%a: failed to allocate %Ld bytes for \"%s\"\n",
       __FUNCTION__, (INT64)Blob->Size, Blob->Name));
@@ -1083,8 +1089,7 @@ FreeBlobs:
   while (BlobType > 0) {
     CurrentBlob = &mKernelBlob[--BlobType];
     if (CurrentBlob->Data != NULL) {
-      FreePages (CurrentBlob->Data,
-        EFI_SIZE_TO_PAGES ((UINTN)CurrentBlob->Size));
+      FreePool (CurrentBlob->Data);
       CurrentBlob->Size = 0;
       CurrentBlob->Data = NULL;
     }
-- 
2.25.1

Re: [edk2-devel] [PATCH v2 1/3] Revert "OvmfPkg/QemuKernelLoaderFsDxe: don't expose kernel command line"

[Laszlo Ersek]

On 06/17/21 14:16, Dov Murik wrote:
> This reverts commit efc52d67e1573ce174d301b52fa1577d552c8441.
> 
> Manually fixed conflicts in:
>   OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> 
> Note that besides re-exposing the kernel command line as a file in the
> synthetic filesystem, we also revert back to AllocatePool instead of
> AllocatePages.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

(1) The bugzilla ticket should be referenced in the commit message,
above your signoff:

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457

With that update:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

> 
> diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> index b09ff6a3590d..c7ddd86f5c75 100644
> --- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> +++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> @@ -33,6 +33,7 @@
>  typedef enum {
>    KernelBlobTypeKernel,
>    KernelBlobTypeInitrd,
> +  KernelBlobTypeCommandLine,
>    KernelBlobTypeMax
>  } KERNEL_BLOB_TYPE;
>  
> @@ -59,6 +60,11 @@ STATIC KERNEL_BLOB mKernelBlob[KernelBlobTypeMax] = {
>      {
>        { QemuFwCfgItemInitrdSize,      QemuFwCfgItemInitrdData,      },
>      }
> +  }, {
> +    L"cmdline",
> +    {
> +      { QemuFwCfgItemCommandLineSize, QemuFwCfgItemCommandLineData, },
> +    }
>    }
>  };
>  
> @@ -948,7 +954,7 @@ FetchBlob (
>    //
>    // Read blob.
>    //
> -  Blob->Data = AllocatePages (EFI_SIZE_TO_PAGES ((UINTN)Blob->Size));
> +  Blob->Data = AllocatePool (Blob->Size);
>    if (Blob->Data == NULL) {
>      DEBUG ((DEBUG_ERROR, "%a: failed to allocate %Ld bytes for \"%s\"\n",
>        __FUNCTION__, (INT64)Blob->Size, Blob->Name));
> @@ -1083,8 +1089,7 @@ FreeBlobs:
>    while (BlobType > 0) {
>      CurrentBlob = &mKernelBlob[--BlobType];
>      if (CurrentBlob->Data != NULL) {
> -      FreePages (CurrentBlob->Data,
> -        EFI_SIZE_TO_PAGES ((UINTN)CurrentBlob->Size));
> +      FreePool (CurrentBlob->Data);
>        CurrentBlob->Size = 0;
>        CurrentBlob->Data = NULL;
>      }
> 

Re: [edk2-devel] [PATCH v2 1/3] Revert "OvmfPkg/QemuKernelLoaderFsDxe: don't expose kernel command line"

[Dov Murik]

On 24/06/2021 16:46, Laszlo Ersek wrote:
> On 06/17/21 14:16, Dov Murik wrote:
>> This reverts commit efc52d67e1573ce174d301b52fa1577d552c8441.
>>
>> Manually fixed conflicts in:
>>   OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
>>
>> Note that besides re-exposing the kernel command line as a file in the
>> synthetic filesystem, we also revert back to AllocatePool instead of
>> AllocatePages.
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> (1) The bugzilla ticket should be referenced in the commit message,
> above your signoff:
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> 

Will do. Maybe worth adding a mention of this requirement in your
contributor guide [1].

[1] https://github.com/tianocore/tianocore.github.io/wiki/Laszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05


> With that update:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!

-Dov

[PATCH v2 2/3] OvmfPkg/GenericQemuLoadImageLib: Read cmdline from QemuKernelLoaderFs

[Dov Murik]

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@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf |   2 +-
 OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c   | 145 ++++++++++++++++++--
 2 files changed, 133 insertions(+), 14 deletions(-)

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
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>
@@ -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 = {
+  {
+    {
+      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;
+  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.
+  //
+  DevicePathNode = (EFI_DEVICE_PATH_PROTOCOL *)&mQemuKernelLoaderFileSystemDevicePath;
+  Status = gBS->LocateDevicePath (
+                  &gEfiSimpleFileSystemProtocolGuid,
+                  &DevicePathNode,
+                  &FsVolumeHandle
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = gBS->HandleProtocol (
+                  FsVolumeHandle,
+                  &gEfiSimpleFileSystemProtocolGuid,
+                  (VOID **)&FsProtocol
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = FsProtocol->OpenVolume (FsVolumeHandle, &Root);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  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;
     }
 
-    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;
 }
-- 
2.25.1

Re: [edk2-devel] [PATCH v2 2/3] OvmfPkg/GenericQemuLoadImageLib: Read cmdline from QemuKernelLoaderFs

[Laszlo Ersek]

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@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@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@redhat.com>
   Fixes: ddd2be6b0026abcd0f819b3915fc80c3de81dd62

#2 OvmfPkg/X86QemuLoadImageLib: plug cmdline blob leak on success
   ...
   Reported-by: Laszlo Ersek <lersek@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

Re: [edk2-devel] [PATCH v2 2/3] OvmfPkg/GenericQemuLoadImageLib: Read cmdline from QemuKernelLoaderFs

[Dov Murik]

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 <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik@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@redhat.com>
>    Fixes: ddd2be6b0026abcd0f819b3915fc80c3de81dd62
> 
> #2 OvmfPkg/X86QemuLoadImageLib: plug cmdline blob leak on success
>    ...
>    Reported-by: Laszlo Ersek <lersek@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

[PATCH v2 3/3] OvmfPkg/X86QemuLoadImageLib: State fw_cfg dependency in file header

[Dov Murik]

Make it clear that X86QemuLoadImageLib relies on fw_cfg; prepare the
ground to add a warning about the incompatibility with boot verification
process.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf | 3 +++
 OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c   | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
index e1615badd2ba..c7ec041cb706 100644
--- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
+++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
@@ -2,6 +2,9 @@
 #  X86 specific implementation of QemuLoadImageLib library class interface
 #  with support for loading mixed mode images and non-EFI stub images
 #
+#  Note that this implementation reads the cmdline (and possibly kernel, setup
+#  data, and initrd in the legacy boot mode) from fw_cfg directly.
+#
 #  Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
index 1177582ab051..dc9018f4333b 100644
--- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
+++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
@@ -2,6 +2,9 @@
   X86 specific implementation of QemuLoadImageLib library class interface
   with support for loading mixed mode images and non-EFI stub images
 
+  Note that this implementation reads the cmdline (and possibly kernel, setup
+  data, and initrd in the legacy boot mode) from fw_cfg directly.
+
   Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
 
-- 
2.25.1

Re: [edk2-devel] [PATCH v2 3/3] OvmfPkg/X86QemuLoadImageLib: State fw_cfg dependency in file header

[Laszlo Ersek]

On 06/17/21 14:17, Dov Murik wrote:
> Make it clear that X86QemuLoadImageLib relies on fw_cfg; prepare the
> ground to add a warning about the incompatibility with boot verification
> process.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf | 3 +++
>  OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c   | 3 +++
>  2 files changed, 6 insertions(+)

(1) The bugzilla ticket should be referenced in the commit message,
above your signoff:

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457

With that update:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

> 
> diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
> index e1615badd2ba..c7ec041cb706 100644
> --- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
> +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
> @@ -2,6 +2,9 @@
>  #  X86 specific implementation of QemuLoadImageLib library class interface
>  #  with support for loading mixed mode images and non-EFI stub images
>  #
> +#  Note that this implementation reads the cmdline (and possibly kernel, setup
> +#  data, and initrd in the legacy boot mode) from fw_cfg directly.
> +#
>  #  Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> index 1177582ab051..dc9018f4333b 100644
> --- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c
> @@ -2,6 +2,9 @@
>    X86 specific implementation of QemuLoadImageLib library class interface
>    with support for loading mixed mode images and non-EFI stub images
>  
> +  Note that this implementation reads the cmdline (and possibly kernel, setup
> +  data, and initrd in the legacy boot mode) from fw_cfg directly.
> +
>    Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
>    Copyright (c) 2020, ARM Ltd. All rights reserved.<BR>
>  
> 

Re: [edk2-devel] [PATCH v2 3/3] OvmfPkg/X86QemuLoadImageLib: State fw_cfg dependency in file header

[Dov Murik]

On 24/06/2021 20:53, Laszlo Ersek wrote:
> On 06/17/21 14:17, Dov Murik wrote:
>> Make it clear that X86QemuLoadImageLib relies on fw_cfg; prepare the
>> ground to add a warning about the incompatibility with boot verification
>> process.
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf | 3 +++
>>  OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c   | 3 +++
>>  2 files changed, 6 insertions(+)
> 
> (1) The bugzilla ticket should be referenced in the commit message,
> above your signoff:
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> 

I'll add it.

> With that update:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!

-Dov