[PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Dov Murik]

Booting with SEV prevented the loading of kernel, initrd, and kernel
command-line via QEMU fw_cfg interface because they arrive from the VMM
which is untrusted in SEV.

However, in some cases the kernel, initrd, and cmdline are not secret
but should not be modified by the host.  In such a case, we want to
verify inside the trusted VM that the kernel, initrd, and cmdline are
indeed the ones expected by the Guest Owner, and only if that is the
case go on and boot them up (removing the need for grub inside OVMF in
that mode).

This patch series declares a new page in MEMFD which will contain the
hashes of these three blobs (kernel, initrd, cmdline), each under its
own GUID entry.  This tables of hashes is populated by QEMU before
launch, and encrypted as part of the initial VM memory; this makes sure
theses hashes are part of the SEV measurement (which has to be approved
by the Guest Owner for secret injection, for example).  Note that this
requires a new QEMU patch which will be submitted soon.

OVMF parses the table of hashes populated by QEMU (patch 5), and as it
reads the fw_cfg blobs from QEMU, it will verify each one against the
expected hash (kernel and initrd verifiers are introduced in patch 6,
and command-line verifier is introduced in patches 7+8).  This is all
done inside the trusted VM context.  If all the hashes are correct, boot
of the kernel is allowed to continue.

Any attempt by QEMU to modify the kernel, initrd, cmdline (including
dropping one of them), or to modify the OVMF code that verifies those
hashes, will cause the initial SEV measurement to change and therefore
will be detectable by the Guest Owner during launch before secret
injection.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>

James Bottomley (8):
  OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
  OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
  OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
  OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
  OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
    device
  OvmfPkg/AmdSev: Add firmware file plugin to verifier
  OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
  OvmfPkg/AmdSev: add SevQemuLoadImageLib

 OvmfPkg/OvmfPkg.dec                                                       |  10 ++
 OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
 OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
 OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
 OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
 OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
 OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
 OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
 21 files changed, 587 insertions(+), 3 deletions(-)
 create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
 create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
 create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
 create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
 create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
 create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
 create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
 create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c

-- 
2.25.1

[PATCH v1 1/8] OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming

[Dov Murik]

From: James Bottomley <jejb@linux.ibm.com>

Commit 96201ae7bf97 ("OvmfPkg/AmdSev/SecretDxe: make secret location
naming generic", 2020-12-15) replaced references to SEV with the generic
term Confidential Computing, but missed the file header comment.  Fix
the naming in that header.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
---
 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
index 308022b5b25e..934ad207632b 100644
--- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
+++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
@@ -1,5 +1,5 @@
 /** @file
-  SEV Secret configuration table constructor
+  Confidential Computing Secret configuration table constructor
 
   Copyright (C) 2020 James Bottomley, IBM Corporation.
   SPDX-License-Identifier: BSD-2-Clause-Patent
-- 
2.25.1

[PATCH v1 2/8] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg

[Dov Murik]

From: James Bottomley <jejb@linux.ibm.com>

Support QEMU's -kernel option.

OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c is an exact copy
of OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c .

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc                                              |  1 +
 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |  2 +
 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  | 11 +++++
 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |  5 ++
 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   | 50 ++++++++++++++++++++
 5 files changed, 69 insertions(+)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 66bbbc80cd18..f820e81fad27 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -283,6 +283,7 @@ [LibraryClasses.common.PEIM]
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
+  QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
   QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf
 
diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf
index 9a806d17ec45..5f6f73d18470 100644
--- a/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf
+++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf
@@ -23,6 +23,7 @@ [Defines]
 
 [Sources]
   BdsPlatform.c
+  QemuKernel.c
   PlatformData.c
   BdsPlatform.h
 
@@ -46,6 +47,7 @@ [LibraryClasses]
   BootLogoLib
   DevicePathLib
   PciLib
+  QemuLoadImageLib
   UefiLib
   PlatformBmPrintScLib
   Tcg2PhysicalPresenceLib
diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
index 748c63081920..f1d3a2906c00 100644
--- a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
+++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h
@@ -172,4 +172,15 @@ PlatformInitializeConsole (
   IN PLATFORM_CONSOLE_CONNECT_ENTRY   *PlatformConsole
   );
 
+/**
+  Loads and boots UEFI Linux via the FwCfg interface.
+
+  @retval    EFI_NOT_FOUND - The Linux kernel was not found
+
+**/
+EFI_STATUS
+TryRunningQemuKernel (
+  VOID
+  );
+
 #endif // _PLATFORM_SPECIFIC_BDS_PLATFORM_H_
diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
index 5c92d4fc2b09..7cceeea4879c 100644
--- a/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c
@@ -1315,6 +1315,11 @@ PlatformBootManagerAfterConsole (
   //
   Tcg2PhysicalPresenceLibProcessRequest (NULL);
 
+  //
+  // Process QEMU's -kernel command line option
+  //
+  TryRunningQemuKernel ();
+
   //
   // Perform some platform specific connect sequence
   //
diff --git a/OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c b/OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
new file mode 100644
index 000000000000..c6255921779e
--- /dev/null
+++ b/OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
@@ -0,0 +1,50 @@
+/** @file
+
+  Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi.h>
+
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/QemuLoadImageLib.h>
+#include <Library/ReportStatusCodeLib.h>
+#include <Library/UefiLib.h>
+
+
+EFI_STATUS
+TryRunningQemuKernel (
+  VOID
+  )
+{
+  EFI_STATUS                Status;
+  EFI_HANDLE                KernelImageHandle;
+
+  Status = QemuLoadKernelImage (&KernelImageHandle);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  //
+  // Signal the EVT_SIGNAL_READY_TO_BOOT event
+  //
+  EfiSignalEventReadyToBoot();
+
+  REPORT_STATUS_CODE (EFI_PROGRESS_CODE,
+    (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_PC_READY_TO_BOOT_EVENT));
+
+  //
+  // Start the image.
+  //
+  Status = QemuStartKernelImage (&KernelImageHandle);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "%a: QemuStartKernelImage(): %r\n", __FUNCTION__,
+      Status));
+  }
+
+  QemuUnloadKernelImage (KernelImageHandle);
+
+  return Status;
+}
-- 
2.25.1

[PATCH v1 3/8] OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes

[Dov Murik]

From: James Bottomley <jejb@linux.ibm.com>

Reserve a page inside the MEMFD for the VMM to place an encrypted page
containing GUID described hashes of firmware configurations.  We do
this so the page gets attested by the PSP and thus the untrusted VMM
can't pass in different files from what the guest owner thinks.

Declare this in the Reset Vector table using GUID
7255371f-3a3b-4b04-927b-1da6efa8d454 and a uint32_t table of a base
and size value, identically to the secret block.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
---
 OvmfPkg/OvmfPkg.dec                          |  6 ++++++
 OvmfPkg/AmdSev/AmdSevX64.fdf                 |  3 +++
 OvmfPkg/ResetVector/ResetVector.inf          |  2 ++
 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 20 ++++++++++++++++++++
 OvmfPkg/ResetVector/ResetVector.nasmb        |  2 ++
 5 files changed, 33 insertions(+)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 6ae733f6e39f..7cd29a60a436 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -321,6 +321,12 @@ [PcdsFixedAtBuild]
   gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|0x0|UINT32|0x42
   gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize|0x0|UINT32|0x43
 
+  ## The base address and size of a hash table confirming allowed
+  #  parameters to be passed in via the Qemu firmware configuration
+  #  device
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|0x0|UINT32|0x47
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize|0x0|UINT32|0x48
+
 [PcdsDynamic, PcdsDynamicEx]
   gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
index dd0030dbf189..6e1bb7723bd1 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -65,6 +65,9 @@ [FD.MEMFD]
 0x00D000|0x001000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
 
+0x00E000|0x001000
+gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
+
 0x010000|0x010000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
 
diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
index dc38f68919cd..d028c92d8cfa 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -47,3 +47,5 @@ [Pcd]
 [FixedPcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
   gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 9c0b5853a46f..7ec3c6e980c3 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -47,7 +47,27 @@ TIMES (15 - ((guidedStructureEnd - guidedStructureStart + 15) % 16)) DB 0
 ;
 guidedStructureStart:
 
+; SEV Hash Table Block
 ;
+; This describes the guest ram area where the hypervisor should
+; install a table describing the hashes of certain firmware configuration
+; device files that would otherwise be passed in unchecked.  The current
+; use is for the kernel, initrd and command line values, but others may be
+; added.  The data format is:
+;
+; base physical address (32 bit word)
+; table length (32 bit word)
+;
+; GUID (SEV FW config hash block): 7255371f-3a3b-4b04-927b-1da6efa8d454
+;
+sevFwHashBlockStart:
+    DD      SEV_FW_HASH_BLOCK_BASE
+    DD      SEV_FW_HASH_BLOCK_SIZE
+    DW      sevFwHashBlockEnd - sevFwHashBlockStart
+    DB      0x1f, 0x37, 0x55, 0x72, 0x3b, 0x3a, 0x04, 0x4b
+    DB      0x92, 0x7b, 0x1d, 0xa6, 0xef, 0xa8, 0xd4, 0x54
+sevFwHashBlockEnd:
+
 ; SEV Secret block
 ;
 ; This describes the guest ram area where the hypervisor should
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 5fbacaed5f9d..8d0bab02f8cb 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -88,5 +88,7 @@
   %define SEV_ES_AP_RESET_IP  FixedPcdGet32 (PcdSevEsWorkAreaBase)
   %define SEV_LAUNCH_SECRET_BASE  FixedPcdGet32 (PcdSevLaunchSecretBase)
   %define SEV_LAUNCH_SECRET_SIZE  FixedPcdGet32 (PcdSevLaunchSecretSize)
+  %define SEV_FW_HASH_BLOCK_BASE  FixedPcdGet32 (PcdQemuHashTableBase)
+  %define SEV_FW_HASH_BLOCK_SIZE  FixedPcdGet32 (PcdQemuHashTableSize)
 %include "Ia16/ResetVectorVtf0.asm"
 
-- 
2.25.1

[PATCH v1 4/8] OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items

[Dov Murik]

From: James Bottomley <jejb@linux.ibm.com>

Allow registering a verifier which is then called for each blob passed
via QEMU's fw_cfg.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
---
 OvmfPkg/Include/Library/QemuFwCfgLib.h                | 35 ++++++++++++++++++++
 OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 31 +++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/OvmfPkg/Include/Library/QemuFwCfgLib.h b/OvmfPkg/Include/Library/QemuFwCfgLib.h
index 68002bb654e6..1095efad5878 100644
--- a/OvmfPkg/Include/Library/QemuFwCfgLib.h
+++ b/OvmfPkg/Include/Library/QemuFwCfgLib.h
@@ -173,5 +173,40 @@ QemuFwCfgFindFile (
   OUT  UINTN                 *Size
   );
 
+/**
+  The verifier is used to abstract a hash verification operation when
+  A firmware config item is accessed via a filesystem and has some type
+  of integrity information passed in.
+
+  @param[in]    Name       The name of the config file to verify.
+  @param[in]    Buffer     A pointer to the loaded config information.
+  @param[in]    Size       The size of the buffer.
+
+  @retval EFI_SUCCESS          The buffer verified OK.
+
+  @retval EFI_ACCESS_DENIED    The buffer failed the integrity check.
+
+**/
+typedef
+RETURN_STATUS
+(EFIAPI *FW_CFG_VERIFIER) (
+  IN  CONST CHAR16    *Name,
+  IN  VOID            *Buffer,
+  IN  UINTN           Size
+  );
+
+/**
+  Register a verifier for the Firmware Configuration Filesystem to use
+
+  @param[in]  Verifier     The verifier to register
+
+  @retval EFI_SUCCESS      The verifier was successfully registered
+**/
+RETURN_STATUS
+EFIAPI
+RegisterFwCfgVerifier (
+  IN FW_CFG_VERIFIER    Verifier
+  );
+
 #endif
 
diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
index b09ff6a3590d..9823d23d1005 100644
--- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
+++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
@@ -982,6 +982,27 @@ FetchBlob (
   return EFI_SUCCESS;
 }
 
+STATIC FW_CFG_VERIFIER mVerifier = NULL;
+
+/**
+  Register a verifier for the Firmware Configuration Filesystem to use
+
+  @param[in]  Verifier     The verifier to register
+
+  @retval EFI_SUCCESS      The verifier was successfully registered
+**/
+EFI_STATUS
+EFIAPI
+RegisterFwCfgVerifier (
+  IN FW_CFG_VERIFIER    Verifier
+  )
+{
+  if (mVerifier != NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+  mVerifier = Verifier;
+  return EFI_SUCCESS;
+}
 
 //
 // The entry point of the feature.
@@ -1033,6 +1054,16 @@ QemuKernelLoaderFsDxeEntrypoint (
     if (EFI_ERROR (Status)) {
       goto FreeBlobs;
     }
+    if (mVerifier != NULL) {
+      Status = mVerifier (
+        CurrentBlob->Name,
+        CurrentBlob->Data,
+        CurrentBlob->Size
+      );
+      if (EFI_ERROR (Status)) {
+        goto FreeBlobs;
+      }
+    }
     mTotalBlobBytes += CurrentBlob->Size;
   }
   KernelBlob      = &mKernelBlob[KernelBlobTypeKernel];
-- 
2.25.1

[PATCH v1 5/8] OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg device

[Dov Murik]

From: James Bottomley <jejb@linux.ibm.com>

The library finds and checks out the encrypted page from the memfd and
installs a finder routine for GUID described hashes if it checks out
OK.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/OvmfPkg.dec                                          |   4 +
 OvmfPkg/AmdSev/AmdSevX64.dsc                                 |   1 +
 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf |  34 ++++++
 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h            |  47 ++++++++
 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c   | 126 ++++++++++++++++++++
 5 files changed, 212 insertions(+)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 7cd29a60a436..36f0a2cb4cf9 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -18,8 +18,12 @@ [Defines]
 [Includes]
   Include
   Csm/Include
+  AmdSev/Include
 
 [LibraryClasses]
+  ##  @libraryclass  Functions for extracting Sev Hashes from the MEMFD
+  SevHashFinderLib|AmdSev/Include/Library/SevHashFinderLib.h
+
   ##  @libraryclass  Access bhyve's firmware control interface.
   BhyveFwCtlLib|Include/Library/BhyveFwCtlLib.h
 
diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index f820e81fad27..b4484ca07614 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -118,6 +118,7 @@ [SkuIds]
 !include MdePkg/MdeLibs.dsc.inc
 
 [LibraryClasses]
+  SevHashFinderLib|OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
   PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
   TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
   ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
diff --git a/OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf b/OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
new file mode 100644
index 000000000000..79ebf51baed0
--- /dev/null
+++ b/OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
@@ -0,0 +1,34 @@
+##  @file
+#  Provides the Secure Verification services for AMD SEV firmware config
+#
+#  Copyright (C) 2021 James Bottomley, IBM Corporation.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = SevHashFinderLib
+  FILE_GUID                      = d8ef4e22-991a-4134-b285-1d970cfe2ca6
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = SevHashFinderLib
+  CONSTRUCTOR                    = SevHashFinderLibConstructor
+
+[Sources]
+  SevHashFinderLib.c
+
+[Packages]
+  CryptoPkg/CryptoPkg.dec
+  MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  BaseCryptLib
+  BaseMemoryLib
+  PcdLib
+
+[FixedPcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
diff --git a/OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h b/OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
new file mode 100644
index 000000000000..79d5039a649b
--- /dev/null
+++ b/OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
@@ -0,0 +1,47 @@
+/** @file
+  Validate a hash against that in the Sev Hash table
+
+  Copyright (C) 2021 James Bottomley, IBM Corporation.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef __SEV_HASH_FINDER_LIB_H__
+#define __SEV_HASH_FINDER_LIB_H__
+
+/**
+  The Sev Hash table must be in encrypted memory and has the table
+  and its entries described by
+
+  <GUID>|UINT16 <len>|<data>
+
+  With the whole table GUID being 9438d606-4f22-4cc9-b479-a793d411fd21
+
+  The current possible table entries are for the kernel, the initrd
+  and the cmdline:
+
+  4de79437-abd2-427f-b835-d5b172d2045b  kernel
+  44baf731-3a2f-4bd7-9af1-41e29169781d  initrd
+  97d02dd8-bd20-4c94-aa78-e7714d36ab2a  cmdline
+
+  The size of the entry is used to identify the hash, but the
+  expectation is that it will be 32 bytes of SHA-256.
+**/
+
+#define SEV_HASH_TABLE_GUID \
+  (GUID) { 0x9438d606, 0x4f22, 0x4cc9, { 0xb4, 0x79, 0xa7, 0x93, 0xd4, 0x11, 0xfd, 0x21 } }
+#define SEV_KERNEL_HASH_GUID \
+  (GUID) { 0x4de79437, 0xabd2, 0x427f, { 0xb8, 0x35, 0xd5, 0xb1, 0x72, 0xd2, 0x04, 0x5b } }
+#define SEV_INITRD_HASH_GUID \
+  (GUID) { 0x44baf731, 0x3a2f, 0x4bd7, { 0x9a, 0xf1, 0x41, 0xe2, 0x91, 0x69, 0x78, 0x1d } }
+#define SEV_CMDLINE_HASH_GUID \
+  (GUID) { 0x97d02dd8, 0xbd20, 0x4c94, { 0xaa, 0x78, 0xe7, 0x71, 0x4d, 0x36, 0xab, 0x2a } }
+
+EFI_STATUS
+EFIAPI
+ValidateHashEntry (
+  IN CONST GUID *Guid,
+  IN CONST VOID *Buf,
+  UINT32 BufSize
+);
+
+#endif
diff --git a/OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c b/OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
new file mode 100644
index 000000000000..9cb999ae8cad
--- /dev/null
+++ b/OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
@@ -0,0 +1,126 @@
+/** @file
+  SEV Hash finder library to locate the SEV encrypted hash table
+
+  Copyright (C) 2021 James Bottomley, IBM Corporation.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <PiDxe.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/BaseCryptLib.h>
+#include <Library/DebugLib.h>
+#include <Library/SevHashFinderLib.h>
+
+#pragma pack (1)
+typedef struct {
+  GUID   Guid;
+  UINT16 Len;
+  UINT8  Data[];
+} HASH_TABLE;
+#pragma pack ()
+
+STATIC HASH_TABLE *mHashTable;
+STATIC UINT16 mHashTableSize;
+
+EFI_STATUS
+EFIAPI
+ValidateHashEntry (
+  IN CONST GUID *Guid,
+  IN CONST VOID *Buf,
+  UINT32 BufSize
+  )
+{
+  INT32 Len;
+  HASH_TABLE *Entry;
+  UINT8 Hash[SHA256_DIGEST_SIZE];
+
+  if (mHashTable == NULL || mHashTableSize == 0) {
+    DEBUG ((DEBUG_ERROR,
+      "%a: Verifier Called but no hash table discoverd in MEMFD\n",
+      __FUNCTION__));
+    return EFI_ACCESS_DENIED;
+  }
+
+  Sha256HashAll (Buf, BufSize, Hash);
+
+  for (Entry = mHashTable, Len = 0;
+       Len < (INT32)mHashTableSize;
+       Len += Entry->Len,
+       Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {
+    UINTN EntrySize;
+    EFI_STATUS Status;
+
+    if (!CompareGuid (&Entry->Guid, Guid)) {
+      continue;
+    }
+
+    DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__, Guid));
+
+    //
+    // Verify that the buffer's hash is identical to the hash table entry
+    //
+    EntrySize = Entry->Len - sizeof (Entry->Guid) - sizeof (Entry->Len);
+    if (EntrySize != SHA256_DIGEST_SIZE) {
+      DEBUG ((DEBUG_ERROR, "%a: Hash has the wrong size %d != %d\n",
+        __FUNCTION__, EntrySize, SHA256_DIGEST_SIZE));
+      return EFI_ACCESS_DENIED;
+    }
+    if (CompareMem (Entry->Data, Hash, EntrySize) == 0) {
+      Status = EFI_SUCCESS;
+      DEBUG ((DEBUG_INFO, "%a: Hash Comparison succeeded\n", __FUNCTION__));
+    } else {
+      Status = EFI_ACCESS_DENIED;
+      DEBUG ((DEBUG_ERROR, "%a: Hash Comparison Failed\n", __FUNCTION__));
+    }
+    return Status;
+  }
+  DEBUG ((DEBUG_ERROR, "%a: Hash GUID %g not found in table\n", __FUNCTION__,
+    Guid));
+  return EFI_ACCESS_DENIED;
+}
+
+/**
+  Register security measurement handler.
+
+  This function always returns success, even if the table
+  can't be found.  It only returns errors if an actual use
+  is made of the non-existent table because that indicates it
+  should have been present.
+
+  @param  ImageHandle   ImageHandle of the loaded driver.
+  @param  SystemTable   Pointer to the EFI System Table.
+
+  @retval EFI_SUCCESS   The verifier tables were set up correctly
+**/
+EFI_STATUS
+EFIAPI
+SevHashFinderLibConstructor (
+  IN EFI_HANDLE        ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  HASH_TABLE *Ptr = (void *)FixedPcdGet64 (PcdQemuHashTableBase);
+  UINT32 Size = FixedPcdGet32 (PcdQemuHashTableSize);
+
+  mHashTable = NULL;
+  mHashTableSize = 0;
+
+  if (Ptr == NULL || Size == 0) {
+    return EFI_SUCCESS;
+  }
+
+  if (!CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID)) {
+    return EFI_SUCCESS;
+  }
+
+  DEBUG ((DEBUG_INFO, "%a: found Injected Hash in secure location\n",
+    __FUNCTION__));
+
+  mHashTable = (HASH_TABLE *)Ptr->Data;
+  mHashTableSize = Ptr->Len - sizeof (Ptr->Guid) - sizeof (Ptr->Len);
+
+  DEBUG ((DEBUG_INFO, "%a: Ptr=%p, Size=%d\n", __FUNCTION__, mHashTable,
+    mHashTableSize));
+
+  return EFI_SUCCESS;
+}
-- 
2.25.1

[PATCH v1 6/8] OvmfPkg/AmdSev: Add firmware file plugin to verifier

[Dov Murik]

From: James Bottomley <jejb@linux.ibm.com>

Provide a library verifier that plugs into the QemuKernelLoaderFs
hooks to verify the hashes against the SEV hash table (stored in
encrypted memory).

The verifier is enabled when SEV memory encryption is active.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc                                 |  5 +-
 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf | 30 ++++++++++
 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c   | 60 ++++++++++++++++++++
 3 files changed, 94 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index b4484ca07614..bfb16798b3b7 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -697,7 +697,10 @@ [Components]
       NULL|MdeModulePkg/Library/BootManagerUiLib/BootManagerUiLib.inf
       NULL|MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceManagerUiLib.inf
   }
-  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf
+  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf {
+    <LibraryClasses>
+      NULL|OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
+  }
   OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
   OvmfPkg/Virtio10Dxe/Virtio10.inf
   OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
diff --git a/OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf b/OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
new file mode 100644
index 000000000000..86d099455d55
--- /dev/null
+++ b/OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
@@ -0,0 +1,30 @@
+##  @file
+#  Provides the Secure Verification services for AMD SEV firmware config
+#
+#  Copyright (C) 2021 James Bottomley, IBM Corporation.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = SevFwCfgVerifier
+  FILE_GUID                      = 33457c78-aae2-4511-9188-ac1fe88d03de
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = NULL|DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER
+  CONSTRUCTOR                    = SevFwCfgVerifierConstructor
+
+[Sources]
+  SevFwCfgVerifier.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  BaseLib
+  DebugLib
+  MemEncryptSevLib
+  SevHashFinderLib
diff --git a/OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c b/OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
new file mode 100644
index 000000000000..53b617a72aa9
--- /dev/null
+++ b/OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
@@ -0,0 +1,60 @@
+/** @file
+  AMD SEV Firmware Config file verifier
+
+  Copyright (C) 2021 James Bottomley, IBM Corporation.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemEncryptSevLib.h>
+#include <Library/QemuFwCfgLib.h>
+#include <Library/SevHashFinderLib.h>
+
+STATIC EFI_STATUS
+EFIAPI
+SevFwCfgVerifier (
+  IN  CONST CHAR16    *Name,
+  IN  VOID            *Buffer,
+  IN  UINTN           Size
+  )
+{
+  DEBUG ((DEBUG_INFO, "%a: Validating Hash of %s\n", __FUNCTION__, Name));
+
+  if (StrCmp (Name, L"kernel") == 0) {
+    return ValidateHashEntry (&SEV_KERNEL_HASH_GUID, Buffer, Size);
+  }
+  if (StrCmp (Name, L"initrd") == 0) {
+    return ValidateHashEntry (&SEV_INITRD_HASH_GUID, Buffer, Size);
+  }
+
+  DEBUG ((DEBUG_ERROR, "%a: Failed to find Filename %s", __FUNCTION__, Name));
+  return EFI_SECURITY_VIOLATION;
+}
+
+/**
+  Register security measurement handler.
+
+  @param  ImageHandle   ImageHandle of the loaded driver.
+  @param  SystemTable   Pointer to the EFI System Table.
+
+  @retval EFI_SUCCESS   The handlers were registered successfully.
+**/
+EFI_STATUS
+EFIAPI
+SevFwCfgVerifierConstructor (
+  IN EFI_HANDLE        ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  if (MemEncryptSevIsEnabled ()) {
+    DEBUG ((DEBUG_INFO, "Enabling hash verification of fw_cfg files"));
+    return RegisterFwCfgVerifier (SevFwCfgVerifier);
+  } else {
+    //
+    // Don't install verifier if SEV isn't enabled
+    //
+    DEBUG ((DEBUG_INFO, "NOT Enabling hash verification of fw_cfg files"));
+    return EFI_SUCCESS;
+  }
+}
-- 
2.25.1

[PATCH v1 7/8] OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line

[Dov Murik]

From: James Bottomley <jejb@linux.ibm.com>

Add optional hook which calls a verifier with the content of the fw_cfg
command line.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
---
 OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 29 ++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
index 114db7e8441f..d3067dae1425 100644
--- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
+++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c
@@ -51,6 +51,28 @@ STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = {
   }
 };
 
+STATIC FW_CFG_VERIFIER mVerifier = NULL;
+
+/**
+  Register a verifier for the Firmware Configuration Filesystem to use
+
+  @param[in]  Verifier     The verifier to register
+
+  @retval EFI_SUCCESS      The verifier was successfully registered
+**/
+EFI_STATUS
+EFIAPI
+RegisterFwCfgVerifier (
+  IN FW_CFG_VERIFIER    Verifier
+  )
+{
+  if (mVerifier != NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+  mVerifier = Verifier;
+  return EFI_SUCCESS;
+}
+
 /**
   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
@@ -149,6 +171,13 @@ QemuLoadKernelImage (
       goto FreeCommandLine;
     }
 
+    if (mVerifier != NULL) {
+      Status = mVerifier (NULL, CommandLine, CommandLineSize);
+      if (EFI_ERROR (Status)) {
+        goto FreeCommandLine;
+      }
+    }
+
     //
     // Drop the terminating NUL, convert to UTF-16.
     //
-- 
2.25.1

[PATCH v1 8/8] OvmfPkg/AmdSev: add SevQemuLoadImageLib

[Dov Murik]

From: James Bottomley <jejb@linux.ibm.com>

This is a wrapper library around GenericQemuLoadImageLib which
activates a verifier on the command line to check its hash against the
one passed in via the SEV hash table.

The verifier is enabled if SEV memory encryption is active.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc                                       |  4 +-
 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf | 30 +++++++++++
 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c   | 52 ++++++++++++++++++++
 3 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index bfb16798b3b7..9db3d993dda0 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -284,7 +284,6 @@ [LibraryClasses.common.PEIM]
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
-  QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
   QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf
 
@@ -372,7 +371,8 @@ [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
+  GenericQemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
+  QemuLoadImageLib|OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
 !if $(TPM_ENABLE) == TRUE
   Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
   Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
diff --git a/OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf b/OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
new file mode 100644
index 000000000000..b87031f4d15a
--- /dev/null
+++ b/OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
@@ -0,0 +1,30 @@
+##  @file
+#  Provides the Secure Verification services for AMD SEV firmware config
+#
+#  Copyright (C) 2021 James Bottomley, IBM Corporation.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 1.27
+  BASE_NAME                      = SevQemuLoadImageLib
+  FILE_GUID                      = 7ceec418-e1e3-42fb-91c5-56abc6d3744d
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = QemuLoadImageLib|DXE_DRIVER
+  CONSTRUCTOR                    = SevQemuLoadImageConstructor
+
+[Sources]
+  SevQemuLoadImageLib.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  DebugLib
+  GenericQemuLoadImageLib
+  MemEncryptSevLib
+  SevHashFinderLib
diff --git a/OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c b/OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
new file mode 100644
index 000000000000..a44d296cea78
--- /dev/null
+++ b/OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
@@ -0,0 +1,52 @@
+/** @file
+  AMD SEV Firmware Config file verifier
+
+  Copyright (C) 2021 James Bottomley, IBM Corporation.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Uefi.h>
+
+#include <Library/DebugLib.h>
+#include <Library/MemEncryptSevLib.h>
+#include <Library/QemuFwCfgLib.h>
+#include <Library/SevHashFinderLib.h>
+
+STATIC EFI_STATUS
+EFIAPI
+SevCmdLineVerifier (
+  IN  CONST CHAR16    *Name,
+  IN  VOID            *Buffer,
+  IN  UINTN           Size
+  )
+{
+  DEBUG ((DEBUG_INFO, "%a: Validating Hash\n", __FUNCTION__));
+
+  return ValidateHashEntry (&SEV_CMDLINE_HASH_GUID, Buffer, Size);
+}
+
+/**
+  Register security measurement handler.
+
+  @param  ImageHandle   ImageHandle of the loaded driver.
+  @param  SystemTable   Pointer to the EFI System Table.
+
+  @retval EFI_SUCCESS   The handlers were registered successfully.
+**/
+EFI_STATUS
+EFIAPI
+SevQemuLoadImageConstructor (
+  VOID
+  )
+{
+  if (MemEncryptSevIsEnabled ()) {
+    DEBUG ((DEBUG_INFO, "Enabling hash verification of fw_cfg cmdline\n"));
+    return RegisterFwCfgVerifier (SevCmdLineVerifier);
+  } else {
+    //
+    // Don't install verifier if SEV isn't enabled
+    //
+    DEBUG ((DEBUG_INFO, "NOT Enabling hash verification of fw_cfg cmdline\n"));
+    return EFI_SUCCESS;
+  }
+}
-- 
2.25.1

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Dov Murik]

On 25/05/2021 8:31, Dov Murik wrote:
> Booting with SEV prevented the loading of kernel, initrd, and kernel
> command-line via QEMU fw_cfg interface because they arrive from the VMM
> which is untrusted in SEV.
> 
> However, in some cases the kernel, initrd, and cmdline are not secret
> but should not be modified by the host.  In such a case, we want to
> verify inside the trusted VM that the kernel, initrd, and cmdline are
> indeed the ones expected by the Guest Owner, and only if that is the
> case go on and boot them up (removing the need for grub inside OVMF in
> that mode).
> 
> This patch series declares a new page in MEMFD which will contain the
> hashes of these three blobs (kernel, initrd, cmdline), each under its
> own GUID entry.  This tables of hashes is populated by QEMU before
> launch, and encrypted as part of the initial VM memory; this makes sure
> theses hashes are part of the SEV measurement (which has to be approved
> by the Guest Owner for secret injection, for example).  Note that this
> requires a new QEMU patch which will be submitted soon.


The QEMU patch is submitted at:

https://lore.kernel.org/qemu-devel/20210525065931.1628554-1-dovmurik@linux.ibm.com/

-Dov

Re: [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Brijesh Singh]

On 5/25/21 12:31 AM, Dov Murik wrote:
> Booting with SEV prevented the loading of kernel, initrd, and kernel
> command-line via QEMU fw_cfg interface because they arrive from the VMM
> which is untrusted in SEV.
>
> However, in some cases the kernel, initrd, and cmdline are not secret
> but should not be modified by the host.  In such a case, we want to
> verify inside the trusted VM that the kernel, initrd, and cmdline are
> indeed the ones expected by the Guest Owner, and only if that is the
> case go on and boot them up (removing the need for grub inside OVMF in
> that mode).
>
> This patch series declares a new page in MEMFD which will contain the
> hashes of these three blobs (kernel, initrd, cmdline), each under its
> own GUID entry.  This tables of hashes is populated by QEMU before
> launch, and encrypted as part of the initial VM memory; this makes sure
> theses hashes are part of the SEV measurement (which has to be approved
> by the Guest Owner for secret injection, for example).  Note that this
> requires a new QEMU patch which will be submitted soon.

I have not looked at the patches, but trying to brainstorm if we can
avoid reserving a new page in the MEMFD and use the existing EDK2
infrastructure to verify the blobs (kernel, initrd) loaded through the
FW_CFG interface in the guest memory.

If I understand correctly, then in your proposed approach, guest owner
wants to ensure that the hypevisor passing its preferred kernel, initrd
and cmdline. The guest owner basically knows the hashes of these
components in advance. So, can we do something like this:

- The secret blob provided by the guest owner should contains the hashes
(sha384) of these components.

- Use openssl API available in the edk2 to calculate the hash while
loading the kernel, initrd and cmdline.

- Before booting the kernel, compare the calculated hash with the one
listed in the secret page. If they don't match then fail otherwise continue.

Did I miss something ?

-Brijesh

> OVMF parses the table of hashes populated by QEMU (patch 5), and as it
> reads the fw_cfg blobs from QEMU, it will verify each one against the
> expected hash (kernel and initrd verifiers are introduced in patch 6,
> and command-line verifier is introduced in patches 7+8).  This is all
> done inside the trusted VM context.  If all the hashes are correct, boot
> of the kernel is allowed to continue.
>
> Any attempt by QEMU to modify the kernel, initrd, cmdline (including
> dropping one of them), or to modify the OVMF code that verifies those
> hashes, will cause the initial SEV measurement to change and therefore
> will be detectable by the Guest Owner during launch before secret
> injection.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ashish Kalra <ashish.kalra@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>
> James Bottomley (8):
>   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
>   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
>   OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
>   OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
>   OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
>     device
>   OvmfPkg/AmdSev: Add firmware file plugin to verifier
>   OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
>   OvmfPkg/AmdSev: add SevQemuLoadImageLib
>
>  OvmfPkg/OvmfPkg.dec                                                       |  10 ++
>  OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
>  OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
>  OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
>  OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
>  OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
>  OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
>  OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
>  21 files changed, 587 insertions(+), 3 deletions(-)
>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
>  create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
>

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Dov Murik]

Hi Brijesh,

On 25/05/2021 18:48, Brijesh Singh wrote:
> 
> On 5/25/21 12:31 AM, Dov Murik wrote:
>> Booting with SEV prevented the loading of kernel, initrd, and kernel
>> command-line via QEMU fw_cfg interface because they arrive from the VMM
>> which is untrusted in SEV.
>>
>> However, in some cases the kernel, initrd, and cmdline are not secret
>> but should not be modified by the host.  In such a case, we want to
>> verify inside the trusted VM that the kernel, initrd, and cmdline are
>> indeed the ones expected by the Guest Owner, and only if that is the
>> case go on and boot them up (removing the need for grub inside OVMF in
>> that mode).
>>
>> This patch series declares a new page in MEMFD which will contain the
>> hashes of these three blobs (kernel, initrd, cmdline), each under its
>> own GUID entry.  This tables of hashes is populated by QEMU before
>> launch, and encrypted as part of the initial VM memory; this makes sure
>> theses hashes are part of the SEV measurement (which has to be approved
>> by the Guest Owner for secret injection, for example).  Note that this
>> requires a new QEMU patch which will be submitted soon.
> 
> I have not looked at the patches, but trying to brainstorm if we can
> avoid reserving a new page in the MEMFD and use the existing EDK2
> infrastructure to verify the blobs (kernel, initrd) loaded through the
> FW_CFG interface in the guest memory.
> 
> If I understand correctly, then in your proposed approach, guest owner
> wants to ensure that the hypevisor passing its preferred kernel, initrd
> and cmdline. The guest owner basically knows the hashes of these
> components in advance.

Yes, that's correct.

> So, can we do something like this:
> 
> - The secret blob provided by the guest owner should contains the hashes
> (sha384) of these components.
> 
> - Use openssl API available in the edk2 to calculate the hash while
> loading the kernel, initrd and cmdline.

Indeed we do something similar already - we use Sha256HashAll (see patch
5 in this series).


> 
> - Before booting the kernel, compare the calculated hash with the one
> listed in the secret page. If they don't match then fail otherwise continue.

That is indeed what we do in patch 6 (the calls to our ValidateHashEntry).


> 
> Did I miss something ?

Thanks for proposing this.

Your approach has the advantage that there's no need for extra
pre-allocated MEMFD page for the hashes, and also it makes the QEMU flow
simpler (QEMU doesn't need to compute the hashes and put them in that
special MEMFD page).  I think that the only change we'll need from QEMU
in the x86_load_linux flow (which is when the user supplies
-kernel/-initrd) is that it won't modify any memory in a way that the
modifies the hashes that Guest Owner expects (for example, avoid writing
over the kernel's setup area).

However, the disadvantage is that it unifies boot measurement with the
secret injection.  The Guest Owner _must_ inject the hashes, otherwise
the system doesn't boot; whereas in our current suggestion the Guest
Owner can check the measurement, verify that everything is OK, and just
let the guest continue.

But as I write this, I think that maybe without secret injection the
guest is not really secure? Because the host could just continue
execution of the guest without waiting for measurement check... If the
Guest Owner _must_ inject a secret for SEV to be secure in any case, we
might as well choose your path and let the Guest Owner inject the table
of hashes themselves.

I'd like to hear your (and others') thoughts.

-Dov

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Lendacky, Thomas]

On 5/25/21 3:08 PM, Dov Murik wrote:
> Hi Brijesh,
> 
> On 25/05/2021 18:48, Brijesh Singh wrote:
>>
>> On 5/25/21 12:31 AM, Dov Murik wrote:
>>> Booting with SEV prevented the loading of kernel, initrd, and kernel
>>> command-line via QEMU fw_cfg interface because they arrive from the VMM
>>> which is untrusted in SEV.
>>>
>>> However, in some cases the kernel, initrd, and cmdline are not secret
>>> but should not be modified by the host.  In such a case, we want to
>>> verify inside the trusted VM that the kernel, initrd, and cmdline are
>>> indeed the ones expected by the Guest Owner, and only if that is the
>>> case go on and boot them up (removing the need for grub inside OVMF in
>>> that mode).
>>>
>>> This patch series declares a new page in MEMFD which will contain the
>>> hashes of these three blobs (kernel, initrd, cmdline), each under its
>>> own GUID entry.  This tables of hashes is populated by QEMU before
>>> launch, and encrypted as part of the initial VM memory; this makes sure
>>> theses hashes are part of the SEV measurement (which has to be approved
>>> by the Guest Owner for secret injection, for example).  Note that this
>>> requires a new QEMU patch which will be submitted soon.
>>
>> I have not looked at the patches, but trying to brainstorm if we can
>> avoid reserving a new page in the MEMFD and use the existing EDK2
>> infrastructure to verify the blobs (kernel, initrd) loaded through the
>> FW_CFG interface in the guest memory.
>>
>> If I understand correctly, then in your proposed approach, guest owner
>> wants to ensure that the hypevisor passing its preferred kernel, initrd
>> and cmdline. The guest owner basically knows the hashes of these
>> components in advance.
> 
> Yes, that's correct.
> 
>> So, can we do something like this:
>>
>> - The secret blob provided by the guest owner should contains the hashes
>> (sha384) of these components.
>>
>> - Use openssl API available in the edk2 to calculate the hash while
>> loading the kernel, initrd and cmdline.
> 
> Indeed we do something similar already - we use Sha256HashAll (see patch
> 5 in this series).
> 
> 
>>
>> - Before booting the kernel, compare the calculated hash with the one
>> listed in the secret page. If they don't match then fail otherwise continue.
> 
> That is indeed what we do in patch 6 (the calls to our ValidateHashEntry).
> 
> 
>>
>> Did I miss something ?
> 
> Thanks for proposing this.
> 
> Your approach has the advantage that there's no need for extra
> pre-allocated MEMFD page for the hashes, and also it makes the QEMU flow
> simpler (QEMU doesn't need to compute the hashes and put them in that
> special MEMFD page).  I think that the only change we'll need from QEMU
> in the x86_load_linux flow (which is when the user supplies
> -kernel/-initrd) is that it won't modify any memory in a way that the
> modifies the hashes that Guest Owner expects (for example, avoid writing
> over the kernel's setup area).
> 
> However, the disadvantage is that it unifies boot measurement with the
> secret injection.  The Guest Owner _must_ inject the hashes, otherwise
> the system doesn't boot; whereas in our current suggestion the Guest
> Owner can check the measurement, verify that everything is OK, and just
> let the guest continue.
> 
> But as I write this, I think that maybe without secret injection the
> guest is not really secure? Because the host could just continue
> execution of the guest without waiting for measurement check... If the
> Guest Owner _must_ inject a secret for SEV to be secure in any case, we
> might as well choose your path and let the Guest Owner inject the table
> of hashes themselves.
> 
> I'd like to hear your (and others') thoughts.

Brijesh and I had a long discussion over this. I think that if you're
dealing with a malicious hypervisor, then it could, in fact, measure all
the components that it wants to be used and, using LAUNCH_UPDATE_DATA, add
a page, that matches the format of the guest secret page, to the guest and
indicate that page as the guest secret. Even though the measurement would
fail validation by the guest owner, the hypervisor could ignore it and
continue to run the guest.

So you need something that proves ownership of the guest secret - like a
disk key that would fail to unlock the disk if the hypervisor is faking
the guest secret.

Does all that make sense?

Thanks,
Tom

> 
> -Dov
> 

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[James Bottomley]

On Tue, 2021-05-25 at 15:33 -0500, Tom Lendacky wrote:
> On 5/25/21 3:08 PM, Dov Murik wrote:
> > Hi Brijesh,
> > 
> > On 25/05/2021 18:48, Brijesh Singh wrote:
> > > On 5/25/21 12:31 AM, Dov Murik wrote:
> > > > Booting with SEV prevented the loading of kernel, initrd, and
> > > > kernel command-line via QEMU fw_cfg interface because they
> > > > arrive from the VMM which is untrusted in SEV.
> > > > 
> > > > However, in some cases the kernel, initrd, and cmdline are not
> > > > secret but should not be modified by the host.  In such a case,
> > > > we want to verify inside the trusted VM that the kernel,
> > > > initrd, and cmdline are indeed the ones expected by the Guest
> > > > Owner, and only if that is the case go on and boot them up
> > > > (removing the need for grub inside OVMF in that mode).
> > > > 
> > > > This patch series declares a new page in MEMFD which will
> > > > contain the hashes of these three blobs (kernel, initrd,
> > > > cmdline), each under its own GUID entry.  This tables of hashes
> > > > is populated by QEMU before launch, and encrypted as part of
> > > > the initial VM memory; this makes sure theses hashes are part
> > > > of the SEV measurement (which has to be approved by the Guest
> > > > Owner for secret injection, for example).  Note that this
> > > > requires a new QEMU patch which will be submitted soon.
> > > 
> > > I have not looked at the patches, but trying to brainstorm if we
> > > can avoid reserving a new page in the MEMFD and use the existing
> > > EDK2 infrastructure to verify the blobs (kernel, initrd) loaded
> > > through the FW_CFG interface in the guest memory.
> > > 
> > > If I understand correctly, then in your proposed approach, guest
> > > owner wants to ensure that the hypevisor passing its preferred
> > > kernel, initrd and cmdline. The guest owner basically knows the
> > > hashes of these components in advance.
> > 
> > Yes, that's correct.
> > 
> > > So, can we do something like this:
> > > 
> > > - The secret blob provided by the guest owner should contains the
> > > hashes (sha384) of these components.
> > > 
> > > - Use openssl API available in the edk2 to calculate the hash
> > > while loading the kernel, initrd and cmdline.
> > 
> > Indeed we do something similar already - we use Sha256HashAll (see
> > patch 5 in this series).
> > 
> > 
> > > - Before booting the kernel, compare the calculated hash with the
> > > one listed in the secret page. If they don't match then fail
> > > otherwise continue.
> > 
> > That is indeed what we do in patch 6 (the calls to our
> > ValidateHashEntry).
> > 
> > 
> > > Did I miss something ?
> > 
> > Thanks for proposing this.
> > 
> > Your approach has the advantage that there's no need for extra
> > pre-allocated MEMFD page for the hashes, and also it makes the QEMU
> > flow simpler (QEMU doesn't need to compute the hashes and put them
> > in that special MEMFD page).  I think that the only change we'll
> > need from QEMU in the x86_load_linux flow (which is when the user
> > supplies -kernel/-initrd) is that it won't modify any memory in a
> > way that the modifies the hashes that Guest Owner expects (for
> > example, avoid writing over the kernel's setup area).
> > 
> > However, the disadvantage is that it unifies boot measurement with
> > the secret injection.  The Guest Owner _must_ inject the hashes,
> > otherwise the system doesn't boot; whereas in our current
> > suggestion the Guest Owner can check the measurement, verify that
> > everything is OK, and just let the guest continue.
> > 
> > But as I write this, I think that maybe without secret injection
> > the guest is not really secure? Because the host could just
> > continue execution of the guest without waiting for measurement
> > check... If the Guest Owner _must_ inject a secret for SEV to be
> > secure in any case, we might as well choose your path and let the
> > Guest Owner inject the table of hashes themselves.
> > 
> > I'd like to hear your (and others') thoughts.
> 
> Brijesh and I had a long discussion over this. I think that if you're
> dealing with a malicious hypervisor, then it could, in fact, measure
> all the components that it wants to be used and, using
> LAUNCH_UPDATE_DATA, add a page, that matches the format of the guest
> secret page, to the guest and indicate that page as the guest secret.
> Even though the measurement would fail validation by the guest owner,
> the hypervisor could ignore it and continue to run the guest.
> 
> So you need something that proves ownership of the guest secret -
> like a disk key that would fail to unlock the disk if the hypervisor
> is faking the guest secret.
> 
> Does all that make sense?

I think it does for the unencrypted boot case.  For the encrypted boot
case, the HV can't inject the decryption key, because it doesn't know
it, so the interior guest will know there's a problem as soon as it
can't decrypt the image.

But I get the point that we can't rely on the secrets page for hashes
if we have nothing cryptographic in it.  However, the actual threat
from this is somewhat unclear.  As you note: the guest owner knows
there's a problem, but the actual guest is still executing because of
intervention by a malicious hypervisor owner.  In the cloud that's more
or less equivalent to me taking over the guest IP and trying to man in
the middle the services ... once the guest owner knows this happened,
they're going to be looking for a new CSP. So I think the threat would
be potent if you could convince the guest owner that nothing were
amiss, so they think the modified guest is legitimate, but less so
otherwise.

James

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Brijesh Singh]

On 5/25/21 6:15 PM, James Bottomley wrote:
> On Tue, 2021-05-25 at 15:33 -0500, Tom Lendacky wrote:
>> On 5/25/21 3:08 PM, Dov Murik wrote:
>>> Hi Brijesh,
>>>
>>> On 25/05/2021 18:48, Brijesh Singh wrote:
>>>> On 5/25/21 12:31 AM, Dov Murik wrote:
>>>>> Booting with SEV prevented the loading of kernel, initrd, and
>>>>> kernel command-line via QEMU fw_cfg interface because they
>>>>> arrive from the VMM which is untrusted in SEV.
>>>>>
>>>>> However, in some cases the kernel, initrd, and cmdline are not
>>>>> secret but should not be modified by the host.  In such a case,
>>>>> we want to verify inside the trusted VM that the kernel,
>>>>> initrd, and cmdline are indeed the ones expected by the Guest
>>>>> Owner, and only if that is the case go on and boot them up
>>>>> (removing the need for grub inside OVMF in that mode).
>>>>>
>>>>> This patch series declares a new page in MEMFD which will
>>>>> contain the hashes of these three blobs (kernel, initrd,
>>>>> cmdline), each under its own GUID entry.  This tables of hashes
>>>>> is populated by QEMU before launch, and encrypted as part of
>>>>> the initial VM memory; this makes sure theses hashes are part
>>>>> of the SEV measurement (which has to be approved by the Guest
>>>>> Owner for secret injection, for example).  Note that this
>>>>> requires a new QEMU patch which will be submitted soon.
>>>> I have not looked at the patches, but trying to brainstorm if we
>>>> can avoid reserving a new page in the MEMFD and use the existing
>>>> EDK2 infrastructure to verify the blobs (kernel, initrd) loaded
>>>> through the FW_CFG interface in the guest memory.
>>>>
>>>> If I understand correctly, then in your proposed approach, guest
>>>> owner wants to ensure that the hypevisor passing its preferred
>>>> kernel, initrd and cmdline. The guest owner basically knows the
>>>> hashes of these components in advance.
>>> Yes, that's correct.
>>>
>>>> So, can we do something like this:
>>>>
>>>> - The secret blob provided by the guest owner should contains the
>>>> hashes (sha384) of these components.
>>>>
>>>> - Use openssl API available in the edk2 to calculate the hash
>>>> while loading the kernel, initrd and cmdline.
>>> Indeed we do something similar already - we use Sha256HashAll (see
>>> patch 5 in this series).
>>>
>>>
>>>> - Before booting the kernel, compare the calculated hash with the
>>>> one listed in the secret page. If they don't match then fail
>>>> otherwise continue.
>>> That is indeed what we do in patch 6 (the calls to our
>>> ValidateHashEntry).
>>>
>>>
>>>> Did I miss something ?
>>> Thanks for proposing this.
>>>
>>> Your approach has the advantage that there's no need for extra
>>> pre-allocated MEMFD page for the hashes, and also it makes the QEMU
>>> flow simpler (QEMU doesn't need to compute the hashes and put them
>>> in that special MEMFD page).  I think that the only change we'll
>>> need from QEMU in the x86_load_linux flow (which is when the user
>>> supplies -kernel/-initrd) is that it won't modify any memory in a
>>> way that the modifies the hashes that Guest Owner expects (for
>>> example, avoid writing over the kernel's setup area).
>>>
>>> However, the disadvantage is that it unifies boot measurement with
>>> the secret injection.  The Guest Owner _must_ inject the hashes,
>>> otherwise the system doesn't boot; whereas in our current
>>> suggestion the Guest Owner can check the measurement, verify that
>>> everything is OK, and just let the guest continue.
>>>
>>> But as I write this, I think that maybe without secret injection
>>> the guest is not really secure? Because the host could just
>>> continue execution of the guest without waiting for measurement
>>> check... If the Guest Owner _must_ inject a secret for SEV to be
>>> secure in any case, we might as well choose your path and let the
>>> Guest Owner inject the table of hashes themselves.
>>>
>>> I'd like to hear your (and others') thoughts.
>> Brijesh and I had a long discussion over this. I think that if you're
>> dealing with a malicious hypervisor, then it could, in fact, measure
>> all the components that it wants to be used and, using
>> LAUNCH_UPDATE_DATA, add a page, that matches the format of the guest
>> secret page, to the guest and indicate that page as the guest secret.
>> Even though the measurement would fail validation by the guest owner,
>> the hypervisor could ignore it and continue to run the guest.
>>
>> So you need something that proves ownership of the guest secret -
>> like a disk key that would fail to unlock the disk if the hypervisor
>> is faking the guest secret.
>>
>> Does all that make sense?
> I think it does for the unencrypted boot case.  For the encrypted boot
> case, the HV can't inject the decryption key, because it doesn't know
> it, so the interior guest will know there's a problem as soon as it
> can't decrypt the image.
>
> But I get the point that we can't rely on the secrets page for hashes
> if we have nothing cryptographic in it.  However, the actual threat
> from this is somewhat unclear.  As you note: the guest owner knows
> there's a problem, but the actual guest is still executing because of
> intervention by a malicious hypervisor owner.  In the cloud that's more
> or less equivalent to me taking over the guest IP and trying to man in
> the middle the services ... once the guest owner knows this happened,
> they're going to be looking for a new CSP. So I think the threat would
> be potent if you could convince the guest owner that nothing were
> amiss, so they think the modified guest is legitimate, but less so
> otherwise.

I guess if we can have the guest BIOS communicate with the guest owner
using a secure channel to obtain the hash'es or key to decrypt the
hashes then all of it be okay. To establish the secret channel the guest
BIOS will use the secret injected through the LAUNCH_SECRET. If OVMF
fails to establish the secret channel with the guest owner, then its an
indicate that malicious hypervisor booted the guest without injecting a
valid secret or skipped the attestation flow.


>
> James
>
>

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Dov Murik]

On 26/05/2021 2:37, Brijesh Singh wrote:
> 
> On 5/25/21 6:15 PM, James Bottomley wrote:
>> On Tue, 2021-05-25 at 15:33 -0500, Tom Lendacky wrote:
>>> On 5/25/21 3:08 PM, Dov Murik wrote:
>>>> Hi Brijesh,
>>>>
>>>> On 25/05/2021 18:48, Brijesh Singh wrote:
>>>>> On 5/25/21 12:31 AM, Dov Murik wrote:
>>>>>> Booting with SEV prevented the loading of kernel, initrd, and
>>>>>> kernel command-line via QEMU fw_cfg interface because they
>>>>>> arrive from the VMM which is untrusted in SEV.
>>>>>>
>>>>>> However, in some cases the kernel, initrd, and cmdline are not
>>>>>> secret but should not be modified by the host.  In such a case,
>>>>>> we want to verify inside the trusted VM that the kernel,
>>>>>> initrd, and cmdline are indeed the ones expected by the Guest
>>>>>> Owner, and only if that is the case go on and boot them up
>>>>>> (removing the need for grub inside OVMF in that mode).
>>>>>>
>>>>>> This patch series declares a new page in MEMFD which will
>>>>>> contain the hashes of these three blobs (kernel, initrd,
>>>>>> cmdline), each under its own GUID entry.  This tables of hashes
>>>>>> is populated by QEMU before launch, and encrypted as part of
>>>>>> the initial VM memory; this makes sure theses hashes are part
>>>>>> of the SEV measurement (which has to be approved by the Guest
>>>>>> Owner for secret injection, for example).  Note that this
>>>>>> requires a new QEMU patch which will be submitted soon.
>>>>> I have not looked at the patches, but trying to brainstorm if we
>>>>> can avoid reserving a new page in the MEMFD and use the existing
>>>>> EDK2 infrastructure to verify the blobs (kernel, initrd) loaded
>>>>> through the FW_CFG interface in the guest memory.
>>>>>
>>>>> If I understand correctly, then in your proposed approach, guest
>>>>> owner wants to ensure that the hypevisor passing its preferred
>>>>> kernel, initrd and cmdline. The guest owner basically knows the
>>>>> hashes of these components in advance.
>>>> Yes, that's correct.
>>>>
>>>>> So, can we do something like this:
>>>>>
>>>>> - The secret blob provided by the guest owner should contains the
>>>>> hashes (sha384) of these components.
>>>>>
>>>>> - Use openssl API available in the edk2 to calculate the hash
>>>>> while loading the kernel, initrd and cmdline.
>>>> Indeed we do something similar already - we use Sha256HashAll (see
>>>> patch 5 in this series).
>>>>
>>>>
>>>>> - Before booting the kernel, compare the calculated hash with the
>>>>> one listed in the secret page. If they don't match then fail
>>>>> otherwise continue.
>>>> That is indeed what we do in patch 6 (the calls to our
>>>> ValidateHashEntry).
>>>>
>>>>
>>>>> Did I miss something ?
>>>> Thanks for proposing this.
>>>>
>>>> Your approach has the advantage that there's no need for extra
>>>> pre-allocated MEMFD page for the hashes, and also it makes the QEMU
>>>> flow simpler (QEMU doesn't need to compute the hashes and put them
>>>> in that special MEMFD page).  I think that the only change we'll
>>>> need from QEMU in the x86_load_linux flow (which is when the user
>>>> supplies -kernel/-initrd) is that it won't modify any memory in a
>>>> way that the modifies the hashes that Guest Owner expects (for
>>>> example, avoid writing over the kernel's setup area).
>>>>
>>>> However, the disadvantage is that it unifies boot measurement with
>>>> the secret injection.  The Guest Owner _must_ inject the hashes,
>>>> otherwise the system doesn't boot; whereas in our current
>>>> suggestion the Guest Owner can check the measurement, verify that
>>>> everything is OK, and just let the guest continue.
>>>>
>>>> But as I write this, I think that maybe without secret injection
>>>> the guest is not really secure? Because the host could just
>>>> continue execution of the guest without waiting for measurement
>>>> check... If the Guest Owner _must_ inject a secret for SEV to be
>>>> secure in any case, we might as well choose your path and let the
>>>> Guest Owner inject the table of hashes themselves.
>>>>
>>>> I'd like to hear your (and others') thoughts.
>>> Brijesh and I had a long discussion over this. I think that if you're
>>> dealing with a malicious hypervisor, then it could, in fact, measure
>>> all the components that it wants to be used and, using
>>> LAUNCH_UPDATE_DATA, add a page, that matches the format of the guest
>>> secret page, to the guest and indicate that page as the guest secret.
>>> Even though the measurement would fail validation by the guest owner,
>>> the hypervisor could ignore it and continue to run the guest.
>>>
>>> So you need something that proves ownership of the guest secret -
>>> like a disk key that would fail to unlock the disk if the hypervisor
>>> is faking the guest secret.
>>>
>>> Does all that make sense?
>> I think it does for the unencrypted boot case.  For the encrypted boot
>> case, the HV can't inject the decryption key, because it doesn't know
>> it, so the interior guest will know there's a problem as soon as it
>> can't decrypt the image.

I think we all agree that encrypted disk is one good solution to prove
that the guest owner actually provided the secret.


>>
>> But I get the point that we can't rely on the secrets page for hashes
>> if we have nothing cryptographic in it.  However, the actual threat
>> from this is somewhat unclear.  As you note: the guest owner knows
>> there's a problem, but the actual guest is still executing because of
>> intervention by a malicious hypervisor owner.  In the cloud that's more
>> or less equivalent to me taking over the guest IP and trying to man in
>> the middle the services ... once the guest owner knows this happened,
>> they're going to be looking for a new CSP. So I think the threat would
>> be potent if you could convince the guest owner that nothing were
>> amiss, so they think the modified guest is legitimate, but less so
>> otherwise.
> 
> I guess if we can have the guest BIOS communicate with the guest owner
> using a secure channel to obtain the hash'es or key to decrypt the
> hashes then all of it be okay. To establish the secret channel the guest
> BIOS will use the secret injected through the LAUNCH_SECRET. If OVMF
> fails to establish the secret channel with the guest owner, then its an
> indicate that malicious hypervisor booted the guest without injecting a
> valid secret or skipped the attestation flow.
> 

We can separate to two cases:

(1-lax) Guest owner wants proof of integrity/trust for the VMs it asked
to launch (checking the measurement). Cloud provider may be able to
launch additional instances and nobody will know about it.

(2-strict) Guest owner wants control of all VM launches; doesn't allow
cloud provider to launch VMs without approval.




For (1-lax) -- the current proposal works OK.  When Guest Owner asks to
launch a new VM, it'll receive the measurement and check it.  If it is
wrong, the guest owner will stop paying the cloud provider.

In such a case nothing prevents the cloud provider to start the VM on
its own terms -- with SEV disabled, with its own OVMF that performs no
hash checks, etc.



For (2-strict) -- Any check that relies on a simple 'if' statement (like
'if hash(kernel)==expected_hash' or 'if verify_sig(some_blob,
guest_owner_pub_key)') can be circumvented by modifying the code of the
if statement itself (or simply using an OVMF that doesn't perform these
checks).  This includes our original proposal in this patch series, and
Brijesh's idea of communicating with guest owner to get the hashes and
then check them.  Like Tom said, measurement doesn't match, but
malicious cloud provider doesn't care because it doesn't need the secret
injection from the guest owner.

It seems like the guest workload *must* heavily rely on an injected
secret which can be one (or more) of:

(a) a decryption key; it can be an encrypted root partition, or some
other crucial (and unguessable[*]) part of the guest workload.

(b) an API key/credentials that are checked *outside* the cloud
provider's reach; the response from that API is a crucial part of the
guest workload.

(c) asymmetric key (like SSH private key) to access a crucial service
which is part of the guest workload.

The crux here is the "crucial part of the guest workload"; the cloud
provider can start the VM without the secret, but such VM can't perform
its actual (confidential) mission.


[*] Initially I thought about adding encrypted-kernel (similar to
encrypted boot partition; the kernel is decrypted in OVMF using the
injected secret).  But I think a malicious hypervisor can simply guess
what kernel is needed, and provide its own cleartext kernel (with plain
OVMF) and successfully boot the guest without the secret.


-Dov

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Laszlo Ersek]

On 05/25/21 07:31, Dov Murik wrote:
> Booting with SEV prevented the loading of kernel, initrd, and kernel
> command-line via QEMU fw_cfg interface because they arrive from the VMM
> which is untrusted in SEV.
> 
> However, in some cases the kernel, initrd, and cmdline are not secret
> but should not be modified by the host.  In such a case, we want to
> verify inside the trusted VM that the kernel, initrd, and cmdline are
> indeed the ones expected by the Guest Owner, and only if that is the
> case go on and boot them up (removing the need for grub inside OVMF in
> that mode).
> 
> This patch series declares a new page in MEMFD which will contain the
> hashes of these three blobs (kernel, initrd, cmdline), each under its
> own GUID entry.  This tables of hashes is populated by QEMU before
> launch, and encrypted as part of the initial VM memory; this makes sure
> theses hashes are part of the SEV measurement (which has to be approved
> by the Guest Owner for secret injection, for example).  Note that this
> requires a new QEMU patch which will be submitted soon.
> 
> OVMF parses the table of hashes populated by QEMU (patch 5), and as it
> reads the fw_cfg blobs from QEMU, it will verify each one against the
> expected hash (kernel and initrd verifiers are introduced in patch 6,
> and command-line verifier is introduced in patches 7+8).  This is all
> done inside the trusted VM context.  If all the hashes are correct, boot
> of the kernel is allowed to continue.
> 
> Any attempt by QEMU to modify the kernel, initrd, cmdline (including
> dropping one of them), or to modify the OVMF code that verifies those
> hashes, will cause the initial SEV measurement to change and therefore
> will be detectable by the Guest Owner during launch before secret
> injection.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ashish Kalra <ashish.kalra@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> 
> James Bottomley (8):
>   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
>   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
>   OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
>   OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
>   OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
>     device
>   OvmfPkg/AmdSev: Add firmware file plugin to verifier
>   OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
>   OvmfPkg/AmdSev: add SevQemuLoadImageLib
> 
>  OvmfPkg/OvmfPkg.dec                                                       |  10 ++
>  OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
>  OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
>  OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
>  OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
>  OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
>  OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
>  OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
>  21 files changed, 587 insertions(+), 3 deletions(-)
>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
>  create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
> 

I'm confirming that this series is in my review queue.

However, I may need unusually long time to get to it. Thanks for your
patience.

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Laszlo Ersek]

Ard,

I'll have a specific question for you below; please feel free to jump
forward (search for your name). Thanks.

Dov, my comments below:

On 05/25/21 07:31, Dov Murik wrote:
> Booting with SEV prevented the loading of kernel, initrd, and kernel
> command-line via QEMU fw_cfg interface because they arrive from the VMM
> which is untrusted in SEV.
> 
> However, in some cases the kernel, initrd, and cmdline are not secret
> but should not be modified by the host.  In such a case, we want to
> verify inside the trusted VM that the kernel, initrd, and cmdline are
> indeed the ones expected by the Guest Owner, and only if that is the
> case go on and boot them up (removing the need for grub inside OVMF in
> that mode).
> 
> This patch series declares a new page in MEMFD which will contain the
> hashes of these three blobs (kernel, initrd, cmdline), each under its
> own GUID entry.  This tables of hashes is populated by QEMU before
> launch, and encrypted as part of the initial VM memory; this makes sure
> theses hashes are part of the SEV measurement (which has to be approved
> by the Guest Owner for secret injection, for example).  Note that this
> requires a new QEMU patch which will be submitted soon.
> 
> OVMF parses the table of hashes populated by QEMU (patch 5), and as it
> reads the fw_cfg blobs from QEMU, it will verify each one against the
> expected hash (kernel and initrd verifiers are introduced in patch 6,
> and command-line verifier is introduced in patches 7+8).  This is all
> done inside the trusted VM context.  If all the hashes are correct, boot
> of the kernel is allowed to continue.
> 
> Any attempt by QEMU to modify the kernel, initrd, cmdline (including
> dropping one of them), or to modify the OVMF code that verifies those
> hashes, will cause the initial SEV measurement to change and therefore
> will be detectable by the Guest Owner during launch before secret
> injection.

Please catch the error in my reasoning below.

The goal is for the guest firmware to ensure the authenticity
(integrity) of kernel, initrd, cmdline.

This is not really different from a normal Secure Boot setup, where the
guest firmware verifies the kernel image (presented as a UEFI
application, i.e. with the UEFI stub). It is up to the kernel to verify
the integrity of the initrd. The command line is not particularly
verified (as far as I know?), but if that's a problem, it should be
solved even for bare metal Secure Boot use cases. (Because, if the
"root" user is compromised on a running Linux system, they can modify
the kernel params for next boot in the grub config.)

The AmdSevX64 platform uses a unified firmware image (executable +
varstore are presented as one blob, no separate CODE and VARS). There is
one pflash chip, and the initial guest-owner-side measurement covers the
whole blob, including the varstore.

This suggests that the guest owner could boot the unified firmware image
in a trusted guest environment first, and use UEFI-level tools to enroll
various SB certificates. Then this modified image would be deployed
every time to the untrusted cloud.

The AmdSevX64 platform could adopt a PlatformBootManagerLib instance
where the TryRunningQemuKernel() call is reinstated, backed by the usual
QemuLoadImageLib class APIs QemuLoadKernelImage() and
QemuStartKernelImage().

edk2 offers two QemuLoadImageLib instances, GenericQemuLoadImageLib and
X86QemuLoadImageLib. The former strictly enforces SB verification. That
was in fact a *problem* for the traditional OvmfPkg platforms; please
refer to commit 82808b422617 ("Revert "OvmfPkg: use generic QEMU image
loader for secure boot enabled ..."", 2020-06-16). But the same rigor
seems just right here, for the AmdSevX64 platform.

Where I see a gap in all this myself -- and of course there could be
plenty other gaps that I just don't see -- is the varstore's protection
from the hypervisor, once the guest is up and running. Can we discuss
that perhaps?

If necessary, we could perhaps rework the AmdSevX64 platform to drop the
pflash-backed variable driver stack, and use in-RAM (memory-only)
variable emulation. Actual persistence / non-volatility of UEFI
variables may not really be relevant for the remotely attested platform,
but keeping all the variables in RAM would subject the varstore to
memory encryption / protection. And perhaps we could integrate the
enrollment of SB certificates into the *code* part of the firmware, with
gRT->SetVariable() calls. (Normally this would be absolutely horrible,
but for the remotely attested platform, anything goes.)

I simply dislike adding brand new code for a use case which at least
*appears* to significantly overlap with that of Secure Boot. Secure Boot
is about image verification, and it's rooted in tamper-resistant storage
of certificates and/or image hashes. If we can figure out "tamper
resistant" in the current context, we could perhaps reuse much of the
existent SB infrastructure.

----*----

Considering the particular approach in this set:

- To reiterate Brijesh's point, I feel a new MEMFD page is wasteful. If
we really need the MEMFD approach, I'd *really* like us to extend one of
the existent structures. If necessary, introduce a new GUID, for a table
that contains both previously injected data, and the new data. If all
that's impossible or too awkward, please document why.

- Modifying the QemuFwCfgLib class for this purpose is inappropriate.
Even if we do our own home-brewed verifier, none of it must go into
QemuFwCfgLib class. QemuFwCfgLib is for transport.

[Ard, please see this one question:]

- A major complication for hashing all three of: kernel, initrd,
cmdline, is that the *fetching* of this triplet is split between two
places. (Well, it is split between *three* places in fact, but I'm going
to ignore LinuxInitrdDynamicShellCommand for now, because the AmdSevX64
platform sets BUILD_SHELL to FALSE for production.)

The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but the
command line is fetched in (both) QemuLoadImageLib instances. This
requires that all these modules be littered with hashing as well, which
I find *really bad*. Even if we factor out the actual logic, I strongly
dislike having *just hooks* for hashing in multiple modules.

Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe: don't
expose kernel command line", 2020-03-05). If we first

(a) reverted that commit, and

(b) modified *both* QemuLoadImageLib instances, to load the kernel
command line from the *synthetic filesystem* (rather than directly from
fw_cfg),

then we could centralize the hashing to just QemuKernelLoaderFsDxe.

Ard -- what's your thought on this?


And then, we could eliminate the dynamic callback registration, plus the
separate SevFwCfgVerifier, SevHashFinderLib, and SevQemuLoadImageLib stuff.

We'd only need one new lib class, with *statically linked* hooks for
QemuKernelLoaderFsDxe, and two instances of this new class, a Null one,
and an actual (SEV hash verifier) one. The latter instance would locate
the hash values, calculate the fresh hashes, and perform the
comparisons. Only the AmdSevX64 platform would use the non-Null instance
of this library class.

(NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
resolutions to the Null instance would be required there too.)

Thanks
Laszlo



> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ashish Kalra <ashish.kalra@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> 
> James Bottomley (8):
>   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
>   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
>   OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
>   OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
>   OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
>     device
>   OvmfPkg/AmdSev: Add firmware file plugin to verifier
>   OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
>   OvmfPkg/AmdSev: add SevQemuLoadImageLib
> 
>  OvmfPkg/OvmfPkg.dec                                                       |  10 ++
>  OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
>  OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
>  OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
>  OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
>  OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
>  OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
>  OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
>  21 files changed, 587 insertions(+), 3 deletions(-)
>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
>  create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
> 

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Ard Biesheuvel]

On Tue, 1 Jun 2021 at 14:12, Laszlo Ersek <lersek@redhat.com> wrote:
>
...
> - A major complication for hashing all three of: kernel, initrd,
> cmdline, is that the *fetching* of this triplet is split between two
> places. (Well, it is split between *three* places in fact, but I'm going
> to ignore LinuxInitrdDynamicShellCommand for now, because the AmdSevX64
> platform sets BUILD_SHELL to FALSE for production.)
>
> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but the
> command line is fetched in (both) QemuLoadImageLib instances. This
> requires that all these modules be littered with hashing as well, which
> I find *really bad*. Even if we factor out the actual logic, I strongly
> dislike having *just hooks* for hashing in multiple modules.
>
> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe: don't
> expose kernel command line", 2020-03-05). If we first
>
> (a) reverted that commit, and
>
> (b) modified *both* QemuLoadImageLib instances, to load the kernel
> command line from the *synthetic filesystem* (rather than directly from
> fw_cfg),
>
> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>
> Ard -- what's your thought on this?
>

I don't have any problems with that - I take it this means we can drop
the QemuFwCfgLib dependency from GenericQemuLoadImageLib altogether,
right?

>
> And then, we could eliminate the dynamic callback registration, plus the
> separate SevFwCfgVerifier, SevHashFinderLib, and SevQemuLoadImageLib stuff.
>
> We'd only need one new lib class, with *statically linked* hooks for
> QemuKernelLoaderFsDxe, and two instances of this new class, a Null one,
> and an actual (SEV hash verifier) one. The latter instance would locate
> the hash values, calculate the fresh hashes, and perform the
> comparisons. Only the AmdSevX64 platform would use the non-Null instance
> of this library class.
>
> (NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
> resolutions to the Null instance would be required there too.)
>

This sounds like a good approach to me.


>
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Ashish Kalra <ashish.kalra@amd.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Cc: Erdem Aktas <erdemaktas@google.com>
> > Cc: James Bottomley <jejb@linux.ibm.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Min Xu <min.m.xu@intel.com>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> >
> > James Bottomley (8):
> >   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
> >   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
> >   OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
> >   OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
> >   OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
> >     device
> >   OvmfPkg/AmdSev: Add firmware file plugin to verifier
> >   OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
> >   OvmfPkg/AmdSev: add SevQemuLoadImageLib
> >
> >  OvmfPkg/OvmfPkg.dec                                                       |  10 ++
> >  OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
> >  OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
> >  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
> >  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
> >  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
> >  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
> >  OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
> >  OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
> >  OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
> >  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
> >  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
> >  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
> >  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
> >  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
> >  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
> >  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
> >  OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
> >  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
> >  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
> >  OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
> >  21 files changed, 587 insertions(+), 3 deletions(-)
> >  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
> >  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
> >  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
> >  create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
> >  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
> >  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
> >  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
> >  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
> >
>

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Laszlo Ersek]

On 06/01/21 15:20, Ard Biesheuvel wrote:
> On Tue, 1 Jun 2021 at 14:12, Laszlo Ersek <lersek@redhat.com> wrote:
>>
> ...
>> - A major complication for hashing all three of: kernel, initrd,
>> cmdline, is that the *fetching* of this triplet is split between two
>> places. (Well, it is split between *three* places in fact, but I'm
>> going to ignore LinuxInitrdDynamicShellCommand for now, because the
>> AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>
>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>> the command line is fetched in (both) QemuLoadImageLib instances.
>> This requires that all these modules be littered with hashing as
>> well, which I find *really bad*. Even if we factor out the actual
>> logic, I strongly dislike having *just hooks* for hashing in multiple
>> modules.
>>
>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>> don't expose kernel command line", 2020-03-05). If we first
>>
>> (a) reverted that commit, and
>>
>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>> command line from the *synthetic filesystem* (rather than directly
>> from fw_cfg),
>>
>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>
>> Ard -- what's your thought on this?
>>
>
> I don't have any problems with that - I take it this means we can drop
> the QemuFwCfgLib dependency from GenericQemuLoadImageLib altogether,
> right?

A bit more work is needed for that (but I agree it should be done),
because we have this additionally:

  QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize);
  InitrdSize = (UINTN)QemuFwCfgRead32 ();

  if (InitrdSize > 0) {
    //
    // Append ' initrd=initrd' in UTF-16.
    //
    KernelLoadedImage->LoadOptionsSize += sizeof (L" initrd=initrd") - 2;
  }

This should also be rebased on top of the synthetic filesystem [*], and
then no more QemuFwCfgLib calls should remain.

[*] From StubFileOpen() in "QemuKernelLoaderFsDxe.c", it seems that we
    successfully open zero-sized fw_cfg files. (We also list them, when
    reading the root directory, in StubFileRead()). That's not a problem
    at all, but it means that, after opening the initrd file temporarily
    in QemuLoadImageLib, EFI_FILE_PROTOCOL.GetInfo() should be used for
    fetching an EFI_FILE_INFO, and then EFI_FILE_INFO.FileSize needs to
    be compared to 0.


>
>>
>> And then, we could eliminate the dynamic callback registration, plus
>> the separate SevFwCfgVerifier, SevHashFinderLib, and
>> SevQemuLoadImageLib stuff.
>>
>> We'd only need one new lib class, with *statically linked* hooks for
>> QemuKernelLoaderFsDxe, and two instances of this new class, a Null
>> one, and an actual (SEV hash verifier) one. The latter instance would
>> locate the hash values, calculate the fresh hashes, and perform the
>> comparisons. Only the AmdSevX64 platform would use the non-Null
>> instance of this library class.
>>
>> (NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
>> resolutions to the Null instance would be required there too.)
>>
>
> This sounds like a good approach to me.

Thank you!
Laszlo

>
>
>>
>>>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Ashish Kalra <ashish.kalra@amd.com>
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>> Cc: Erdem Aktas <erdemaktas@google.com>
>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Min Xu <min.m.xu@intel.com>
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> James Bottomley (8):
>>>   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
>>>   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
>>>   OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
>>>   OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
>>>   OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
>>>     device
>>>   OvmfPkg/AmdSev: Add firmware file plugin to verifier
>>>   OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
>>>   OvmfPkg/AmdSev: add SevQemuLoadImageLib
>>>
>>>  OvmfPkg/OvmfPkg.dec                                                       |  10 ++
>>>  OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
>>>  OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
>>>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
>>>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
>>>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
>>>  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
>>>  OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
>>>  OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
>>>  OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
>>>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
>>>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
>>>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
>>>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
>>>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
>>>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
>>>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
>>>  OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
>>>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
>>>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
>>>  OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
>>>  21 files changed, 587 insertions(+), 3 deletions(-)
>>>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
>>>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
>>>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
>>>  create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
>>>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
>>>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
>>>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
>>>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
>>>
>>
>
>
> 
>
>

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[James Bottomley]

On Tue, 2021-06-01 at 14:11 +0200, Laszlo Ersek wrote:
> Ard,
> 
> I'll have a specific question for you below; please feel free to jump
> forward (search for your name). Thanks.
> 
> Dov, my comments below:
> 
> On 05/25/21 07:31, Dov Murik wrote:
> > Booting with SEV prevented the loading of kernel, initrd, and
> > kernel command-line via QEMU fw_cfg interface because they arrive
> > from the VMM which is untrusted in SEV.
> > 
> > However, in some cases the kernel, initrd, and cmdline are not
> > secret but should not be modified by the host.  In such a case, we
> > want to verify inside the trusted VM that the kernel, initrd, and
> > cmdline are indeed the ones expected by the Guest Owner, and only
> > if that is the case go on and boot them up (removing the need for
> > grub inside OVMF in that mode).
> > 
> > This patch series declares a new page in MEMFD which will contain
> > the hashes of these three blobs (kernel, initrd, cmdline), each
> > under its own GUID entry.  This tables of hashes is populated by
> > QEMU before launch, and encrypted as part of the initial VM memory;
> > this makes sure theses hashes are part of the SEV measurement
> > (which has to be approved by the Guest Owner for secret injection,
> > for example).  Note that this requires a new QEMU patch which will
> > be submitted soon.
> > 
> > OVMF parses the table of hashes populated by QEMU (patch 5), and as
> > it reads the fw_cfg blobs from QEMU, it will verify each one
> > against the expected hash (kernel and initrd verifiers are
> > introduced in patch 6, and command-line verifier is introduced in
> > patches 7+8).  This is all done inside the trusted VM context.  If
> > all the hashes are correct, boot of the kernel is allowed to continue.
> > 
> > Any attempt by QEMU to modify the kernel, initrd, cmdline
> > (including dropping one of them), or to modify the OVMF code that
> > verifies those hashes, will cause the initial SEV measurement to
> > change and therefore will be detectable by the Guest Owner during
> > launch before secret injection.
> 
> Please catch the error in my reasoning below.
> 
> The goal is for the guest firmware to ensure the authenticity
> (integrity) of kernel, initrd, cmdline.

That's right

> This is not really different from a normal Secure Boot setup, where
> the guest firmware verifies the kernel image (presented as a UEFI
> application, i.e. with the UEFI stub). It is up to the kernel to
> verify the integrity of the initrd.

Right, but this doesn't happen today (there's no initrd measure in the
kernel), so the only current choice you have is to combine the kernel
and initrd then sign the whole combination, which gives one signature
for both.

>  The command line is not particularly verified (as far as I know?), 

Right, it's not, which means secure boot can't replace the
verification, because the command line is an essential thing to measure
given the things it can do to the booting kernel.

> but if that's a problem, it should be solved even for bare metal
> Secure Boot use cases. (Because, if the "root" user is compromised on
> a running Linux system, they can modify the kernel params for next
> boot in the grub config.)

The usual statement to this is that secure boot doesn't need to do this
because the commandline is a measured boot problem, so grub measures
its entire execution to PCR 8.  However, then you have to grope around
in the measurement log and verify it via a TPM quote, which is
incredibly sophisticated stuff compared to taking a single measurement
hash.  Plus, we have no secure TPM currently for virtual machines, so
there's no measured boot measurement the guest owner can trust.

> The AmdSevX64 platform uses a unified firmware image (executable +
> varstore are presented as one blob, no separate CODE and VARS). There
> is one pflash chip, and the initial guest-owner-side measurement
> covers the whole blob, including the varstore.

That's right, except being part of a single rom volume, the varstore is
read only.  This is a deliberate design choice: the absence of SMM and
the fact that a R/W interface wouldn't get measured properly and also
because NV  config changes can be used to effect secret leakage means
it needs to be immutable.

If it's mutable, you need to store it separately, protect it with
something like SMM and be aware of how the measurement would change
when the store was updated ... which is another thing that's not that
easy because of the way flash operates on overwrite.

Finally there's the secure write problem: the DMA for the variable
write would have to go through an unencrypted buffer (because that's
the only way DMA works in confidential computing today).  For disks,
including boot, we cope with this by using an encrypted disk, so we
take the hardware protected page, encrypt it and place it in the clear
DMA buffer for disk write, meaning confidentiality and integrity are
preserved.  We'd have to add an encryption mechanism to pflash or
something to match this process.

> This suggests that the guest owner could boot the unified firmware
> image in a trusted guest environment first, and use UEFI-level tools
> to enroll various SB certificates. Then this modified image would be
> deployed every time to the untrusted cloud.

No, because of the read only nature of the NV store.  You could alter
it from outside using EDK tools, but you can't update it from inside. 
There's also the trust problem: in a current boot OVMF is provided by
the cloud service provider (this could be changed by re-engineering the
control plane, but it's the way it works today), we were thinking
having a single trusted OVMF provided by an outside entity (i.e. Red
Hat for the IBM cloud) would give higher confidence in the solution. 
There may be cases where customers insist on rolling their own, but the
hope is most of them would use a standard package.

> The AmdSevX64 platform could adopt a PlatformBootManagerLib instance
> where the TryRunningQemuKernel() call is reinstated, backed by the
> usual QemuLoadImageLib class APIs QemuLoadKernelImage() and
> QemuStartKernelImage().
> 
> edk2 offers two QemuLoadImageLib instances, GenericQemuLoadImageLib
> and X86QemuLoadImageLib. The former strictly enforces SB
> verification. That was in fact a *problem* for the traditional
> OvmfPkg platforms; please refer to commit 82808b422617 ("Revert
> "OvmfPkg: use generic QEMU image loader for secure boot enabled
> ..."", 2020-06-16). But the same rigor seems just right here, for the
> AmdSevX64 platform.
> 
> Where I see a gap in all this myself -- and of course there could be
> plenty other gaps that I just don't see -- is the varstore's
> protection from the hypervisor, once the guest is up and running. Can
> we discuss that perhaps?

I think it could possibly be done, but we're missing the encrypted
pflash, the measurement computation, and the kernel measuring initrd
and cmdline, all of which have to be controlled by the guest owner.

I did consider the secure boot variable paths, but given the complexity
explosion and all the missing pieces it looked like a bit of a non
starter compared to adding the hashes, which is fairly simple.

> If necessary, we could perhaps rework the AmdSevX64 platform to drop
> the pflash-backed variable driver stack, and use in-RAM (memory-only)
> variable emulation. Actual persistence / non-volatility of UEFI
> variables may not really be relevant for the remotely attested
> platform, but keeping all the variables in RAM would subject the
> varstore to memory encryption / protection. And perhaps we could
> integrate the enrollment of SB certificates into the *code* part of
> the firmware, with gRT->SetVariable() calls. (Normally this would be
> absolutely horrible, but for the remotely attested platform, anything
> goes.)
> 
> I simply dislike adding brand new code for a use case which at least
> *appears* to significantly overlap with that of Secure Boot. Secure
> Boot is about image verification, and it's rooted in tamper-resistant
> storage of certificates and/or image hashes. If we can figure out
> "tamper resistant" in the current context, we could perhaps reuse
> much of the existent SB infrastructure.

The problem even with sharing code paths is secure boot it about
authenticode hashes.  Most of what we're hashing for confidential
computing isn't even authenticode, so we'd have to ream the SecurityPkg
fairly comprehensively to get it to take either authenticode on the
image path or linear hashes for the other elements.  The way grub does
this is to have a separate verifier plugin for the measurements of any
opened file, but even in the grub case that does a linear hash of the
kernel rather than an authenticode one.

I'll let Dov answer the implementation details.

Regards,

James

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Laszlo Ersek]

Hi James,

thanks for the answer, one comment below:

On 06/02/21 20:10, James Bottomley wrote:
> On Tue, 2021-06-01 at 14:11 +0200, Laszlo Ersek wrote:
>> Ard,
>>
>> I'll have a specific question for you below; please feel free to jump
>> forward (search for your name). Thanks.
>>
>> Dov, my comments below:
>>
>> On 05/25/21 07:31, Dov Murik wrote:
>>> Booting with SEV prevented the loading of kernel, initrd, and
>>> kernel command-line via QEMU fw_cfg interface because they arrive
>>> from the VMM which is untrusted in SEV.
>>>
>>> However, in some cases the kernel, initrd, and cmdline are not
>>> secret but should not be modified by the host.  In such a case, we
>>> want to verify inside the trusted VM that the kernel, initrd, and
>>> cmdline are indeed the ones expected by the Guest Owner, and only
>>> if that is the case go on and boot them up (removing the need for
>>> grub inside OVMF in that mode).
>>>
>>> This patch series declares a new page in MEMFD which will contain
>>> the hashes of these three blobs (kernel, initrd, cmdline), each
>>> under its own GUID entry.  This tables of hashes is populated by
>>> QEMU before launch, and encrypted as part of the initial VM memory;
>>> this makes sure theses hashes are part of the SEV measurement
>>> (which has to be approved by the Guest Owner for secret injection,
>>> for example).  Note that this requires a new QEMU patch which will
>>> be submitted soon.
>>>
>>> OVMF parses the table of hashes populated by QEMU (patch 5), and as
>>> it reads the fw_cfg blobs from QEMU, it will verify each one
>>> against the expected hash (kernel and initrd verifiers are
>>> introduced in patch 6, and command-line verifier is introduced in
>>> patches 7+8).  This is all done inside the trusted VM context.  If
>>> all the hashes are correct, boot of the kernel is allowed to continue.
>>>
>>> Any attempt by QEMU to modify the kernel, initrd, cmdline
>>> (including dropping one of them), or to modify the OVMF code that
>>> verifies those hashes, will cause the initial SEV measurement to
>>> change and therefore will be detectable by the Guest Owner during
>>> launch before secret injection.
>>
>> Please catch the error in my reasoning below.
>>
>> The goal is for the guest firmware to ensure the authenticity
>> (integrity) of kernel, initrd, cmdline.
> 
> That's right
> 
>> This is not really different from a normal Secure Boot setup, where
>> the guest firmware verifies the kernel image (presented as a UEFI
>> application, i.e. with the UEFI stub). It is up to the kernel to
>> verify the integrity of the initrd.
> 
> Right, but this doesn't happen today (there's no initrd measure in the
> kernel), so the only current choice you have is to combine the kernel
> and initrd then sign the whole combination, which gives one signature
> for both.
> 
>>  The command line is not particularly verified (as far as I know?), 
> 
> Right, it's not, which means secure boot can't replace the
> verification, because the command line is an essential thing to measure
> given the things it can do to the booting kernel.
> 
>> but if that's a problem, it should be solved even for bare metal
>> Secure Boot use cases. (Because, if the "root" user is compromised on
>> a running Linux system, they can modify the kernel params for next
>> boot in the grub config.)
> 
> The usual statement to this is that secure boot doesn't need to do this
> because the commandline is a measured boot problem, so grub measures
> its entire execution to PCR 8.  However, then you have to grope around
> in the measurement log and verify it via a TPM quote, which is
> incredibly sophisticated stuff compared to taking a single measurement
> hash.  Plus, we have no secure TPM currently for virtual machines, so
> there's no measured boot measurement the guest owner can trust.
> 
>> The AmdSevX64 platform uses a unified firmware image (executable +
>> varstore are presented as one blob, no separate CODE and VARS). There
>> is one pflash chip, and the initial guest-owner-side measurement
>> covers the whole blob, including the varstore.
> 
> That's right, except being part of a single rom volume, the varstore is
> read only.  This is a deliberate design choice: the absence of SMM and
> the fact that a R/W interface wouldn't get measured properly and also
> because NV  config changes can be used to effect secret leakage means
> it needs to be immutable.
> 
> If it's mutable, you need to store it separately, protect it with
> something like SMM and be aware of how the measurement would change
> when the store was updated ... which is another thing that's not that
> easy because of the way flash operates on overwrite.
> 
> Finally there's the secure write problem: the DMA for the variable
> write would have to go through an unencrypted buffer (because that's
> the only way DMA works in confidential computing today).  For disks,
> including boot, we cope with this by using an encrypted disk, so we
> take the hardware protected page, encrypt it and place it in the clear
> DMA buffer for disk write, meaning confidentiality and integrity are
> preserved.  We'd have to add an encryption mechanism to pflash or
> something to match this process.
> 
>> This suggests that the guest owner could boot the unified firmware
>> image in a trusted guest environment first, and use UEFI-level tools
>> to enroll various SB certificates. Then this modified image would be
>> deployed every time to the untrusted cloud.
> 
> No, because of the read only nature of the NV store.  You could alter
> it from outside using EDK tools, but you can't update it from inside. 
> There's also the trust problem: in a current boot OVMF is provided by
> the cloud service provider (this could be changed by re-engineering the
> control plane, but it's the way it works today), we were thinking
> having a single trusted OVMF provided by an outside entity (i.e. Red
> Hat for the IBM cloud) would give higher confidence in the solution. 
> There may be cases where customers insist on rolling their own, but the
> hope is most of them would use a standard package.

Here I meant, by "booting the unified firmware image in a trusted guest
environment first", that the guest owner would do that on their own
premises, not on an untrusted cloud. In other words, in that initial
run, they wouldn't use any kind of memory encryption. "Trusted
environment" meant that the guest owner would trust their own
environment not to compromise their image in any way. And in this
environment, they could use EnrollDefaultKeys.efi or some other
guest-side tool to pre-enroll certificates.

In the actual production (cloud) environment, read-only access to the
varstore would be sufficient (with the certificates already existing in it).

But, anyway, the rest of your explanation is convincing; and I agree the
general idea of this series is fine. We just need to sort out the
technical details / solution structure, per the second half of my review.

Thanks!
Laszlo


> 
>> The AmdSevX64 platform could adopt a PlatformBootManagerLib instance
>> where the TryRunningQemuKernel() call is reinstated, backed by the
>> usual QemuLoadImageLib class APIs QemuLoadKernelImage() and
>> QemuStartKernelImage().
>>
>> edk2 offers two QemuLoadImageLib instances, GenericQemuLoadImageLib
>> and X86QemuLoadImageLib. The former strictly enforces SB
>> verification. That was in fact a *problem* for the traditional
>> OvmfPkg platforms; please refer to commit 82808b422617 ("Revert
>> "OvmfPkg: use generic QEMU image loader for secure boot enabled
>> ..."", 2020-06-16). But the same rigor seems just right here, for the
>> AmdSevX64 platform.
>>
>> Where I see a gap in all this myself -- and of course there could be
>> plenty other gaps that I just don't see -- is the varstore's
>> protection from the hypervisor, once the guest is up and running. Can
>> we discuss that perhaps?
> 
> I think it could possibly be done, but we're missing the encrypted
> pflash, the measurement computation, and the kernel measuring initrd
> and cmdline, all of which have to be controlled by the guest owner.
> 
> I did consider the secure boot variable paths, but given the complexity
> explosion and all the missing pieces it looked like a bit of a non
> starter compared to adding the hashes, which is fairly simple.
> 
>> If necessary, we could perhaps rework the AmdSevX64 platform to drop
>> the pflash-backed variable driver stack, and use in-RAM (memory-only)
>> variable emulation. Actual persistence / non-volatility of UEFI
>> variables may not really be relevant for the remotely attested
>> platform, but keeping all the variables in RAM would subject the
>> varstore to memory encryption / protection. And perhaps we could
>> integrate the enrollment of SB certificates into the *code* part of
>> the firmware, with gRT->SetVariable() calls. (Normally this would be
>> absolutely horrible, but for the remotely attested platform, anything
>> goes.)
>>
>> I simply dislike adding brand new code for a use case which at least
>> *appears* to significantly overlap with that of Secure Boot. Secure
>> Boot is about image verification, and it's rooted in tamper-resistant
>> storage of certificates and/or image hashes. If we can figure out
>> "tamper resistant" in the current context, we could perhaps reuse
>> much of the existent SB infrastructure.
> 
> The problem even with sharing code paths is secure boot it about
> authenticode hashes.  Most of what we're hashing for confidential
> computing isn't even authenticode, so we'd have to ream the SecurityPkg
> fairly comprehensively to get it to take either authenticode on the
> image path or linear hashes for the other elements.  The way grub does
> this is to have a separate verifier plugin for the measurements of any
> opened file, but even in the grub case that does a linear hash of the
> kernel rather than an authenticode one.
> 
> I'll let Dov answer the implementation details.
> 
> Regards,
> 
> James
> 
> 

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Dov Murik]

Thank you Laszlo for reviewing this.


On 01/06/2021 15:11, Laszlo Ersek wrote:
> Ard,
> 
> I'll have a specific question for you below; please feel free to jump
> forward (search for your name). Thanks.
> 
> Dov, my comments below:
> 
> On 05/25/21 07:31, Dov Murik wrote:
>> Booting with SEV prevented the loading of kernel, initrd, and kernel
>> command-line via QEMU fw_cfg interface because they arrive from the VMM
>> which is untrusted in SEV.
>>
>> However, in some cases the kernel, initrd, and cmdline are not secret
>> but should not be modified by the host.  In such a case, we want to
>> verify inside the trusted VM that the kernel, initrd, and cmdline are
>> indeed the ones expected by the Guest Owner, and only if that is the
>> case go on and boot them up (removing the need for grub inside OVMF in
>> that mode).
>>
>> This patch series declares a new page in MEMFD which will contain the
>> hashes of these three blobs (kernel, initrd, cmdline), each under its
>> own GUID entry.  This tables of hashes is populated by QEMU before
>> launch, and encrypted as part of the initial VM memory; this makes sure
>> theses hashes are part of the SEV measurement (which has to be approved
>> by the Guest Owner for secret injection, for example).  Note that this
>> requires a new QEMU patch which will be submitted soon.
>>
>> OVMF parses the table of hashes populated by QEMU (patch 5), and as it
>> reads the fw_cfg blobs from QEMU, it will verify each one against the
>> expected hash (kernel and initrd verifiers are introduced in patch 6,
>> and command-line verifier is introduced in patches 7+8).  This is all
>> done inside the trusted VM context.  If all the hashes are correct, boot
>> of the kernel is allowed to continue.
>>
>> Any attempt by QEMU to modify the kernel, initrd, cmdline (including
>> dropping one of them), or to modify the OVMF code that verifies those
>> hashes, will cause the initial SEV measurement to change and therefore
>> will be detectable by the Guest Owner during launch before secret
>> injection.
> 
> Please catch the error in my reasoning below.
> 
> The goal is for the guest firmware to ensure the authenticity
> (integrity) of kernel, initrd, cmdline.
> 
> This is not really different from a normal Secure Boot setup, where the
> guest firmware verifies the kernel image (presented as a UEFI
> application, i.e. with the UEFI stub). It is up to the kernel to verify
> the integrity of the initrd. The command line is not particularly
> verified (as far as I know?), but if that's a problem, it should be
> solved even for bare metal Secure Boot use cases. (Because, if the
> "root" user is compromised on a running Linux system, they can modify
> the kernel params for next boot in the grub config.)

James explained the difference from Secure Boot setup and our choice of
hash validation of kernel+initrd+cmdline.


[Skipping...]


> 
> ----*----
> 
> Considering the particular approach in this set:
> 
> - To reiterate Brijesh's point, I feel a new MEMFD page is wasteful. If
> we really need the MEMFD approach, I'd *really* like us to extend one of
> the existent structures. If necessary, introduce a new GUID, for a table
> that contains both previously injected data, and the new data. If all
> that's impossible or too awkward, please document why.
> 

Brijesh's approach mandates the guest owner use of SEV secret injection,
because the approved hashes are injected into the secrets page at
launch.  There are two disadvantages for this approach:

(a) It requires the use of SEV's LAUNCH_SECRET during launch, thus tying
together measurement of approved boot binaries and the secrets (which in
this case will be used by later kernel or userspace processes).  Note
also that the hashes are not confidential (in fact, the host has access
to the full kernel+initrd).

(b) AMD SEV-SNP doesn't support LAUNCH_SECRET any more; instead, in SNP
(/me hand-waving) there's a mechanism to securely verify measurement
later (e.g. during initrd) and if measurement matches then there's a
secure way to retrieve secrets.

My hope is that the approach we proposed (QEMU is building a "hashes
page" which is measured at launch) will be useful also for
secure-booting SNP (and TDX?) guests.  The idea is that in SNP the
initial encrypted memory will include OVMF and the hahses page (as in
SEV); this will be measured at launch and saved by SNP; at a later step,
a userspace process requests the initial measurement from the PSP
hardware (in a secure way - that's only possible in SNP); if that
measurement matches then the userspace process may request some secrets.
 That said, I'm only beginning to learn about these new generations.

So I argue to keep the existing approach with two separate areas:
existing one for injected secrets, and new one for a table of approved
hashes (filled by QEMU and updated as initial encrypted measured guest
memory).

If the issue is MEMFD space, maybe we can do something like: use the
existing secrets page (4KB) for two uses: first 3KB for secrets, and
last 1KB for hashes.  If this is not enough, the hashes are even smaller
than 1KB; and we can even publish only one hash - the hash of all 3
hashes (need to think about edge cases when there's no cmdline/initrd).
 But all these "solutions" feel a bit hacky for me and might complicate
the code.

I don't understand your suggestion: "I'd *really* like us to extend one
of the existent structures. If necessary, introduce a new GUID, for a
table that contains both previously injected data, and the new data.";
does this mean to use a single MEMFD page for the injected secrets and
the hashes?

Also, in general, I don't really understand the implications of running
out of MEMFD place; maybe you have other ideas around this (for example,
can we make MEMFD bigger only for AmdSevX64 platform?).


> - Modifying the QemuFwCfgLib class for this purpose is inappropriate.
> Even if we do our own home-brewed verifier, none of it must go into
> QemuFwCfgLib class. QemuFwCfgLib is for transport.
> 

OK, we'll take the verifier out (as you suggested below - to a
BlobVerifierLib with two implementations).


> [Ard, please see this one question:]
> 
> - A major complication for hashing all three of: kernel, initrd,
> cmdline, is that the *fetching* of this triplet is split between two
> places. (Well, it is split between *three* places in fact, but I'm going
> to ignore LinuxInitrdDynamicShellCommand for now, because the AmdSevX64
> platform sets BUILD_SHELL to FALSE for production.)
> 
> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but the
> command line is fetched in (both) QemuLoadImageLib instances. This
> requires that all these modules be littered with hashing as well, which
> I find *really bad*. Even if we factor out the actual logic, I strongly
> dislike having *just hooks* for hashing in multiple modules.
> 
> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe: don't
> expose kernel command line", 2020-03-05). If we first
> 
> (a) reverted that commit, and
> 
> (b) modified *both* QemuLoadImageLib instances, to load the kernel
> command line from the *synthetic filesystem* (rather than directly from
> fw_cfg),
> 
> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
> 
> Ard -- what's your thought on this?
> 

I understand there's agreement here, and that both this suggested change
(use the synthetic filesystem) and my patch series (add hash
verification) touch the same code areas.  How do you envision this
process in the mailing list?  Seperate patch serieses with dependency?
One long patch series with both changes?  What goes first?


> 
> And then, we could eliminate the dynamic callback registration, plus the
> separate SevFwCfgVerifier, SevHashFinderLib, and SevQemuLoadImageLib stuff.
> 
> We'd only need one new lib class, with *statically linked* hooks for
> QemuKernelLoaderFsDxe, and two instances of this new class, a Null one,
> and an actual (SEV hash verifier) one. The latter instance would locate
> the hash values, calculate the fresh hashes, and perform the
> comparisons. Only the AmdSevX64 platform would use the non-Null instance
> of this library class.

OK, I'll refactor to static linking with two BlobVerifierLib imlementations.


> 
> (NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
> resolutions to the Null instance would be required there too.)

I'll need to learn how to build edk2 for Arm to test this.  Thanks for
the heads-up.


-Dov


> 
> Thanks
> Laszlo
> 
> 
> 
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ashish Kalra <ashish.kalra@amd.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> James Bottomley (8):
>>   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
>>   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
>>   OvmfPkg/AmdSev: add a page to the MEMFD for firmware config hashes
>>   OvmfPkg/QemuKernelLoaderFsDxe: Add ability to verify loaded items
>>   OvmfPkg/AmdSev: Add library to find encrypted hashes for the FwCfg
>>     device
>>   OvmfPkg/AmdSev: Add firmware file plugin to verifier
>>   OvmfPkg: GenericQemuLoadImageLib: Allow verifying fw_cfg command line
>>   OvmfPkg/AmdSev: add SevQemuLoadImageLib
>>
>>  OvmfPkg/OvmfPkg.dec                                                       |  10 ++
>>  OvmfPkg/AmdSev/AmdSevX64.dsc                                              |   9 +-
>>  OvmfPkg/AmdSev/AmdSevX64.fdf                                              |   3 +
>>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf              |  30 +++++
>>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf              |  34 ++++++
>>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf        |  30 +++++
>>  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf |   2 +
>>  OvmfPkg/ResetVector/ResetVector.inf                                       |   2 +
>>  OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h                         |  47 ++++++++
>>  OvmfPkg/Include/Library/QemuFwCfgLib.h                                    |  35 ++++++
>>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                  |  11 ++
>>  OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c                |  60 ++++++++++
>>  OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c                | 126 ++++++++++++++++++++
>>  OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c          |  52 ++++++++
>>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                      |   2 +-
>>  OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c         |  29 +++++
>>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                  |   5 +
>>  OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c                   |  50 ++++++++
>>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                     |  31 +++++
>>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                              |  20 ++++
>>  OvmfPkg/ResetVector/ResetVector.nasmb                                     |   2 +
>>  21 files changed, 587 insertions(+), 3 deletions(-)
>>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.inf
>>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.inf
>>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.inf
>>  create mode 100644 OvmfPkg/AmdSev/Include/Library/SevHashFinderLib.h
>>  create mode 100644 OvmfPkg/AmdSev/Library/SevFwCfgVerifier/SevFwCfgVerifier.c
>>  create mode 100644 OvmfPkg/AmdSev/Library/SevHashFinderLib/SevHashFinderLib.c
>>  create mode 100644 OvmfPkg/AmdSev/Library/SevQemuLoadImageLib/SevQemuLoadImageLib.c
>>  create mode 100644 OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
>>
> 

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Laszlo Ersek]

On 06/04/21 12:30, Dov Murik wrote:

> So I argue to keep the existing approach with two separate areas:
> existing one for injected secrets, and new one for a table of approved
> hashes (filled by QEMU and updated as initial encrypted measured guest
> memory).

OK.

> If the issue is MEMFD space,

Yes, that's it.

> maybe we can do something like: use the
> existing secrets page (4KB) for two uses: first 3KB for secrets, and
> last 1KB for hashes.  If this is not enough, the hashes are even
> smaller than 1KB; and we can even publish only one hash - the hash of
> all 3 hashes (need to think about edge cases when there's no
> cmdline/initrd). But all these "solutions" feel a bit hacky for me and
> might complicate the code.

All these PCDs come in pairs -- base and size. (IIRC.) If there's no
architectural requirement to keep these two kinds of info in different
pages (such as different page protections or whatever), then packing
them into a single page is something I'd like. The above 3K+1K
subdivision sounds OK to me.

>
> I don't understand your suggestion: "I'd *really* like us to extend
> one of the existent structures. If necessary, introduce a new GUID,
> for a table that contains both previously injected data, and the new
> data."; does this mean to use a single MEMFD page for the injected
> secrets and the hashes?

Yes, it's the same (say, 3K+1K) idea, just expressed differently. In one
case, you have two GUIDed structs in the (plaintext, not compressed)
reset vector in the pflash, and the base+size structures associated wth
those two separate GUIDs happen to identify distinct ranges of the same
MEMFD page. In the other case, you have just one GUIDed structure (with
base+size contents), and the page pointed-to by this base+size pair is
subdivided by *internal* structuring -- such as internal GUIDs and so
on. Whichever is simpler to implement in both QEMU and edk2; I just want
to avoid wasing a full page for three hashes.

>
> Also, in general, I don't really understand the implications of
> running out of MEMFD place;

Here's one implication of enlarging MEMFD. It pushes BS Code, BS Data,
Loader Code, Loader Data, perhaps some AcpiNVS and Reserved memory
allocations to higher addresses. Then when the kernel is loaded, its
load address may be higher too. I'm not worried about wasted guest
memory, but abut various silent assumptions as to where the kernel
"should be". For example, after one round of enlarging DXEFV, the
"crash" utility stopped opening guest memory dumps, because it couldn't
find a kernel signature in the (low) address range that it used to scan.
The fix wasn't too difficult (the range to scan could be specified on
the "crash" commadn line, and then my colleague Dave Anderson just
modified "crash"), but it was a *surprise*. I don't like those.

> maybe you have other ideas around this (for example,
> can we make MEMFD bigger only for AmdSevX64 platform?).

Yes, experimenting with a larger MEMFD in just the AmdSevX64 platform is
fine.

NB reordering various PCDs between each other, so that their relative
relationships (orders) change, is a *lot* more risky than just enlarging
existing areas. The code in OVMF tends not to rely on actual bases and
sizes, but it may very well rely on a particular BasePCD + SizePCD sum
not exceeding another particular BasePCD.

>
>
>> - Modifying the QemuFwCfgLib class for this purpose is inappropriate.
>> Even if we do our own home-brewed verifier, none of it must go into
>> QemuFwCfgLib class. QemuFwCfgLib is for transport.
>>
>
> OK, we'll take the verifier out (as you suggested below - to a
> BlobVerifierLib with two implementations).
>
>
>> [Ard, please see this one question:]
>>
>> - A major complication for hashing all three of: kernel, initrd,
>> cmdline, is that the *fetching* of this triplet is split between two
>> places. (Well, it is split between *three* places in fact, but I'm
>> going to ignore LinuxInitrdDynamicShellCommand for now, because the
>> AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>
>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>> the command line is fetched in (both) QemuLoadImageLib instances.
>> This requires that all these modules be littered with hashing as
>> well, which I find *really bad*. Even if we factor out the actual
>> logic, I strongly dislike having *just hooks* for hashing in multiple
>> modules.
>>
>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>> don't expose kernel command line", 2020-03-05). If we first
>>
>> (a) reverted that commit, and
>>
>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>> command line from the *synthetic filesystem* (rather than directly
>> from fw_cfg),
>>
>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>
>> Ard -- what's your thought on this?
>>
>
> I understand there's agreement here, and that both this suggested
> change (use the synthetic filesystem) and my patch series (add hash
> verification) touch the same code areas.  How do you envision this
> process in the mailing list?  Seperate patch serieses with dependency?
> One long patch series with both changes?  What goes first?

Good point. I do have a kind of patch order laid out in my mind, but I
didn't think of whether we should have the patches in one patch series,
or in two "waves".

OK, let's go with two patch sets.

In the first set, we should just focus on the above steps (a) and (b).
Step (a) shouldn't be too hard. In step (b), you'd modify both
QemuLoadImageLib instances (two separate patches), replacing the
QemuFwCfgLib APIs for fetching the cmdline with
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.

Speaking from memory, the synthetic filesystem has a unique device path,
so the first step would be calling gBS->LocateDevicePath(), for finding
SimpleFs on the unique device path. Once you have the SimpleFs
interface, you can call OpenVolume, then open the "cmdline" file using
the EFI_FILE_PROTOCOL output by OpenVolume.

Once we merge this series (basically just three patches), there is no
QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
reckon. Then you can post the second wave, in which:

- a new "firmware config verifier" library class is introduced,

- two library instances for that class are introduced (null, and the
  real thing),

- the AmdSevX64.dsc platform resolves the new lib class to the "real"
  (hashing) instance,

- all other platform DSCs using QemuKernelLoaderFsDxe resolve the new
  lib class to the null instance,

- QemuKernelLoaderFsDxe is extended with a dependency on the new class,
  calling the proper APIs to (a) initialize the verifier, and (b) verify
  every fw_cfg blob that is about to be exposed as a synthetic file.

Then QemuLoadImageLib needs no changes, as it will not depend on fw_cfg,
and every synthetic file it may want to access will have been verified
by QemuKernelLoaderFsDxe already, according to the verifier lib instance
that's used in the respective platform DSC file.

I would recommend only posting the first patch set initially. It has a
very well defined goal (--> hide the fw_cfg dependency in both
QemuLoadImageLib instances behind the synthetic filesystem); we can
validate / review that regardless of the ultimate crypto / security
goal. Using the SimpleFs / FILE protocol APIs is not trivial IMO, so
it's possible that just the first wave will require a v2.

>
>
>>
>> And then, we could eliminate the dynamic callback registration, plus
>> the separate SevFwCfgVerifier, SevHashFinderLib, and
>> SevQemuLoadImageLib stuff.
>>
>> We'd only need one new lib class, with *statically linked* hooks for
>> QemuKernelLoaderFsDxe, and two instances of this new class, a Null
>> one, and an actual (SEV hash verifier) one. The latter instance would
>> locate the hash values, calculate the fresh hashes, and perform the
>> comparisons. Only the AmdSevX64 platform would use the non-Null
>> instance of this library class.
>
> OK, I'll refactor to static linking with two BlobVerifierLib
> imlementations.
>
>
>>
>> (NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
>> resolutions to the Null instance would be required there too.)
>
> I'll need to learn how to build edk2 for Arm to test this.  Thanks for
> the heads-up.

With regard to QemuKernelLoaderFsDxe specifically:

  build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc               -a AARCH64
  build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc       -a ARM
  build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc         -a AARCH64
  build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc -a ARM

If you work on an x86_64 machine, you'll need cross-gcc and
cross-binutils for this. I have the following packages installed on my
laptop:

  binutils-aarch64-linux-gnu-2.31.1-3.el7.x86_64
  binutils-arm-linux-gnu-2.31.1-3.el7.x86_64
  cross-binutils-common-2.31.1-3.el7.noarch

  cross-gcc-common-9.2.1-3.el7.1.noarch
  gcc-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
  gcc-arm-linux-gnu-9.2.1-3.el7.1.x86_64
  gcc-c++-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
  gcc-c++-arm-linux-gnu-9.2.1-3.el7.1.x86_64

(I don't remember why I have the c++ cross-compiler installed.)

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Dov Murik]

On 04/06/2021 14:26, Laszlo Ersek wrote:
> On 06/04/21 12:30, Dov Murik wrote:
> 
>> So I argue to keep the existing approach with two separate areas:
>> existing one for injected secrets, and new one for a table of approved
>> hashes (filled by QEMU and updated as initial encrypted measured guest
>> memory).
> 
> OK.
> 
>> If the issue is MEMFD space,
> 
> Yes, that's it.
> 
>> maybe we can do something like: use the
>> existing secrets page (4KB) for two uses: first 3KB for secrets, and
>> last 1KB for hashes.  If this is not enough, the hashes are even
>> smaller than 1KB; and we can even publish only one hash - the hash of
>> all 3 hashes (need to think about edge cases when there's no
>> cmdline/initrd). But all these "solutions" feel a bit hacky for me and
>> might complicate the code.
> 
> All these PCDs come in pairs -- base and size. (IIRC.) If there's no
> architectural requirement to keep these two kinds of info in different
> pages (such as different page protections or whatever), then packing
> them into a single page is something I'd like. The above 3K+1K
> subdivision sounds OK to me.
> 

I'll go with 3KB secrets + 1KB hashes.


>>
>> I don't understand your suggestion: "I'd *really* like us to extend
>> one of the existent structures. If necessary, introduce a new GUID,
>> for a table that contains both previously injected data, and the new
>> data."; does this mean to use a single MEMFD page for the injected
>> secrets and the hashes?
> 
> Yes, it's the same (say, 3K+1K) idea, just expressed differently. In one
> case, you have two GUIDed structs in the (plaintext, not compressed)
> reset vector in the pflash, and the base+size structures associated wth
> those two separate GUIDs happen to identify distinct ranges of the same
> MEMFD page. In the other case, you have just one GUIDed structure (with
> base+size contents), and the page pointed-to by this base+size pair is
> subdivided by *internal* structuring -- such as internal GUIDs and so
> on. Whichever is simpler to implement in both QEMU and edk2; I just want
> to avoid wasing a full page for three hashes.
> 

I'll go with the two GUIDed structures in the reset vector (which will
point to distinct parts of a single 4KB page).

That actually means shortening the existing secrets MEMFD area from 4KB
to 3KB. Is that OK?



>>
>> Also, in general, I don't really understand the implications of
>> running out of MEMFD place;
> 
> Here's one implication of enlarging MEMFD. It pushes BS Code, BS Data,
> Loader Code, Loader Data, perhaps some AcpiNVS and Reserved memory
> allocations to higher addresses. Then when the kernel is loaded, its
> load address may be higher too. I'm not worried about wasted guest
> memory, but abut various silent assumptions as to where the kernel
> "should be". For example, after one round of enlarging DXEFV, the
> "crash" utility stopped opening guest memory dumps, because it couldn't
> find a kernel signature in the (low) address range that it used to scan.
> The fix wasn't too difficult (the range to scan could be specified on
> the "crash" commadn line, and then my colleague Dave Anderson just
> modified "crash"), but it was a *surprise*. I don't like those.
> 
>> maybe you have other ideas around this (for example,
>> can we make MEMFD bigger only for AmdSevX64 platform?).
> 
> Yes, experimenting with a larger MEMFD in just the AmdSevX64 platform is
> fine.
> 

But now I understand that failures can appear way later in userspace
(the crash utility), so just testing that a modern AMD VM boots fine is
not enough to get confidence here.


> NB reordering various PCDs between each other, so that their relative
> relationships (orders) change, is a *lot* more risky than just enlarging
> existing areas. The code in OVMF tends not to rely on actual bases and
> sizes, but it may very well rely on a particular BasePCD + SizePCD sum
> not exceeding another particular BasePCD.
> 

Thanks for pointing this out. I'll avoid reordering.


>>
>>
>>> - Modifying the QemuFwCfgLib class for this purpose is inappropriate.
>>> Even if we do our own home-brewed verifier, none of it must go into
>>> QemuFwCfgLib class. QemuFwCfgLib is for transport.
>>>
>>
>> OK, we'll take the verifier out (as you suggested below - to a
>> BlobVerifierLib with two implementations).
>>
>>
>>> [Ard, please see this one question:]
>>>
>>> - A major complication for hashing all three of: kernel, initrd,
>>> cmdline, is that the *fetching* of this triplet is split between two
>>> places. (Well, it is split between *three* places in fact, but I'm
>>> going to ignore LinuxInitrdDynamicShellCommand for now, because the
>>> AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>>
>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>>> the command line is fetched in (both) QemuLoadImageLib instances.
>>> This requires that all these modules be littered with hashing as
>>> well, which I find *really bad*. Even if we factor out the actual
>>> logic, I strongly dislike having *just hooks* for hashing in multiple
>>> modules.
>>>
>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>>> don't expose kernel command line", 2020-03-05). If we first
>>>
>>> (a) reverted that commit, and
>>>
>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>>> command line from the *synthetic filesystem* (rather than directly
>>> from fw_cfg),
>>>
>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>>
>>> Ard -- what's your thought on this?
>>>
>>
>> I understand there's agreement here, and that both this suggested
>> change (use the synthetic filesystem) and my patch series (add hash
>> verification) touch the same code areas.  How do you envision this
>> process in the mailing list?  Seperate patch serieses with dependency?
>> One long patch series with both changes?  What goes first?
> 
> Good point. I do have a kind of patch order laid out in my mind, but I
> didn't think of whether we should have the patches in one patch series,
> or in two "waves".
> 
> OK, let's go with two patch sets.
> 
> In the first set, we should just focus on the above steps (a) and (b).
> Step (a) shouldn't be too hard. In step (b), you'd modify both
> QemuLoadImageLib instances (two separate patches), replacing the
> QemuFwCfgLib APIs for fetching the cmdline with
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
> 
> Speaking from memory, the synthetic filesystem has a unique device path,
> so the first step would be calling gBS->LocateDevicePath(), for finding
> SimpleFs on the unique device path. Once you have the SimpleFs
> interface, you can call OpenVolume, then open the "cmdline" file using
> the EFI_FILE_PROTOCOL output by OpenVolume.
> 
> Once we merge this series (basically just three patches), there is no
> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
> reckon. Then you can post the second wave, in which:
> 
> - a new "firmware config verifier" library class is introduced,
> 
> - two library instances for that class are introduced (null, and the
>   real thing),
> 
> - the AmdSevX64.dsc platform resolves the new lib class to the "real"
>   (hashing) instance,
> 
> - all other platform DSCs using QemuKernelLoaderFsDxe resolve the new
>   lib class to the null instance,
> 
> - QemuKernelLoaderFsDxe is extended with a dependency on the new class,
>   calling the proper APIs to (a) initialize the verifier, and (b) verify
>   every fw_cfg blob that is about to be exposed as a synthetic file.
> 
> Then QemuLoadImageLib needs no changes, as it will not depend on fw_cfg,
> and every synthetic file it may want to access will have been verified
> by QemuKernelLoaderFsDxe already, according to the verifier lib instance
> that's used in the respective platform DSC file.
> 
> I would recommend only posting the first patch set initially. It has a
> very well defined goal (--> hide the fw_cfg dependency in both
> QemuLoadImageLib instances behind the synthetic filesystem); we can
> validate / review that regardless of the ultimate crypto / security
> goal. Using the SimpleFs / FILE protocol APIs is not trivial IMO, so
> it's possible that just the first wave will require a v2.
> 

OK, I'll try to follow this plan.

>>
>>
>>>
>>> And then, we could eliminate the dynamic callback registration, plus
>>> the separate SevFwCfgVerifier, SevHashFinderLib, and
>>> SevQemuLoadImageLib stuff.
>>>
>>> We'd only need one new lib class, with *statically linked* hooks for
>>> QemuKernelLoaderFsDxe, and two instances of this new class, a Null
>>> one, and an actual (SEV hash verifier) one. The latter instance would
>>> locate the hash values, calculate the fresh hashes, and perform the
>>> comparisons. Only the AmdSevX64 platform would use the non-Null
>>> instance of this library class.
>>
>> OK, I'll refactor to static linking with two BlobVerifierLib
>> imlementations.
>>
>>
>>>
>>> (NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
>>> resolutions to the Null instance would be required there too.)
>>
>> I'll need to learn how to build edk2 for Arm to test this.  Thanks for
>> the heads-up.
> 
> With regard to QemuKernelLoaderFsDxe specifically:
> 
>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc               -a AARCH64
>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc       -a ARM
>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc         -a AARCH64
>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc -a ARM
> 
> If you work on an x86_64 machine, you'll need cross-gcc and
> cross-binutils for this. I have the following packages installed on my
> laptop:
> 
>   binutils-aarch64-linux-gnu-2.31.1-3.el7.x86_64
>   binutils-arm-linux-gnu-2.31.1-3.el7.x86_64
>   cross-binutils-common-2.31.1-3.el7.noarch
> 
>   cross-gcc-common-9.2.1-3.el7.1.noarch
>   gcc-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
>   gcc-arm-linux-gnu-9.2.1-3.el7.1.x86_64
>   gcc-c++-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
>   gcc-c++-arm-linux-gnu-9.2.1-3.el7.1.x86_64
> 
> (I don't remember why I have the c++ cross-compiler installed.)
> 

Thanks for the details; I'll try it.

-Dov

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Laszlo Ersek]

On 06/06/21 15:21, Dov Murik wrote:
> 
> 
> On 04/06/2021 14:26, Laszlo Ersek wrote:
>> On 06/04/21 12:30, Dov Murik wrote:
>>
>>> So I argue to keep the existing approach with two separate areas:
>>> existing one for injected secrets, and new one for a table of approved
>>> hashes (filled by QEMU and updated as initial encrypted measured guest
>>> memory).
>>
>> OK.
>>
>>> If the issue is MEMFD space,
>>
>> Yes, that's it.
>>
>>> maybe we can do something like: use the
>>> existing secrets page (4KB) for two uses: first 3KB for secrets, and
>>> last 1KB for hashes.  If this is not enough, the hashes are even
>>> smaller than 1KB; and we can even publish only one hash - the hash of
>>> all 3 hashes (need to think about edge cases when there's no
>>> cmdline/initrd). But all these "solutions" feel a bit hacky for me and
>>> might complicate the code.
>>
>> All these PCDs come in pairs -- base and size. (IIRC.) If there's no
>> architectural requirement to keep these two kinds of info in different
>> pages (such as different page protections or whatever), then packing
>> them into a single page is something I'd like. The above 3K+1K
>> subdivision sounds OK to me.
>>
> 
> I'll go with 3KB secrets + 1KB hashes.
> 
> 
>>>
>>> I don't understand your suggestion: "I'd *really* like us to extend
>>> one of the existent structures. If necessary, introduce a new GUID,
>>> for a table that contains both previously injected data, and the new
>>> data."; does this mean to use a single MEMFD page for the injected
>>> secrets and the hashes?
>>
>> Yes, it's the same (say, 3K+1K) idea, just expressed differently. In one
>> case, you have two GUIDed structs in the (plaintext, not compressed)
>> reset vector in the pflash, and the base+size structures associated wth
>> those two separate GUIDs happen to identify distinct ranges of the same
>> MEMFD page. In the other case, you have just one GUIDed structure (with
>> base+size contents), and the page pointed-to by this base+size pair is
>> subdivided by *internal* structuring -- such as internal GUIDs and so
>> on. Whichever is simpler to implement in both QEMU and edk2; I just want
>> to avoid wasing a full page for three hashes.
>>
> 
> I'll go with the two GUIDed structures in the reset vector (which will
> point to distinct parts of a single 4KB page).
> 
> That actually means shortening the existing secrets MEMFD area from 4KB
> to 3KB. Is that OK?

I don't know how that area is used in practice; from my perspective,
shortening it to 3KB is OK.

> 
> 
> 
>>>
>>> Also, in general, I don't really understand the implications of
>>> running out of MEMFD place;
>>
>> Here's one implication of enlarging MEMFD. It pushes BS Code, BS Data,
>> Loader Code, Loader Data, perhaps some AcpiNVS and Reserved memory
>> allocations to higher addresses. Then when the kernel is loaded, its
>> load address may be higher too. I'm not worried about wasted guest
>> memory, but abut various silent assumptions as to where the kernel
>> "should be". For example, after one round of enlarging DXEFV, the
>> "crash" utility stopped opening guest memory dumps, because it couldn't
>> find a kernel signature in the (low) address range that it used to scan.
>> The fix wasn't too difficult (the range to scan could be specified on
>> the "crash" commadn line, and then my colleague Dave Anderson just
>> modified "crash"), but it was a *surprise*. I don't like those.
>>
>>> maybe you have other ideas around this (for example,
>>> can we make MEMFD bigger only for AmdSevX64 platform?).
>>
>> Yes, experimenting with a larger MEMFD in just the AmdSevX64 platform is
>> fine.
>>
> 
> But now I understand that failures can appear way later in userspace
> (the crash utility), so just testing that a modern AMD VM boots fine is
> not enough to get confidence here.

Indeed if you expect the same userspace to work seamlessly, there is a
risk.

Cheers
Laszlo

> 
> 
>> NB reordering various PCDs between each other, so that their relative
>> relationships (orders) change, is a *lot* more risky than just enlarging
>> existing areas. The code in OVMF tends not to rely on actual bases and
>> sizes, but it may very well rely on a particular BasePCD + SizePCD sum
>> not exceeding another particular BasePCD.
>>
> 
> Thanks for pointing this out. I'll avoid reordering.
> 
> 
>>>
>>>
>>>> - Modifying the QemuFwCfgLib class for this purpose is inappropriate.
>>>> Even if we do our own home-brewed verifier, none of it must go into
>>>> QemuFwCfgLib class. QemuFwCfgLib is for transport.
>>>>
>>>
>>> OK, we'll take the verifier out (as you suggested below - to a
>>> BlobVerifierLib with two implementations).
>>>
>>>
>>>> [Ard, please see this one question:]
>>>>
>>>> - A major complication for hashing all three of: kernel, initrd,
>>>> cmdline, is that the *fetching* of this triplet is split between two
>>>> places. (Well, it is split between *three* places in fact, but I'm
>>>> going to ignore LinuxInitrdDynamicShellCommand for now, because the
>>>> AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>>>
>>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>>>> the command line is fetched in (both) QemuLoadImageLib instances.
>>>> This requires that all these modules be littered with hashing as
>>>> well, which I find *really bad*. Even if we factor out the actual
>>>> logic, I strongly dislike having *just hooks* for hashing in multiple
>>>> modules.
>>>>
>>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>>>> don't expose kernel command line", 2020-03-05). If we first
>>>>
>>>> (a) reverted that commit, and
>>>>
>>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>>>> command line from the *synthetic filesystem* (rather than directly
>>>> from fw_cfg),
>>>>
>>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>>>
>>>> Ard -- what's your thought on this?
>>>>
>>>
>>> I understand there's agreement here, and that both this suggested
>>> change (use the synthetic filesystem) and my patch series (add hash
>>> verification) touch the same code areas.  How do you envision this
>>> process in the mailing list?  Seperate patch serieses with dependency?
>>> One long patch series with both changes?  What goes first?
>>
>> Good point. I do have a kind of patch order laid out in my mind, but I
>> didn't think of whether we should have the patches in one patch series,
>> or in two "waves".
>>
>> OK, let's go with two patch sets.
>>
>> In the first set, we should just focus on the above steps (a) and (b).
>> Step (a) shouldn't be too hard. In step (b), you'd modify both
>> QemuLoadImageLib instances (two separate patches), replacing the
>> QemuFwCfgLib APIs for fetching the cmdline with
>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
>>
>> Speaking from memory, the synthetic filesystem has a unique device path,
>> so the first step would be calling gBS->LocateDevicePath(), for finding
>> SimpleFs on the unique device path. Once you have the SimpleFs
>> interface, you can call OpenVolume, then open the "cmdline" file using
>> the EFI_FILE_PROTOCOL output by OpenVolume.
>>
>> Once we merge this series (basically just three patches), there is no
>> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
>> reckon. Then you can post the second wave, in which:
>>
>> - a new "firmware config verifier" library class is introduced,
>>
>> - two library instances for that class are introduced (null, and the
>>   real thing),
>>
>> - the AmdSevX64.dsc platform resolves the new lib class to the "real"
>>   (hashing) instance,
>>
>> - all other platform DSCs using QemuKernelLoaderFsDxe resolve the new
>>   lib class to the null instance,
>>
>> - QemuKernelLoaderFsDxe is extended with a dependency on the new class,
>>   calling the proper APIs to (a) initialize the verifier, and (b) verify
>>   every fw_cfg blob that is about to be exposed as a synthetic file.
>>
>> Then QemuLoadImageLib needs no changes, as it will not depend on fw_cfg,
>> and every synthetic file it may want to access will have been verified
>> by QemuKernelLoaderFsDxe already, according to the verifier lib instance
>> that's used in the respective platform DSC file.
>>
>> I would recommend only posting the first patch set initially. It has a
>> very well defined goal (--> hide the fw_cfg dependency in both
>> QemuLoadImageLib instances behind the synthetic filesystem); we can
>> validate / review that regardless of the ultimate crypto / security
>> goal. Using the SimpleFs / FILE protocol APIs is not trivial IMO, so
>> it's possible that just the first wave will require a v2.
>>
> 
> OK, I'll try to follow this plan.
> 
>>>
>>>
>>>>
>>>> And then, we could eliminate the dynamic callback registration, plus
>>>> the separate SevFwCfgVerifier, SevHashFinderLib, and
>>>> SevQemuLoadImageLib stuff.
>>>>
>>>> We'd only need one new lib class, with *statically linked* hooks for
>>>> QemuKernelLoaderFsDxe, and two instances of this new class, a Null
>>>> one, and an actual (SEV hash verifier) one. The latter instance would
>>>> locate the hash values, calculate the fresh hashes, and perform the
>>>> comparisons. Only the AmdSevX64 platform would use the non-Null
>>>> instance of this library class.
>>>
>>> OK, I'll refactor to static linking with two BlobVerifierLib
>>> imlementations.
>>>
>>>
>>>>
>>>> (NB QemuKernelLoaderFsDxe is used by some ArmVirtPkg platforms, so
>>>> resolutions to the Null instance would be required there too.)
>>>
>>> I'll need to learn how to build edk2 for Arm to test this.  Thanks for
>>> the heads-up.
>>
>> With regard to QemuKernelLoaderFsDxe specifically:
>>
>>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc               -a AARCH64
>>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc       -a ARM
>>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc         -a AARCH64
>>   build -b NOOPT -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc -a ARM
>>
>> If you work on an x86_64 machine, you'll need cross-gcc and
>> cross-binutils for this. I have the following packages installed on my
>> laptop:
>>
>>   binutils-aarch64-linux-gnu-2.31.1-3.el7.x86_64
>>   binutils-arm-linux-gnu-2.31.1-3.el7.x86_64
>>   cross-binutils-common-2.31.1-3.el7.noarch
>>
>>   cross-gcc-common-9.2.1-3.el7.1.noarch
>>   gcc-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
>>   gcc-arm-linux-gnu-9.2.1-3.el7.1.x86_64
>>   gcc-c++-aarch64-linux-gnu-9.2.1-3.el7.1.x86_64
>>   gcc-c++-arm-linux-gnu-9.2.1-3.el7.1.x86_64
>>
>> (I don't remember why I have the c++ cross-compiler installed.)
>>
> 
> Thanks for the details; I'll try it.
> 
> -Dov
> 

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Dov Murik]

On 04/06/2021 14:26, Laszlo Ersek wrote:
> On 06/04/21 12:30, Dov Murik wrote:
> 

...

>>
>>> [Ard, please see this one question:]
>>>
>>> - A major complication for hashing all three of: kernel, initrd,
>>> cmdline, is that the *fetching* of this triplet is split between two
>>> places. (Well, it is split between *three* places in fact, but I'm
>>> going to ignore LinuxInitrdDynamicShellCommand for now, because the
>>> AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>>
>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>>> the command line is fetched in (both) QemuLoadImageLib instances.
>>> This requires that all these modules be littered with hashing as
>>> well, which I find *really bad*. Even if we factor out the actual
>>> logic, I strongly dislike having *just hooks* for hashing in multiple
>>> modules.
>>>
>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>>> don't expose kernel command line", 2020-03-05). If we first
>>>
>>> (a) reverted that commit, and
>>>
>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>>> command line from the *synthetic filesystem* (rather than directly
>>> from fw_cfg),
>>>
>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>>
>>> Ard -- what's your thought on this?
>>>
>>
>> I understand there's agreement here, and that both this suggested
>> change (use the synthetic filesystem) and my patch series (add hash
>> verification) touch the same code areas.  How do you envision this
>> process in the mailing list?  Seperate patch serieses with dependency?
>> One long patch series with both changes?  What goes first?
> 
> Good point. I do have a kind of patch order laid out in my mind, but I
> didn't think of whether we should have the patches in one patch series,
> or in two "waves".
> 
> OK, let's go with two patch sets.
> 
> In the first set, we should just focus on the above steps (a) and (b).
> Step (a) shouldn't be too hard. In step (b), you'd modify both
> QemuLoadImageLib instances (two separate patches), replacing the
> QemuFwCfgLib APIs for fetching the cmdline with
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
> 
> Speaking from memory, the synthetic filesystem has a unique device path,
> so the first step would be calling gBS->LocateDevicePath(), for finding
> SimpleFs on the unique device path. Once you have the SimpleFs
> interface, you can call OpenVolume, then open the "cmdline" file using
> the EFI_FILE_PROTOCOL output by OpenVolume.
> 
> Once we merge this series (basically just three patches), there is no
> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
> reckon. 

I started working on that, and managed to remove all QemuFwCfg* calls in
the main path of QemuLoadKernelImage (so far working on
X86QemuLoadImageLib.c).  That works fine: I read the content of the
"cmdline" synthetic file, and I check the size of the synthetic "initrd"
file.  I used Library/FileHandleLib.h; I hope that's fine.

However, there's another path (which I don't reach with my test setup),
which is the call to QemuLoadLegacyImage, which has a lot of calls to
QemuFwCfg* in its body.

Am I expected to change that legacy path as well?
Or is it in a "it's working don't touch" state?
If I modify this, how do I test it?


Thanks for the guidance,

-Dov

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Laszlo Ersek]

Ard,

do you have any comments please, on the topic at the bottom?

My comments follow:

On 06/08/21 11:57, Dov Murik wrote:
>
>
> On 04/06/2021 14:26, Laszlo Ersek wrote:
>> On 06/04/21 12:30, Dov Murik wrote:
>>
>
> ...
>
>>>
>>>> [Ard, please see this one question:]
>>>>
>>>> - A major complication for hashing all three of: kernel, initrd,
>>>> cmdline, is that the *fetching* of this triplet is split between
>>>> two places. (Well, it is split between *three* places in fact, but
>>>> I'm going to ignore LinuxInitrdDynamicShellCommand for now, because
>>>> the AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>>>
>>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>>>> the command line is fetched in (both) QemuLoadImageLib instances.
>>>> This requires that all these modules be littered with hashing as
>>>> well, which I find *really bad*. Even if we factor out the actual
>>>> logic, I strongly dislike having *just hooks* for hashing in
>>>> multiple modules.
>>>>
>>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>>>> don't expose kernel command line", 2020-03-05). If we first
>>>>
>>>> (a) reverted that commit, and
>>>>
>>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>>>> command line from the *synthetic filesystem* (rather than directly
>>>> from fw_cfg),
>>>>
>>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>>>
>>>> Ard -- what's your thought on this?
>>>>
>>>
>>> I understand there's agreement here, and that both this suggested
>>> change (use the synthetic filesystem) and my patch series (add hash
>>> verification) touch the same code areas.  How do you envision this
>>> process in the mailing list?  Seperate patch serieses with
>>> dependency? One long patch series with both changes?  What goes
>>> first?
>>
>> Good point. I do have a kind of patch order laid out in my mind, but
>> I didn't think of whether we should have the patches in one patch
>> series, or in two "waves".
>>
>> OK, let's go with two patch sets.
>>
>> In the first set, we should just focus on the above steps (a) and
>> (b). Step (a) shouldn't be too hard. In step (b), you'd modify both
>> QemuLoadImageLib instances (two separate patches), replacing the
>> QemuFwCfgLib APIs for fetching the cmdline with
>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
>>
>> Speaking from memory, the synthetic filesystem has a unique device
>> path, so the first step would be calling gBS->LocateDevicePath(), for
>> finding SimpleFs on the unique device path. Once you have the
>> SimpleFs interface, you can call OpenVolume, then open the "cmdline"
>> file using the EFI_FILE_PROTOCOL output by OpenVolume.
>>
>> Once we merge this series (basically just three patches), there is no
>> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
>> reckon.
>
> I started working on that, and managed to remove all QemuFwCfg* calls
> in the main path of QemuLoadKernelImage (so far working on
> X86QemuLoadImageLib.c).  That works fine: I read the content of the
> "cmdline" synthetic file, and I check the size of the synthetic
> "initrd" file.  I used Library/FileHandleLib.h; I hope that's fine.

The lib class header says "Provides interface to EFI_FILE_HANDLE
functionality", which is not too bad; but I don't immediately see what
those APIs save us -- the APIs that I believe to be relevant to this use
case all seem to be thin wrappers around EFI_FILE_PROTOCOL. (The only
instance of FileHandleLib is
"MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
necessarily a problem, it doesn't seem an obvious win (unless it saves
you much complexity and/or code in a way that I'm missing).

In OVMF, the following executables use UefiFileHandleLib at the moment:

- MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
- ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
- ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
- OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
- ShellPkg/Application/Shell/Shell.inf

The last four are shell-related, so "prior art" is really just BdsDxe...

> However, there's another path (which I don't reach with my test
> setup), which is the call to QemuLoadLegacyImage, which has a lot of
> calls to QemuFwCfg* in its body.
>
> Am I expected to change that legacy path as well?
> Or is it in a "it's working don't touch" state?
> If I modify this, how do I test it?

The use case that you foresee for this feature is really important here.

When you say that you don't reach QemuLoadLegacyImage(), that means your
guest kernel is built with the UEFI stub.

(1) If you can make the Linux UEFI stub a *required* part of the use
case, then:

(1a) switch the QemuLoadImageLib class resolution from
"OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
"OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" in
"OvmfPkg/AmdSev/AmdSevX64.dsc",

(1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
modifications thus far should be easy to transplant to this lib
instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
QemuLoadLegacyImage() in GenericQemuLoadImageLib.

This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" does
not offer Secure Boot support, so there's not going to be a case when
gBS->LoadImage() rejects the UEFI stubbed Linux binary due to Secure
Boot verification failing (EFI_SECURITY_VIOLATION, EFI_ACCESS_DENIED).


(2) If you cannot make the Linux UEFI stub a required part of the use
case, then X86QemuLoadImageLib needs to be modified indeed.

(2a) Unfortunately, in this case we have to add a hack, because the
synthetic filesystem only exposes the concatenated "setup data + kernel
image" blob. The following would have to be preserved (as the only
remaining QemuFwCfgLib access):

  QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
  SetupSize = (UINTN)QemuFwCfgRead32 ();

(2b) and then the kernel blob from the synthetic fs would have to be
split in two (= setup, kernel), within QemuLoadLegacyImage().


I'm sorry for missing this aspect previously! I really hope we can go
with (1)!

Thanks,
Laszlo

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Dov Murik]

On 08/06/2021 13:59, Laszlo Ersek wrote:
> Ard,
> 
> do you have any comments please, on the topic at the bottom?
> 
> My comments follow:
> 
> On 06/08/21 11:57, Dov Murik wrote:
>>
>>
>> On 04/06/2021 14:26, Laszlo Ersek wrote:
>>> On 06/04/21 12:30, Dov Murik wrote:
>>>
>>
>> ...
>>
>>>>
>>>>> [Ard, please see this one question:]
>>>>>
>>>>> - A major complication for hashing all three of: kernel, initrd,
>>>>> cmdline, is that the *fetching* of this triplet is split between
>>>>> two places. (Well, it is split between *three* places in fact, but
>>>>> I'm going to ignore LinuxInitrdDynamicShellCommand for now, because
>>>>> the AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>>>>
>>>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>>>>> the command line is fetched in (both) QemuLoadImageLib instances.
>>>>> This requires that all these modules be littered with hashing as
>>>>> well, which I find *really bad*. Even if we factor out the actual
>>>>> logic, I strongly dislike having *just hooks* for hashing in
>>>>> multiple modules.
>>>>>
>>>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>>>>> don't expose kernel command line", 2020-03-05). If we first
>>>>>
>>>>> (a) reverted that commit, and
>>>>>
>>>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>>>>> command line from the *synthetic filesystem* (rather than directly
>>>>> from fw_cfg),
>>>>>
>>>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>>>>
>>>>> Ard -- what's your thought on this?
>>>>>
>>>>
>>>> I understand there's agreement here, and that both this suggested
>>>> change (use the synthetic filesystem) and my patch series (add hash
>>>> verification) touch the same code areas.  How do you envision this
>>>> process in the mailing list?  Seperate patch serieses with
>>>> dependency? One long patch series with both changes?  What goes
>>>> first?
>>>
>>> Good point. I do have a kind of patch order laid out in my mind, but
>>> I didn't think of whether we should have the patches in one patch
>>> series, or in two "waves".
>>>
>>> OK, let's go with two patch sets.
>>>
>>> In the first set, we should just focus on the above steps (a) and
>>> (b). Step (a) shouldn't be too hard. In step (b), you'd modify both
>>> QemuLoadImageLib instances (two separate patches), replacing the
>>> QemuFwCfgLib APIs for fetching the cmdline with
>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
>>>
>>> Speaking from memory, the synthetic filesystem has a unique device
>>> path, so the first step would be calling gBS->LocateDevicePath(), for
>>> finding SimpleFs on the unique device path. Once you have the
>>> SimpleFs interface, you can call OpenVolume, then open the "cmdline"
>>> file using the EFI_FILE_PROTOCOL output by OpenVolume.
>>>
>>> Once we merge this series (basically just three patches), there is no
>>> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
>>> reckon.
>>
>> I started working on that, and managed to remove all QemuFwCfg* calls
>> in the main path of QemuLoadKernelImage (so far working on
>> X86QemuLoadImageLib.c).  That works fine: I read the content of the
>> "cmdline" synthetic file, and I check the size of the synthetic
>> "initrd" file.  I used Library/FileHandleLib.h; I hope that's fine.
> 
> The lib class header says "Provides interface to EFI_FILE_HANDLE
> functionality", which is not too bad; but I don't immediately see what
> those APIs save us -- the APIs that I believe to be relevant to this use
> case all seem to be thin wrappers around EFI_FILE_PROTOCOL. (The only
> instance of FileHandleLib is
> "MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
> necessarily a problem, it doesn't seem an obvious win (unless it saves
> you much complexity and/or code in a way that I'm missing).


Using FileHandleGetSize() saves some handling of a EFI_FILE_INFO pointer
and freeing it etc (for getting the size of "cmdline" and "initrd").
But maybe it's better not to add another dependency.


> 
> In OVMF, the following executables use UefiFileHandleLib at the moment:
> 
> - MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> - ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> - ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
> - OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
> - ShellPkg/Application/Shell/Shell.inf
> 
> The last four are shell-related, so "prior art" is really just BdsDxe...
> 
>> However, there's another path (which I don't reach with my test
>> setup), which is the call to QemuLoadLegacyImage, which has a lot of
>> calls to QemuFwCfg* in its body.
>>
>> Am I expected to change that legacy path as well?
>> Or is it in a "it's working don't touch" state?
>> If I modify this, how do I test it?
> 
> The use case that you foresee for this feature is really important here.
> 
> When you say that you don't reach QemuLoadLegacyImage(), that means your
> guest kernel is built with the UEFI stub.
> 
> (1) If you can make the Linux UEFI stub a *required* part of the use
> case, then:
> 
> (1a) switch the QemuLoadImageLib class resolution from
> "OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
> "OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" in
> "OvmfPkg/AmdSev/AmdSevX64.dsc",
> 
> (1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
> modifications thus far should be easy to transplant to this lib
> instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
> QemuLoadLegacyImage() in GenericQemuLoadImageLib.
> 
> This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" does
> not offer Secure Boot support, so there's not going to be a case when
> gBS->LoadImage() rejects the UEFI stubbed Linux binary due to Secure
> Boot verification failing (EFI_SECURITY_VIOLATION, EFI_ACCESS_DENIED).
> 
> 
> (2) If you cannot make the Linux UEFI stub a required part of the use
> case, then X86QemuLoadImageLib needs to be modified indeed.
> 
> (2a) Unfortunately, in this case we have to add a hack, because the
> synthetic filesystem only exposes the concatenated "setup data + kernel
> image" blob. The following would have to be preserved (as the only
> remaining QemuFwCfgLib access):
> 
>   QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
>   SetupSize = (UINTN)QemuFwCfgRead32 ();
> 
> (2b) and then the kernel blob from the synthetic fs would have to be
> split in two (= setup, kernel), within QemuLoadLegacyImage().
> 
> 
> I'm sorry for missing this aspect previously! I really hope we can go
> with (1)!
> 

I'll check.

But if we go with (1) -- do you (and Ard) prefer:

(a) leave X86QemuLoadImageLib as it is in master;

-or-

(b) modify X86QemuLoadImageLib the "main" path to use the
QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
with QemuFwCfg

?


Or, in other words, is the refactoring to read the cmdline from
QemuKernelLoaderFs (across both QemuLoadImageLib implementations)
beneficial even if we don't add the verification "hooks"?


-Dov

> Thanks,
> Laszlo
> 

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Laszlo Ersek]

On 06/08/21 14:09, Dov Murik wrote:
> On 08/06/2021 13:59, Laszlo Ersek wrote:
>> On 06/08/21 11:57, Dov Murik wrote:

>>> I started working on that, and managed to remove all QemuFwCfg*
>>> calls in the main path of QemuLoadKernelImage (so far working on
>>> X86QemuLoadImageLib.c).  That works fine: I read the content of the
>>> "cmdline" synthetic file, and I check the size of the synthetic
>>> "initrd" file.  I used Library/FileHandleLib.h; I hope that's fine.
>>
>> The lib class header says "Provides interface to EFI_FILE_HANDLE
>> functionality", which is not too bad; but I don't immediately see
>> what those APIs save us -- the APIs that I believe to be relevant to
>> this use case all seem to be thin wrappers around EFI_FILE_PROTOCOL.
>> (The only instance of FileHandleLib is
>> "MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
>> necessarily a problem, it doesn't seem an obvious win (unless it
>> saves you much complexity and/or code in a way that I'm missing).
>
>
> Using FileHandleGetSize() saves some handling of a EFI_FILE_INFO
> pointer and freeing it etc (for getting the size of "cmdline" and
> "initrd"). But maybe it's better not to add another dependency.

Hmm, no; you have convinced me. Please go ahead with FileHandleLib.
Thank you for taking the time to talk this through with me.

>>> Am I expected to change that legacy path as well?
>>> Or is it in a "it's working don't touch" state?
>>> If I modify this, how do I test it?
>>
>> The use case that you foresee for this feature is really important
>> here.
>>
>> When you say that you don't reach QemuLoadLegacyImage(), that means
>> your guest kernel is built with the UEFI stub.
>>
>> (1) If you can make the Linux UEFI stub a *required* part of the use
>> case, then:
>>
>> (1a) switch the QemuLoadImageLib class resolution from
>> "OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
>> "OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf"
>> in "OvmfPkg/AmdSev/AmdSevX64.dsc",
>>
>> (1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
>> modifications thus far should be easy to transplant to this lib
>> instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
>> QemuLoadLegacyImage() in GenericQemuLoadImageLib.
>>
>> This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc"
>> does not offer Secure Boot support, so there's not going to be a case
>> when gBS->LoadImage() rejects the UEFI stubbed Linux binary due to
>> Secure Boot verification failing (EFI_SECURITY_VIOLATION,
>> EFI_ACCESS_DENIED).
>>
>>
>> (2) If you cannot make the Linux UEFI stub a required part of the use
>> case, then X86QemuLoadImageLib needs to be modified indeed.
>>
>> (2a) Unfortunately, in this case we have to add a hack, because the
>> synthetic filesystem only exposes the concatenated "setup data +
>> kernel image" blob. The following would have to be preserved (as the
>> only remaining QemuFwCfgLib access):
>>
>>   QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
>>   SetupSize = (UINTN)QemuFwCfgRead32 ();
>>
>> (2b) and then the kernel blob from the synthetic fs would have to be
>> split in two (= setup, kernel), within QemuLoadLegacyImage().
>>
>>
>> I'm sorry for missing this aspect previously! I really hope we can go
>> with (1)!
>>
>
> I'll check.
>
> But if we go with (1) -- do you (and Ard) prefer:
>
> (a) leave X86QemuLoadImageLib as it is in master;
>
> -or-
>
> (b) modify X86QemuLoadImageLib the "main" path to use the
> QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
> with QemuFwCfg
>
> ?

I prefer option (a), with the extension that we need to update the
following file-top comment in the files under
"OvmfPkg/Library/X86QemuLoadImageLib":

  X86 specific implementation of QemuLoadImageLib library class interface
  with support for loading mixed mode images and non-EFI stub images

We should add a warning there that this library instance (a) depends on
fw_cfg directly, and (b) is therefore unsuitable for blob verification
purposes.

> Or, in other words, is the refactoring to read the cmdline from
> QemuKernelLoaderFs (across both QemuLoadImageLib implementations)
> beneficial even if we don't add the verification "hooks"?

My answer would be "no". IMO, the refactoring is only useful for
unifying the blob verifier in a single layer (the synthetic fs). Without
the blob verifier in the picture, I see nothing wrong with fetching
different fw_cfg blobs in different modules. The kernel command line
never needed to be available through the synthetic FS, for any
particular consumer, so removing that synthetic file was fine.

Now, we do have an internal consumer of sorts (the blob verifier). Given
that we don't want to add a whole new extra layer for it, and we also
don't want to scatter it over multiple modules, integrating it into the
synthetic FS make sense, hopefully.

I initially thought that the refactoring would benefit both
QemuLoadImageLib instances, but you've shown that I missed the
complications in the legacy path of X86QemuLoadImageLib. Namely, that
the synthetic filesystem abstraction (= the fused setup data + kernel
image blob) isn't a good match for the legacy path.

Library instances are permitted to have different features and different
limitations.

(If we *really* wanted to do refactor the legacy path cleanly, we could
further extend the QemuKernelLoaderFsDxe driver, to present -- under new
filenames -- the setup data blob isolation, and the bare kernel image
blob in isolation. But this would be a lot of wasted work, in practice,
provided that your use case requires a UEFI stub on the guest kernel at
all times (= the legacy path would never be taken). Also, let's not
forget, if we exposed such *new* synthetic files, the blob verifier
would have to verify those isolated blobs too (you'd have to provide
hashes for them from the outside); otherwise the whole exercise would be
moot, I think.)

Thanks,
Laszlo

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Dov Murik]

On 08/06/2021 18:59, Laszlo Ersek wrote:
> On 06/08/21 14:09, Dov Murik wrote:
>> On 08/06/2021 13:59, Laszlo Ersek wrote:
>>> On 06/08/21 11:57, Dov Murik wrote:
> 

>>
>> But if we go with (1) -- do you (and Ard) prefer:
>>
>> (a) leave X86QemuLoadImageLib as it is in master;
>>
>> -or-
>>
>> (b) modify X86QemuLoadImageLib the "main" path to use the
>> QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
>> with QemuFwCfg
>>
>> ?
> 
> I prefer option (a), with the extension that we need to update the
> following file-top comment in the files under
> "OvmfPkg/Library/X86QemuLoadImageLib":
> 
>   X86 specific implementation of QemuLoadImageLib library class interface
>   with support for loading mixed mode images and non-EFI stub images
> 

First attempt at this is submitted to the mailing list:
https://edk2.groups.io/g/devel/message/76265


> We should add a warning there that this library instance (a) depends on
> fw_cfg directly, and (b) is therefore unsuitable for blob verification
> purposes.

I'll add the warning (b) when I add the blob verification feature.

-Dov

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Laszlo Ersek]

On 06/09/21 14:25, Dov Murik wrote:
> 
> 
> On 08/06/2021 18:59, Laszlo Ersek wrote:
>> On 06/08/21 14:09, Dov Murik wrote:
>>> On 08/06/2021 13:59, Laszlo Ersek wrote:
>>>> On 06/08/21 11:57, Dov Murik wrote:
>>
> 
>>>
>>> But if we go with (1) -- do you (and Ard) prefer:
>>>
>>> (a) leave X86QemuLoadImageLib as it is in master;
>>>
>>> -or-
>>>
>>> (b) modify X86QemuLoadImageLib the "main" path to use the
>>> QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
>>> with QemuFwCfg
>>>
>>> ?
>>
>> I prefer option (a), with the extension that we need to update the
>> following file-top comment in the files under
>> "OvmfPkg/Library/X86QemuLoadImageLib":
>>
>>   X86 specific implementation of QemuLoadImageLib library class interface
>>   with support for loading mixed mode images and non-EFI stub images
>>
> 
> First attempt at this is submitted to the mailing list:
> https://edk2.groups.io/g/devel/message/76265
> 
> 
>> We should add a warning there that this library instance (a) depends on
>> fw_cfg directly, and (b) is therefore unsuitable for blob verification
>> purposes.
> 
> I'll add the warning (b) when I add the blob verification feature.

That makes sense to me, thanks.
Laszlo

回复: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[gaoliming]

Dov:
  Can you submit one BZ for this new feature? I will add it into edk2 202108 stable tag planning. 

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Laszlo Ersek
> 发送时间: 2021年6月9日 21:54
> 收件人: Dov Murik <dovmurik@linux.ibm.com>; devel@edk2.groups.io; Ard
> Biesheuvel <ardb+tianocore@kernel.org>
> 抄送: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin
> Feldman-Fitzthum <tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>;
> James Bottomley <jejb@linux.ibm.com>; Hubertus Franke
> <frankeh@us.ibm.com>; Jordan Justen <jordan.l.justen@intel.com>; Ashish
> Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Erdem Aktas <erdemaktas@google.com>; Jiewen Yao
> <jiewen.yao@intel.com>; Min Xu <min.m.xu@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> 主题: Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with
> kernel/initrd/cmdline
> 
> On 06/09/21 14:25, Dov Murik wrote:
> >
> >
> > On 08/06/2021 18:59, Laszlo Ersek wrote:
> >> On 06/08/21 14:09, Dov Murik wrote:
> >>> On 08/06/2021 13:59, Laszlo Ersek wrote:
> >>>> On 06/08/21 11:57, Dov Murik wrote:
> >>
> >
> >>>
> >>> But if we go with (1) -- do you (and Ard) prefer:
> >>>
> >>> (a) leave X86QemuLoadImageLib as it is in master;
> >>>
> >>> -or-
> >>>
> >>> (b) modify X86QemuLoadImageLib the "main" path to use the
> >>> QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
> >>> with QemuFwCfg
> >>>
> >>> ?
> >>
> >> I prefer option (a), with the extension that we need to update the
> >> following file-top comment in the files under
> >> "OvmfPkg/Library/X86QemuLoadImageLib":
> >>
> >>   X86 specific implementation of QemuLoadImageLib library class
> interface
> >>   with support for loading mixed mode images and non-EFI stub images
> >>
> >
> > First attempt at this is submitted to the mailing list:
> > https://edk2.groups.io/g/devel/message/76265
> >
> >
> >> We should add a warning there that this library instance (a) depends on
> >> fw_cfg directly, and (b) is therefore unsuitable for blob verification
> >> purposes.
> >
> > I'll add the warning (b) when I add the blob verification feature.
> 
> That makes sense to me, thanks.
> Laszlo
> 
> 
> 
> 
> 

Re: 回复: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Dov Murik]

On 10/06/2021 12:15, gaoliming wrote:
> Dov:
>   Can you submit one BZ for this new feature? I will add it into edk2 202108 stable tag planning. 

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

I'll add the BZ link to future versions of the patch series.

Thanks,
-Dov

> 
> Thanks
> Liming
>> -----邮件原件-----
>> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Laszlo Ersek
>> 发送时间: 2021年6月9日 21:54
>> 收件人: Dov Murik <dovmurik@linux.ibm.com>; devel@edk2.groups.io; Ard
>> Biesheuvel <ardb+tianocore@kernel.org>
>> 抄送: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin
>> Feldman-Fitzthum <tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>;
>> James Bottomley <jejb@linux.ibm.com>; Hubertus Franke
>> <frankeh@us.ibm.com>; Jordan Justen <jordan.l.justen@intel.com>; Ashish
>> Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
>> Erdem Aktas <erdemaktas@google.com>; Jiewen Yao
>> <jiewen.yao@intel.com>; Min Xu <min.m.xu@intel.com>; Tom Lendacky
>> <thomas.lendacky@amd.com>
>> 主题: Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with
>> kernel/initrd/cmdline
>>
>> On 06/09/21 14:25, Dov Murik wrote:
>>>
>>>
>>> On 08/06/2021 18:59, Laszlo Ersek wrote:
>>>> On 06/08/21 14:09, Dov Murik wrote:
>>>>> On 08/06/2021 13:59, Laszlo Ersek wrote:
>>>>>> On 06/08/21 11:57, Dov Murik wrote:
>>>>
>>>
>>>>>
>>>>> But if we go with (1) -- do you (and Ard) prefer:
>>>>>
>>>>> (a) leave X86QemuLoadImageLib as it is in master;
>>>>>
>>>>> -or-
>>>>>
>>>>> (b) modify X86QemuLoadImageLib the "main" path to use the
>>>>> QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
>>>>> with QemuFwCfg
>>>>>
>>>>> ?
>>>>
>>>> I prefer option (a), with the extension that we need to update the
>>>> following file-top comment in the files under
>>>> "OvmfPkg/Library/X86QemuLoadImageLib":
>>>>
>>>>   X86 specific implementation of QemuLoadImageLib library class
>> interface
>>>>   with support for loading mixed mode images and non-EFI stub images
>>>>
>>>
>>> First attempt at this is submitted to the mailing list:
>>> https://edk2.groups.io/g/devel/message/76265
>>>
>>>
>>>> We should add a warning there that this library instance (a) depends on
>>>> fw_cfg directly, and (b) is therefore unsuitable for blob verification
>>>> purposes.
>>>
>>> I'll add the warning (b) when I add the blob verification feature.
>>
>> That makes sense to me, thanks.
>> Laszlo
>>
>>
>>
>> 
>>
> 
> 
> 

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Ard Biesheuvel]

On Tue, 8 Jun 2021 at 12:59, Laszlo Ersek <lersek@redhat.com> wrote:
>
> Ard,
>
> do you have any comments please, on the topic at the bottom?
>
> My comments follow:
>
> On 06/08/21 11:57, Dov Murik wrote:
> >
> >
> > On 04/06/2021 14:26, Laszlo Ersek wrote:
> >> On 06/04/21 12:30, Dov Murik wrote:
> >>
> >
> > ...
> >
> >>>
> >>>> [Ard, please see this one question:]
> >>>>
> >>>> - A major complication for hashing all three of: kernel, initrd,
> >>>> cmdline, is that the *fetching* of this triplet is split between
> >>>> two places. (Well, it is split between *three* places in fact, but
> >>>> I'm going to ignore LinuxInitrdDynamicShellCommand for now, because
> >>>> the AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
> >>>>
> >>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
> >>>> the command line is fetched in (both) QemuLoadImageLib instances.
> >>>> This requires that all these modules be littered with hashing as
> >>>> well, which I find *really bad*. Even if we factor out the actual
> >>>> logic, I strongly dislike having *just hooks* for hashing in
> >>>> multiple modules.
> >>>>
> >>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
> >>>> don't expose kernel command line", 2020-03-05). If we first
> >>>>
> >>>> (a) reverted that commit, and
> >>>>
> >>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
> >>>> command line from the *synthetic filesystem* (rather than directly
> >>>> from fw_cfg),
> >>>>
> >>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
> >>>>
> >>>> Ard -- what's your thought on this?
> >>>>
> >>>
> >>> I understand there's agreement here, and that both this suggested
> >>> change (use the synthetic filesystem) and my patch series (add hash
> >>> verification) touch the same code areas.  How do you envision this
> >>> process in the mailing list?  Seperate patch serieses with
> >>> dependency? One long patch series with both changes?  What goes
> >>> first?
> >>
> >> Good point. I do have a kind of patch order laid out in my mind, but
> >> I didn't think of whether we should have the patches in one patch
> >> series, or in two "waves".
> >>
> >> OK, let's go with two patch sets.
> >>
> >> In the first set, we should just focus on the above steps (a) and
> >> (b). Step (a) shouldn't be too hard. In step (b), you'd modify both
> >> QemuLoadImageLib instances (two separate patches), replacing the
> >> QemuFwCfgLib APIs for fetching the cmdline with
> >> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
> >>
> >> Speaking from memory, the synthetic filesystem has a unique device
> >> path, so the first step would be calling gBS->LocateDevicePath(), for
> >> finding SimpleFs on the unique device path. Once you have the
> >> SimpleFs interface, you can call OpenVolume, then open the "cmdline"
> >> file using the EFI_FILE_PROTOCOL output by OpenVolume.
> >>
> >> Once we merge this series (basically just three patches), there is no
> >> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
> >> reckon.
> >
> > I started working on that, and managed to remove all QemuFwCfg* calls
> > in the main path of QemuLoadKernelImage (so far working on
> > X86QemuLoadImageLib.c).  That works fine: I read the content of the
> > "cmdline" synthetic file, and I check the size of the synthetic
> > "initrd" file.  I used Library/FileHandleLib.h; I hope that's fine.
>
> The lib class header says "Provides interface to EFI_FILE_HANDLE
> functionality", which is not too bad; but I don't immediately see what
> those APIs save us -- the APIs that I believe to be relevant to this use
> case all seem to be thin wrappers around EFI_FILE_PROTOCOL. (The only
> instance of FileHandleLib is
> "MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
> necessarily a problem, it doesn't seem an obvious win (unless it saves
> you much complexity and/or code in a way that I'm missing).
>
> In OVMF, the following executables use UefiFileHandleLib at the moment:
>
> - MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> - ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> - ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
> - OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
> - ShellPkg/Application/Shell/Shell.inf
>
> The last four are shell-related, so "prior art" is really just BdsDxe...
>
> > However, there's another path (which I don't reach with my test
> > setup), which is the call to QemuLoadLegacyImage, which has a lot of
> > calls to QemuFwCfg* in its body.
> >
> > Am I expected to change that legacy path as well?
> > Or is it in a "it's working don't touch" state?
> > If I modify this, how do I test it?
>
> The use case that you foresee for this feature is really important here.
>
> When you say that you don't reach QemuLoadLegacyImage(), that means your
> guest kernel is built with the UEFI stub.
>
> (1) If you can make the Linux UEFI stub a *required* part of the use
> case, then:
>
> (1a) switch the QemuLoadImageLib class resolution from
> "OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
> "OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" in
> "OvmfPkg/AmdSev/AmdSevX64.dsc",
>
> (1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
> modifications thus far should be easy to transplant to this lib
> instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
> QemuLoadLegacyImage() in GenericQemuLoadImageLib.
>
> This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" does
> not offer Secure Boot support, so there's not going to be a case when
> gBS->LoadImage() rejects the UEFI stubbed Linux binary due to Secure
> Boot verification failing (EFI_SECURITY_VIOLATION, EFI_ACCESS_DENIED).
>
>
> (2) If you cannot make the Linux UEFI stub a required part of the use
> case, then X86QemuLoadImageLib needs to be modified indeed.
>
> (2a) Unfortunately, in this case we have to add a hack, because the
> synthetic filesystem only exposes the concatenated "setup data + kernel
> image" blob. The following would have to be preserved (as the only
> remaining QemuFwCfgLib access):
>
>   QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
>   SetupSize = (UINTN)QemuFwCfgRead32 ();
>
> (2b) and then the kernel blob from the synthetic fs would have to be
> split in two (= setup, kernel), within QemuLoadLegacyImage().
>
>
> I'm sorry for missing this aspect previously! I really hope we can go
> with (1)!
>

The legacy x86 loader should only be kept if there is a strong need to
do so. Keeping it around for some theoretical compatibility concern is
simply not worth it, IMHO.

Having two boot paths to reason about in terms of security is not the
only problem: another concern is that the legacy fallback path is
strictly x86, and is tightly coupled with the kernel's struct
boot_params, which is only documented by the .h file that happens to
declare it (and some outdated prose under Documentation/ perhaps)

Also, the EFI stub does some non-trivial work at boot, and having this
uniform between architectures is an important goal, especially for
non-x86 folks like me. We have introduced an initrd loader mechanism
that is fully arch agnostic, and there are patches on the list to move
the measurement of the initrd into the EFI stub if it was loaded using
this mechanism.

In summary, please stick with the generic loader unless you *really* can't.

-- 
Ard.

Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

[Laszlo Ersek]

On 06/08/21 14:49, Ard Biesheuvel wrote:
> On Tue, 8 Jun 2021 at 12:59, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> Ard,
>>
>> do you have any comments please, on the topic at the bottom?
>>
>> My comments follow:
>>
>> On 06/08/21 11:57, Dov Murik wrote:
>>>
>>>
>>> On 04/06/2021 14:26, Laszlo Ersek wrote:
>>>> On 06/04/21 12:30, Dov Murik wrote:
>>>>
>>>
>>> ...
>>>
>>>>>
>>>>>> [Ard, please see this one question:]
>>>>>>
>>>>>> - A major complication for hashing all three of: kernel, initrd,
>>>>>> cmdline, is that the *fetching* of this triplet is split between
>>>>>> two places. (Well, it is split between *three* places in fact, but
>>>>>> I'm going to ignore LinuxInitrdDynamicShellCommand for now, because
>>>>>> the AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)
>>>>>>
>>>>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
>>>>>> the command line is fetched in (both) QemuLoadImageLib instances.
>>>>>> This requires that all these modules be littered with hashing as
>>>>>> well, which I find *really bad*. Even if we factor out the actual
>>>>>> logic, I strongly dislike having *just hooks* for hashing in
>>>>>> multiple modules.
>>>>>>
>>>>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
>>>>>> don't expose kernel command line", 2020-03-05). If we first
>>>>>>
>>>>>> (a) reverted that commit, and
>>>>>>
>>>>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel
>>>>>> command line from the *synthetic filesystem* (rather than directly
>>>>>> from fw_cfg),
>>>>>>
>>>>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe.
>>>>>>
>>>>>> Ard -- what's your thought on this?
>>>>>>
>>>>>
>>>>> I understand there's agreement here, and that both this suggested
>>>>> change (use the synthetic filesystem) and my patch series (add hash
>>>>> verification) touch the same code areas.  How do you envision this
>>>>> process in the mailing list?  Seperate patch serieses with
>>>>> dependency? One long patch series with both changes?  What goes
>>>>> first?
>>>>
>>>> Good point. I do have a kind of patch order laid out in my mind, but
>>>> I didn't think of whether we should have the patches in one patch
>>>> series, or in two "waves".
>>>>
>>>> OK, let's go with two patch sets.
>>>>
>>>> In the first set, we should just focus on the above steps (a) and
>>>> (b). Step (a) shouldn't be too hard. In step (b), you'd modify both
>>>> QemuLoadImageLib instances (two separate patches), replacing the
>>>> QemuFwCfgLib APIs for fetching the cmdline with
>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.
>>>>
>>>> Speaking from memory, the synthetic filesystem has a unique device
>>>> path, so the first step would be calling gBS->LocateDevicePath(), for
>>>> finding SimpleFs on the unique device path. Once you have the
>>>> SimpleFs interface, you can call OpenVolume, then open the "cmdline"
>>>> file using the EFI_FILE_PROTOCOL output by OpenVolume.
>>>>
>>>> Once we merge this series (basically just three patches), there is no
>>>> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
>>>> reckon.
>>>
>>> I started working on that, and managed to remove all QemuFwCfg* calls
>>> in the main path of QemuLoadKernelImage (so far working on
>>> X86QemuLoadImageLib.c).  That works fine: I read the content of the
>>> "cmdline" synthetic file, and I check the size of the synthetic
>>> "initrd" file.  I used Library/FileHandleLib.h; I hope that's fine.
>>
>> The lib class header says "Provides interface to EFI_FILE_HANDLE
>> functionality", which is not too bad; but I don't immediately see what
>> those APIs save us -- the APIs that I believe to be relevant to this use
>> case all seem to be thin wrappers around EFI_FILE_PROTOCOL. (The only
>> instance of FileHandleLib is
>> "MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
>> necessarily a problem, it doesn't seem an obvious win (unless it saves
>> you much complexity and/or code in a way that I'm missing).
>>
>> In OVMF, the following executables use UefiFileHandleLib at the moment:
>>
>> - MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
>> - ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
>> - ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
>> - OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
>> - ShellPkg/Application/Shell/Shell.inf
>>
>> The last four are shell-related, so "prior art" is really just BdsDxe...
>>
>>> However, there's another path (which I don't reach with my test
>>> setup), which is the call to QemuLoadLegacyImage, which has a lot of
>>> calls to QemuFwCfg* in its body.
>>>
>>> Am I expected to change that legacy path as well?
>>> Or is it in a "it's working don't touch" state?
>>> If I modify this, how do I test it?
>>
>> The use case that you foresee for this feature is really important here.
>>
>> When you say that you don't reach QemuLoadLegacyImage(), that means your
>> guest kernel is built with the UEFI stub.
>>
>> (1) If you can make the Linux UEFI stub a *required* part of the use
>> case, then:
>>
>> (1a) switch the QemuLoadImageLib class resolution from
>> "OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
>> "OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" in
>> "OvmfPkg/AmdSev/AmdSevX64.dsc",
>>
>> (1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
>> modifications thus far should be easy to transplant to this lib
>> instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
>> QemuLoadLegacyImage() in GenericQemuLoadImageLib.
>>
>> This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" does
>> not offer Secure Boot support, so there's not going to be a case when
>> gBS->LoadImage() rejects the UEFI stubbed Linux binary due to Secure
>> Boot verification failing (EFI_SECURITY_VIOLATION, EFI_ACCESS_DENIED).
>>
>>
>> (2) If you cannot make the Linux UEFI stub a required part of the use
>> case, then X86QemuLoadImageLib needs to be modified indeed.
>>
>> (2a) Unfortunately, in this case we have to add a hack, because the
>> synthetic filesystem only exposes the concatenated "setup data + kernel
>> image" blob. The following would have to be preserved (as the only
>> remaining QemuFwCfgLib access):
>>
>>   QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
>>   SetupSize = (UINTN)QemuFwCfgRead32 ();
>>
>> (2b) and then the kernel blob from the synthetic fs would have to be
>> split in two (= setup, kernel), within QemuLoadLegacyImage().
>>
>>
>> I'm sorry for missing this aspect previously! I really hope we can go
>> with (1)!
>>
> 
> The legacy x86 loader should only be kept if there is a strong need to
> do so. Keeping it around for some theoretical compatibility concern is
> simply not worth it, IMHO.
> 
> Having two boot paths to reason about in terms of security is not the
> only problem: another concern is that the legacy fallback path is
> strictly x86, and is tightly coupled with the kernel's struct
> boot_params, which is only documented by the .h file that happens to
> declare it (and some outdated prose under Documentation/ perhaps)
> 
> Also, the EFI stub does some non-trivial work at boot, and having this
> uniform between architectures is an important goal, especially for
> non-x86 folks like me. We have introduced an initrd loader mechanism
> that is fully arch agnostic, and there are patches on the list to move
> the measurement of the initrd into the EFI stub if it was loaded using
> this mechanism.
> 
> In summary, please stick with the generic loader unless you *really* can't.
> 

Awesome, thank you!
Laszlo