[PATCH v2 00/11] Measured SEV boot with kernel/initrd/cmdline

[PATCH v2 00/11] Measured SEV boot with kernel/initrd/cmdline

[Dov Murik]

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

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 reserves an area in MEMFD (previously the last 1KB of
the launch secret page) 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 QEMU support [1].

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.

Relevant part of OVMF serial log during boot with AmdSevX86 build and QEMU with
-kernel/-initrd/-append:

  ...
  SevHashesBlobVerifierLibConstructor: found injected hashes table in secure location
  Select Item: 0x17
  Select Item: 0x8
  FetchBlob: loading 7379328 bytes for "kernel"
  Select Item: 0x18
  Select Item: 0x11
  VerifyBlob: Found GUID 4DE79437-ABD2-427F-B835-D5B172D2045B in table
  VerifyBlob: Hash comparison succeeded for entry 'kernel'
  Select Item: 0xB
  FetchBlob: loading 12483878 bytes for "initrd"
  Select Item: 0x12
  VerifyBlob: Found GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table
  VerifyBlob: Hash comparison succeeded for entry 'initrd'
  Select Item: 0x14
  FetchBlob: loading 86 bytes for "cmdline"
  Select Item: 0x15
  VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in table
  VerifyBlob: Hash comparison succeeded for entry 'cmdline'
  ...

The patch series is organized as follows:

1:     Simple comment fix in adjacent area in the code.
2:     Use GenericQemuLoadImageLib to gain one location for fw_cfg blob
       fetching.
3:     Allow the (previously blocked) usage of -kernel in AmdSevX64.
4-7:   Add BlobVerifierLib with null implementation and use it in the correct
       location in QemuKernelLoaderFsDxe.
8-9:   Reserve memory for hashes table, declare this area in the reset vector.
10-11: Add the secure implementation SevHashesBlobVerifierLib and use it in
       AmdSevX64 builds.

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

Code is at
https://github.com/confidential-containers-demo/edk2/tree/sev-hashes-v2

v2 changes:
 - Use the last 1KB of the existing SEV launch secret page for hashes table
   (instead of reserving a whole new MEMFD page).
 - Build on top of commit cf203024745f ("OvmfPkg/GenericQemuLoadImageLib: Read
   cmdline from QemuKernelLoaderFs", 2021-06-28) to have a single location in
   which all of kernel/initrd/cmdline are fetched from QEMU.
 - Use static linking of the two BlobVerifierLib implemenatations.
 - Reorganize series.

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

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>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Dov Murik (8):
  OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
  OvmfPkg: add library class BlobVerifierLib with null implementation
  OvmfPkg: add NullBlobVerifierLib to DSC
  ArmVirtPkg: add NullBlobVerifierLib to DSC
  OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
  OvmfPkg/AmdSev/SecretPei: build hob for full page
  OvmfPkg: add SevHashesBlobVerifierLib
  OvmfPkg/AmdSev: Enforce hash verification of kernel blobs

James Bottomley (3):
  OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
  OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
  OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes

 OvmfPkg/OvmfPkg.dec                                                                 |   9 +
 ArmVirtPkg/ArmVirtQemu.dsc                                                          |   5 +-
 ArmVirtPkg/ArmVirtQemuKernel.dsc                                                    |   5 +-
 OvmfPkg/AmdSev/AmdSevX64.dsc                                                        |   9 +-
 OvmfPkg/OvmfPkgIa32.dsc                                                             |   5 +-
 OvmfPkg/OvmfPkgIa32X64.dsc                                                          |   5 +-
 OvmfPkg/OvmfPkgX64.dsc                                                              |   5 +-
 OvmfPkg/AmdSev/AmdSevX64.fdf                                                        |   5 +-
 OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf                             |  27 +++
 OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf                        |  36 ++++
 OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf           |   2 +
 OvmfPkg/ResetVector/ResetVector.inf                                                 |   2 +
 OvmfPkg/Include/Library/BlobVerifierLib.h                                           |  38 ++++
 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                            |  11 ++
 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                                |   2 +-
 OvmfPkg/AmdSev/SecretPei/SecretPei.c                                                |   9 +-
 OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c                                  |  34 ++++
 OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c                             | 199 ++++++++++++++++++++
 OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                            |   5 +
 OvmfPkg/Library/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c |   0
 OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                               |   9 +
 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                                        |  20 ++
 OvmfPkg/ResetVector/ResetVector.nasmb                                               |   2 +
 23 files changed, 434 insertions(+), 10 deletions(-)
 create mode 100644 OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
 create mode 100644 OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
 create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h
 create mode 100644 OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c
 create mode 100644 OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
 copy OvmfPkg/Library/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c (100%)

-- 
2.25.1

[PATCH v2 01/11] 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 v2 02/11] OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds

[Dov Murik]

Newer kernels support efistub and therefore don't need all the legacy
stuff in X86QemuLoadImageLib, which are harder to secure.  Specifically
the verification of kernel/initrd/cmdlien blobs will be added only to
the GenericQemuLoadImageLib implementation, so use that for SEV builds.

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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 1d487befae08..a2f1324c40a6 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -368,7 +368,7 @@ [LibraryClasses.common.DXE_DRIVER]
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
-  QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
+  QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
 !if $(TPM_ENABLE) == TRUE
   Tpm12DeviceLib|SecurityPkg/Library/Tpm12DeviceLibTcg/Tpm12DeviceLibTcg.inf
   Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibTcg2/Tpm2DeviceLibTcg2.inf
-- 
2.25.1

[PATCH v2 03/11] 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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
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/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c |  0
 5 files changed, 19 insertions(+)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index a2f1324c40a6..aefdcf881c99 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -281,6 +281,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/PlatformBootManagerLib/QemuKernel.c b/OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
similarity index 100%
copy from OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
copy to OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
-- 
2.25.1

[PATCH v2 04/11] OvmfPkg: add library class BlobVerifierLib with null implementation

[Dov Murik]

BlobVerifierLib will be used to verify blobs fetching them from QEMU's
firmware config (fw_cfg) in platforms that enable such verification.

The null implementation NullBlobVerifierLib treats all blobs as valid.

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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/OvmfPkg.dec                                     |  3 ++
 OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf | 27 ++++++++++++++
 OvmfPkg/Include/Library/BlobVerifierLib.h               | 38 ++++++++++++++++++++
 OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c      | 34 ++++++++++++++++++
 4 files changed, 102 insertions(+)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 6ae733f6e39f..f82228d69cc2 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -23,6 +23,9 @@ [LibraryClasses]
   ##  @libraryclass  Access bhyve's firmware control interface.
   BhyveFwCtlLib|Include/Library/BhyveFwCtlLib.h
 
+  ##  @libraryclass  Verify blobs read from the VMM
+  BlobVerifierLib|Include/Library/BlobVerifierLib.h
+
   ##  @libraryclass  Loads and boots a Linux kernel image
   #
   LoadLinuxLib|Include/Library/LoadLinuxLib.h
diff --git a/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf b/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
new file mode 100644
index 000000000000..c8942ad05d96
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
@@ -0,0 +1,27 @@
+## @file
+#
+#  Null implementation of the blob verifier library.
+#
+#  Copyright (C) 2021, IBM Corp
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = NullBlobVerifierLib
+  FILE_GUID                      = b1b5533e-e01a-43bb-9e54-414f00ca036e
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = BlobVerifierLib
+
+[Sources]
+  NullBlobVerifier.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  DebugLib
diff --git a/OvmfPkg/Include/Library/BlobVerifierLib.h b/OvmfPkg/Include/Library/BlobVerifierLib.h
new file mode 100644
index 000000000000..667024766681
--- /dev/null
+++ b/OvmfPkg/Include/Library/BlobVerifierLib.h
@@ -0,0 +1,38 @@
+/** @file
+
+  Blob verification library
+
+  This library class allows verifiying whether blobs from external sources
+  (such as QEMU's firmware config) are trusted.
+
+  Copyright (C) 2021, IBM Corporation
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef BLOB_VERIFIER_LIB_H__
+#define BLOB_VERIFIER_LIB_H__
+
+#include <Uefi/UefiBaseType.h>
+#include <Base.h>
+
+/**
+  Verify blob from an external source.
+
+  @param BlobName               The name of the blob
+  @param Buf                    The data of the blob
+  @param BufSize                The size of the blob in bytes
+
+  @retval EFI_SUCCESS           The blob was verified successfully.
+  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore
+                                should be considered non-secure.
+**/
+EFI_STATUS
+EFIAPI
+VerifyBlob (
+  IN  CONST CHAR16    *BlobName,
+  IN  CONST VOID      *Buf,
+  UINT32              BufSize
+  );
+
+#endif
diff --git a/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c b/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c
new file mode 100644
index 000000000000..7b31b6ec767d
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c
@@ -0,0 +1,34 @@
+/** @file
+
+  Null implementation of the blob verifier library.
+
+  Copyright (C) 2021, IBM Corporation
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/BlobVerifierLib.h>
+
+/**
+  Verify blob from an external source.
+
+  @param BlobName               The name of the blob
+  @param Buf                    The data of the blob
+  @param BufSize                The size of the blob in bytes
+
+  @retval EFI_SUCCESS           The blob was verified successfully.
+  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore
+                                should be considered non-secure.
+**/
+EFI_STATUS
+EFIAPI
+VerifyBlob (
+  IN  CONST CHAR16    *BlobName,
+  IN  CONST VOID      *Buf,
+  UINT32              BufSize
+  )
+{
+  return EFI_SUCCESS;
+}
-- 
2.25.1

[PATCH v2 05/11] OvmfPkg: add NullBlobVerifierLib to DSC

[Dov Murik]

This prepares the ground for calling VerifyBlob() in
QemuKernelLoaderFsDxe.

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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc | 6 +++++-
 OvmfPkg/OvmfPkgIa32.dsc      | 5 ++++-
 OvmfPkg/OvmfPkgIa32X64.dsc   | 5 ++++-
 OvmfPkg/OvmfPkgX64.dsc       | 5 ++++-
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index aefdcf881c99..8b260df114e3 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -173,6 +173,7 @@ [LibraryClasses]
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
   CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
   FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
+  BlobVerifierLib|OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
 
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLibDebug.inf
@@ -693,7 +694,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/Library/BlobVerifierLib/NullBlobVerifierLib.inf
+  }
   OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
   OvmfPkg/Virtio10Dxe/Virtio10.inf
   OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index f53efeae7986..68e8a2f74249 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -786,7 +786,10 @@ [Components]
       NULL|OvmfPkg/Csm/LegacyBootMaintUiLib/LegacyBootMaintUiLib.inf
 !endif
   }
-  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf
+  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf {
+    <LibraryClasses>
+      NULL|OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
+  }
   OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
   OvmfPkg/Virtio10Dxe/Virtio10.inf
   OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index b3662e17f256..24d9cddc2447 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -800,7 +800,10 @@ [Components.X64]
       NULL|OvmfPkg/Csm/LegacyBootMaintUiLib/LegacyBootMaintUiLib.inf
 !endif
   }
-  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf
+  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf {
+    <LibraryClasses>
+      NULL|OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
+  }
   OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
   OvmfPkg/Virtio10Dxe/Virtio10.inf
   OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 0a237a905866..c4907efd7b67 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -798,7 +798,10 @@ [Components]
       NULL|OvmfPkg/Csm/LegacyBootMaintUiLib/LegacyBootMaintUiLib.inf
 !endif
   }
-  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf
+  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf {
+    <LibraryClasses>
+      NULL|OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
+  }
   OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
   OvmfPkg/Virtio10Dxe/Virtio10.inf
   OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
-- 
2.25.1

[PATCH v2 06/11] ArmVirtPkg: add NullBlobVerifierLib to DSC

[Dov Murik]

This prepares the ground for calling VerifyBlob() in
QemuKernelLoaderFsDxe.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 ArmVirtPkg/ArmVirtQemu.dsc       | 5 ++++-
 ArmVirtPkg/ArmVirtQemuKernel.dsc | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 7ef5e7297bc7..a0d3592ae0e6 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -440,7 +440,10 @@ [Components.common]
       NULL|MdeModulePkg/Library/BootManagerUiLib/BootManagerUiLib.inf
       NULL|MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceManagerUiLib.inf
   }
-  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf
+  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf {
+    <LibraryClasses>
+      NULL|OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
+  }
 
   #
   # Networking stack
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index a542fcb157e9..e63a7f0b6d63 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -376,7 +376,10 @@ [Components.common]
       NULL|MdeModulePkg/Library/BootManagerUiLib/BootManagerUiLib.inf
       NULL|MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceManagerUiLib.inf
   }
-  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf
+  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf {
+    <LibraryClasses>
+      NULL|OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
+  }
 
   #
   # Networking stack
-- 
2.25.1

[PATCH v2 07/11] OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg

[Dov Murik]

In QemuKernelLoaderFsDxeEntrypoint we use FetchBlob to read the content
of the kernel/initrd/cmdline from the QEMU fw_cfg interface.  Insert a
call to VerifyBlob after fetching to allow BlobVerifierLib
implementations to add a verification step for these blobs.

This will allow confidential computing OVMF builds to add verification
mechanisms for these blobs that originate from an untrusted source
(QEMU).

The null implementation of BlobVerifierLib does nothing in VerifyBlob,
and therefore no functional change is expected.

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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Co-developed-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
index c7ddd86f5c75..b43330d23b80 100644
--- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
+++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
@@ -17,6 +17,7 @@
 #include <Guid/QemuKernelLoaderFsMedia.h>
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
+#include <Library/BlobVerifierLib.h>
 #include <Library/DebugLib.h>
 #include <Library/DevicePathLib.h>
 #include <Library/MemoryAllocationLib.h>
@@ -1039,6 +1040,14 @@ QemuKernelLoaderFsDxeEntrypoint (
     if (EFI_ERROR (Status)) {
       goto FreeBlobs;
     }
+    Status = VerifyBlob (
+               CurrentBlob->Name,
+               CurrentBlob->Data,
+               CurrentBlob->Size
+             );
+    if (EFI_ERROR (Status)) {
+      goto FreeBlobs;
+    }
     mTotalBlobBytes += CurrentBlob->Size;
   }
   KernelBlob      = &mKernelBlob[KernelBlobTypeKernel];
-- 
2.25.1

[PATCH v2 08/11] OvmfPkg/AmdSev/SecretPei: build hob for full page

[Dov Murik]

Round up the size of the SEV launch secret area to a whole page, as
required by BuildMemoryAllocationHob.  This will allow the secret
area defined in the MEMFD to take less than a whole 4KB page.

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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/AmdSev/SecretPei/SecretPei.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
index ad491515dd5d..db4267428e5a 100644
--- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
+++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
@@ -15,9 +15,16 @@ InitializeSecretPei (
   IN CONST EFI_PEI_SERVICES     **PeiServices
   )
 {
+  UINT64 RoundedSize;
+
+  RoundedSize = PcdGet32 (PcdSevLaunchSecretSize);
+  if (RoundedSize % EFI_PAGE_SIZE != 0) {
+    RoundedSize = (RoundedSize / EFI_PAGE_SIZE + 1) * EFI_PAGE_SIZE;
+  }
+
   BuildMemoryAllocationHob (
     PcdGet32 (PcdSevLaunchSecretBase),
-    PcdGet32 (PcdSevLaunchSecretSize),
+    RoundedSize,
     EfiBootServicesData
     );
 
-- 
2.25.1

[PATCH v2 09/11] OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes

[Dov Murik]

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

Split the existing 4KB page reserved for SEV launch secrets into two
parts: first 3KB for SEV launch secrets and last 1KB for firmware
config hashes.

The area of the firmware config hashes will be attested (measured) by
the PSP and thus the untrusted VMM can't pass in different files from
what the guest owner allows.

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 (similar to the structure used to declare the launch
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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Co-developed-by: Dov Murik <dovmurik@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
---
 OvmfPkg/OvmfPkg.dec                          |  6 ++++++
 OvmfPkg/AmdSev/AmdSevX64.fdf                 |  5 ++++-
 OvmfPkg/ResetVector/ResetVector.inf          |  2 ++
 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 20 ++++++++++++++++++++
 OvmfPkg/ResetVector/ResetVector.nasmb        |  2 ++
 5 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index f82228d69cc2..2ab27f0c73c2 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -324,6 +324,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 9977b0f00a18..0a89749700c3 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -59,9 +59,12 @@ [FD.MEMFD]
 0x00B000|0x001000
 gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize
 
-0x00C000|0x001000
+0x00C000|0x000C00
 gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
 
+0x00CC00|0x000400
+gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
+
 0x00D000|0x001000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
 
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 v2 10/11] OvmfPkg: add SevHashesBlobVerifierLib

[Dov Murik]

Add an implementation for BlobVerifierLib that locates the SEV hashes
table and verifies that the calculated hashes of the kernel, initrd, and
cmdline blobs indeed match the expected hashes stated in the hashes
table.

If there's a missing hash or a hash mismatch then EFI_ACCESS_DENIED is
returned which will cause a failure to load a kernel image.

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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Co-developed-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf |  36 ++++
 OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c      | 199 ++++++++++++++++++++
 2 files changed, 235 insertions(+)

diff --git a/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
new file mode 100644
index 000000000000..b060d6a1b545
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
@@ -0,0 +1,36 @@
+## @file
+#
+#  Blob verifier library that uses SEV hashes table.
+#
+#  Copyright (C) 2021, IBM Corp
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = SevHashesBlobVerifierLib
+  FILE_GUID                      = 59e713b5-eff3-46a7-8d8b-46f4c004ad7b
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = BlobVerifierLib
+  CONSTRUCTOR                    = SevHashesBlobVerifierLibConstructor
+
+[Sources]
+  SevHashesBlobVerifier.c
+
+[Packages]
+  CryptoPkg/CryptoPkg.dec
+  MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  BaseCryptLib
+  BaseMemoryLib
+  DebugLib
+  PcdLib
+
+[FixedPcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
diff --git a/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
new file mode 100644
index 000000000000..961ee29f5df3
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
@@ -0,0 +1,199 @@
+/** @file
+
+  Blob verifier library that uses SEV hashes table.
+
+  Copyright (C) 2021, IBM Corporation
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/BaseCryptLib.h>
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/BlobVerifierLib.h>
+
+/**
+  The SEV Hashes 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 } }
+
+STATIC CONST EFI_GUID mSevKernelHashGuid = SEV_KERNEL_HASH_GUID;
+STATIC CONST EFI_GUID mSevInitrdHashGuid = SEV_INITRD_HASH_GUID;
+STATIC CONST EFI_GUID mSevCmdlineHashGuid = SEV_CMDLINE_HASH_GUID;
+
+#pragma pack (1)
+typedef struct {
+  GUID   Guid;
+  UINT16 Len;
+  UINT8  Data[];
+} HASH_TABLE;
+#pragma pack ()
+
+STATIC HASH_TABLE *mHashesTable;
+STATIC UINT16 mHashesTableSize;
+
+STATIC
+CONST GUID*
+FindBlobEntryGuid (
+  IN  CONST CHAR16    *BlobName
+  )
+{
+  if (StrCmp (BlobName, L"kernel") == 0) {
+    return &mSevKernelHashGuid;
+  } else if (StrCmp (BlobName, L"initrd") == 0) {
+    return &mSevInitrdHashGuid;
+  } else if (StrCmp (BlobName, L"cmdline") == 0) {
+    return &mSevCmdlineHashGuid;
+  } else {
+    return NULL;
+  }
+}
+
+/**
+  Verify blob from an external source.
+
+  @param BlobName               The name of the blob
+  @param Buf                    The data of the blob
+  @param BufSize                The size of the blob in bytes
+
+  @retval EFI_SUCCESS           The blob was verified successfully.
+  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore
+                                should be considered non-secure.
+**/
+EFI_STATUS
+EFIAPI
+VerifyBlob (
+  IN  CONST CHAR16    *BlobName,
+  IN  CONST VOID      *Buf,
+  UINT32              BufSize
+  )
+{
+  CONST GUID *Guid;
+  INT32 Len;
+  HASH_TABLE *Entry;
+  UINT8 Hash[SHA256_DIGEST_SIZE];
+
+  if (mHashesTable == NULL || mHashesTableSize == 0) {
+    DEBUG ((DEBUG_ERROR,
+      "%a: Verifier called but no hashes table discoverd in MEMFD\n",
+      __FUNCTION__));
+    return EFI_ACCESS_DENIED;
+  }
+
+  Guid = FindBlobEntryGuid (BlobName);
+  if (Guid == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a: Unknown blob name \"%s\"\n", __FUNCTION__,
+      BlobName));
+    return EFI_ACCESS_DENIED;
+  }
+
+  Sha256HashAll (Buf, BufSize, Hash);
+
+  for (Entry = mHashesTable, Len = 0;
+       Len < (INT32)mHashesTableSize;
+       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 calculated hash is identical to the expected
+    // 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 for \"%s\"\n",
+        __FUNCTION__, BlobName));
+    } else {
+      Status = EFI_ACCESS_DENIED;
+      DEBUG ((DEBUG_ERROR, "%a: Hash comparison failed for \"%s\"\n",
+        __FUNCTION__, BlobName));
+    }
+    return Status;
+  }
+
+  DEBUG ((DEBUG_ERROR, "%a: Hash GUID %g not found in table\n", __FUNCTION__,
+    Guid));
+  return EFI_ACCESS_DENIED;
+}
+
+/**
+  Locate the SEV hashes table.
+
+  This function always returns success, even if the table can't be found.  The
+  subsequent VerifyBlob calls will fail if no table was found.
+
+  @retval RETURN_SUCCESS   The verifier tables were set up correctly
+**/
+RETURN_STATUS
+EFIAPI
+SevHashesBlobVerifierLibConstructor (
+  VOID
+  )
+{
+  HASH_TABLE *Ptr = (void *)(UINTN)FixedPcdGet64 (PcdQemuHashTableBase);
+  UINT32 Size = FixedPcdGet32 (PcdQemuHashTableSize);
+
+  mHashesTable = NULL;
+  mHashesTableSize = 0;
+
+  if (Ptr == NULL || Size == 0) {
+    return RETURN_SUCCESS;
+  }
+
+  if (!CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID)) {
+    return RETURN_SUCCESS;
+  }
+
+  if (Ptr->Len < (sizeof Ptr->Guid + sizeof Ptr->Len)) {
+    return RETURN_SUCCESS;
+  }
+
+  DEBUG ((DEBUG_INFO, "%a: Found injected hashes table in secure location\n",
+    __FUNCTION__));
+
+  mHashesTable = (HASH_TABLE *)Ptr->Data;
+  mHashesTableSize = Ptr->Len - sizeof Ptr->Guid - sizeof Ptr->Len;
+
+  DEBUG ((DEBUG_VERBOSE, "%a: mHashesTable=0x%p, Size=%u\n", __FUNCTION__,
+    mHashesTable, mHashesTableSize));
+
+  return RETURN_SUCCESS;
+}
-- 
2.25.1

[PATCH v2 11/11] OvmfPkg/AmdSev: Enforce hash verification of kernel blobs

[Dov Murik]

In the AmdSevX86 build, use SevHashesBlobVerifierLib to enforce
verification of hashes of the kernel/initrd/cmdline blobs fetched from
firmware config.

This allows for secure (measured) boot of SEV guests with QEMU's
-kernel/-initrd/-append switches (with the corresponding QEMU support
for injecting the hashes table into initial measured guest memory).

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>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 8b260df114e3..d1ed0abbd0fb 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -173,7 +173,7 @@ [LibraryClasses]
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
   CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
   FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
-  BlobVerifierLib|OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
+  BlobVerifierLib|OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
 
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLibDebug.inf
@@ -696,7 +696,7 @@ [Components]
   }
   OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf {
     <LibraryClasses>
-      NULL|OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
+      NULL|OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
   }
   OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
   OvmfPkg/Virtio10Dxe/Virtio10.inf
-- 
2.25.1

Re: [edk2-devel] [PATCH v2 00/11] Measured SEV boot with kernel/initrd/cmdline

[Ard Biesheuvel]

On Tue, 6 Jul 2021 at 10:55, Dov Murik <dovmurik@linux.ibm.com> wrote:
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
>
> 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 reserves an area in MEMFD (previously the last 1KB of
> the launch secret page) 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 QEMU support [1].
>
> 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.
>
> Relevant part of OVMF serial log during boot with AmdSevX86 build and QEMU with
> -kernel/-initrd/-append:
>
>   ...
>   SevHashesBlobVerifierLibConstructor: found injected hashes table in secure location
>   Select Item: 0x17
>   Select Item: 0x8
>   FetchBlob: loading 7379328 bytes for "kernel"
>   Select Item: 0x18
>   Select Item: 0x11
>   VerifyBlob: Found GUID 4DE79437-ABD2-427F-B835-D5B172D2045B in table
>   VerifyBlob: Hash comparison succeeded for entry 'kernel'
>   Select Item: 0xB
>   FetchBlob: loading 12483878 bytes for "initrd"
>   Select Item: 0x12
>   VerifyBlob: Found GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table
>   VerifyBlob: Hash comparison succeeded for entry 'initrd'
>   Select Item: 0x14
>   FetchBlob: loading 86 bytes for "cmdline"
>   Select Item: 0x15
>   VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in table
>   VerifyBlob: Hash comparison succeeded for entry 'cmdline'
>   ...
>
> The patch series is organized as follows:
>
> 1:     Simple comment fix in adjacent area in the code.
> 2:     Use GenericQemuLoadImageLib to gain one location for fw_cfg blob
>        fetching.
> 3:     Allow the (previously blocked) usage of -kernel in AmdSevX64.
> 4-7:   Add BlobVerifierLib with null implementation and use it in the correct
>        location in QemuKernelLoaderFsDxe.
> 8-9:   Reserve memory for hashes table, declare this area in the reset vector.
> 10-11: Add the secure implementation SevHashesBlobVerifierLib and use it in
>        AmdSevX64 builds.
>
> [1] https://lore.kernel.org/qemu-devel/20210624102040.2015280-1-dovmurik@linux.ibm.com/
>
> Code is at
> https://github.com/confidential-containers-demo/edk2/tree/sev-hashes-v2
>
> v2 changes:
>  - Use the last 1KB of the existing SEV launch secret page for hashes table
>    (instead of reserving a whole new MEMFD page).
>  - Build on top of commit cf203024745f ("OvmfPkg/GenericQemuLoadImageLib: Read
>    cmdline from QemuKernelLoaderFs", 2021-06-28) to have a single location in
>    which all of kernel/initrd/cmdline are fetched from QEMU.
>  - Use static linking of the two BlobVerifierLib implemenatations.
>  - Reorganize series.
>
> v1: https://edk2.groups.io/g/devel/message/75567
>
> 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>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
>

Anyone on the cc list care to review this?


> Dov Murik (8):
>   OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
>   OvmfPkg: add library class BlobVerifierLib with null implementation
>   OvmfPkg: add NullBlobVerifierLib to DSC
>   ArmVirtPkg: add NullBlobVerifierLib to DSC
>   OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
>   OvmfPkg/AmdSev/SecretPei: build hob for full page
>   OvmfPkg: add SevHashesBlobVerifierLib
>   OvmfPkg/AmdSev: Enforce hash verification of kernel blobs
>
> James Bottomley (3):
>   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
>   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
>   OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes
>
>  OvmfPkg/OvmfPkg.dec                                                                 |   9 +
>  ArmVirtPkg/ArmVirtQemu.dsc                                                          |   5 +-
>  ArmVirtPkg/ArmVirtQemuKernel.dsc                                                    |   5 +-
>  OvmfPkg/AmdSev/AmdSevX64.dsc                                                        |   9 +-
>  OvmfPkg/OvmfPkgIa32.dsc                                                             |   5 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc                                                          |   5 +-
>  OvmfPkg/OvmfPkgX64.dsc                                                              |   5 +-
>  OvmfPkg/AmdSev/AmdSevX64.fdf                                                        |   5 +-
>  OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf                             |  27 +++
>  OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf                        |  36 ++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf           |   2 +
>  OvmfPkg/ResetVector/ResetVector.inf                                                 |   2 +
>  OvmfPkg/Include/Library/BlobVerifierLib.h                                           |  38 ++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                            |  11 ++
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                                |   2 +-
>  OvmfPkg/AmdSev/SecretPei/SecretPei.c                                                |   9 +-
>  OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c                                  |  34 ++++
>  OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c                             | 199 ++++++++++++++++++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                            |   5 +
>  OvmfPkg/Library/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c |   0
>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                               |   9 +
>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                                        |  20 ++
>  OvmfPkg/ResetVector/ResetVector.nasmb                                               |   2 +
>  23 files changed, 434 insertions(+), 10 deletions(-)
>  create mode 100644 OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
>  create mode 100644 OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
>  create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h
>  create mode 100644 OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c
>  create mode 100644 OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
>  copy OvmfPkg/Library/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c (100%)
>
> --
> 2.25.1
>
>
>
> 
>
>

Re: [PATCH v2 01/11] OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming

[Brijesh Singh]

On 7/6/21 3:54 AM, Dov Murik wrote:
> 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>

Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

thanks

Re: [PATCH v2 02/11] OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds

[Brijesh Singh]

On 7/6/21 3:54 AM, Dov Murik wrote:
> Newer kernels support efistub and therefore don't need all the legacy
> stuff in X86QemuLoadImageLib, which are harder to secure.  Specifically
> the verification of kernel/initrd/cmdlien blobs will be added only to
> the GenericQemuLoadImageLib implementation, so use that for SEV builds.
>
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>

Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

thanks

Re: [PATCH v2 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg

[Brijesh Singh]

On 7/6/21 3:54 AM, Dov Murik wrote:
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>

Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

thanks

Re: [PATCH v2 04/11] OvmfPkg: add library class BlobVerifierLib with null implementation

[Brijesh Singh]

On 7/6/21 3:54 AM, Dov Murik wrote:
> BlobVerifierLib will be used to verify blobs fetching them from QEMU's
> firmware config (fw_cfg) in platforms that enable such verification.
>
> The null implementation NullBlobVerifierLib treats all blobs as valid.
>
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>

Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

thanks

Re: [PATCH v2 05/11] OvmfPkg: add NullBlobVerifierLib to DSC

[Brijesh Singh]

On 7/6/21 3:54 AM, Dov Murik wrote:
> This prepares the ground for calling VerifyBlob() in
> QemuKernelLoaderFsDxe.
>
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>

Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

thanks

Re: [PATCH v2 06/11] ArmVirtPkg: add NullBlobVerifierLib to DSC

[Brijesh Singh]

On 7/6/21 3:54 AM, Dov Murik wrote:
> This prepares the ground for calling VerifyBlob() in
> QemuKernelLoaderFsDxe.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>

Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

thanks

Re: [PATCH v2 07/11] OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg

[Brijesh Singh]

On 7/6/21 3:54 AM, Dov Murik wrote:
> In QemuKernelLoaderFsDxeEntrypoint we use FetchBlob to read the content
> of the kernel/initrd/cmdline from the QEMU fw_cfg interface.  Insert a
> call to VerifyBlob after fetching to allow BlobVerifierLib
> implementations to add a verification step for these blobs.
>
> This will allow confidential computing OVMF builds to add verification
> mechanisms for these blobs that originate from an untrusted source
> (QEMU).
>
> The null implementation of BlobVerifierLib does nothing in VerifyBlob,
> and therefore no functional change is expected.
>
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>

The patch itself is okay. Just curious, do we also need to add a
verification for the QEMU FW cfg file ?

Re: [edk2-devel] [PATCH v2 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg

[Christoph Willing]

[-- Attachment #1: Type: text/plain, Size: 549 bytes --]

I have been using Qemu with -kernel,-initrd,-append options without OVMF for some time in my workflow. When booting with OVMF, I have found that the most recent tag in the edk2 public git repo that supports those options is vUDK2018. Therefore I'm extremely interested in this series of patches, hoping they'll restore the missing functionality, and would like to test them locally. Is there any location from where I could obtain the patches themselves or, better still, access to a repo somewhere that already has them applied?

Thanks,
chris

[-- Attachment #2: Type: text/html, Size: 561 bytes --]

Re: [edk2-devel] [PATCH v2 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg

[Dov Murik]

Chris,

On 19/07/2021 7:46, Christoph Willing wrote:
> I have been using Qemu with -kernel,-initrd,-append options without OVMF for some time in my workflow. When booting with OVMF, I have found that the most recent tag in the edk2 public git repo that supports those options is vUDK2018. Therefore I'm extremely interested in this series of patches, hoping they'll restore the missing functionality, and would like to test them locally. Is there any location from where I could obtain the patches themselves or, better still, access to a repo somewhere that already has them applied?
> 

Are you trying to boot a VM with AMD SEV? This patch series is all about enabling
these switches for SEV (memory-encrypted VMs).  For "normal" VMs there should
be no change.

If you indeed use SEV guests, this patch series is available in 
https://github.com/confidential-containers-demo/edk2/tree/sev-hashes-v2

but also requires the corresponding QEMU patches in
https://lore.kernel.org/qemu-devel/20210624102040.2015280-1-dovmurik@linux.ibm.com/ 
which are not yet upstream.


-Dov

Re: [PATCH v2 07/11] OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg

[Dov Murik]

On 18/07/2021 18:47, Brijesh Singh wrote:
> 
> On 7/6/21 3:54 AM, Dov Murik wrote:
>> In QemuKernelLoaderFsDxeEntrypoint we use FetchBlob to read the content
>> of the kernel/initrd/cmdline from the QEMU fw_cfg interface.  Insert a
>> call to VerifyBlob after fetching to allow BlobVerifierLib
>> implementations to add a verification step for these blobs.
>>
>> This will allow confidential computing OVMF builds to add verification
>> mechanisms for these blobs that originate from an untrusted source
>> (QEMU).
>>
>> The null implementation of BlobVerifierLib does nothing in VerifyBlob,
>> and therefore no functional change is expected.
>>
>> 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>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
>> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> 
> The patch itself is okay. Just curious, do we also need to add a
> verification for the QEMU FW cfg file ?
> 

I don't really understand.  This patch adds the VerifyBlob() call on
blobs that were read by FetchBlob(), which in turn reads the contents of
kernel/initrd/cmdline from QEMU FW cfg (using QemuFwCfgReadBytes for
example).

We currently *don't* add verification for all other FW cfg settings,
like number of CPUs, E820 memory entries, ... similar to what we (don't)
do in SEV boot with encrypted root image (in which only OVMF is measured).

What else do you think we should verify?

-Dov

Re: [edk2-devel] [PATCH v2 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg

[Christoph Willing]

[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]

Thanks for the clarification Dov.

I've been trying with just "normal" VMs, not SEV. I did already find and try the confidential-containers-demo sev-hashes-v2 branch but it didn't help - not surprising if it's not relevant to normal VMs.

Do you know whether this functionality (-kernel, -initrd, -append options) is actually supposed to work in normal VMs at the moment? The only conditions under which it works here with qemu-6.0.0 is with vUDK2017 & 2018 and an old ovmf binary package from kraxel.og dated 2017. Anything built from the edk2 master branch has failed when using those qemu options, although all the same builds work perfectly using the VMs' internal kernels & initrds. I've also extracted OVMF files from the current kraxel.org package as well as Ubuntu's (hirsute) package and these also fail the same way i.e. kernel boots and initrd works (loads modules) but then the VM filesystem doesn't seem to be found (no /dev/sdX exists to mount the filesystem root).

I guess this could be a qemu problem but since it works with some (old) udk/edk2 versions, I thought I'd look here first.

Thanks for any help or pointers,
chris

[-- Attachment #2: Type: text/html, Size: 1197 bytes --]

Re: [PATCH v2 00/11] Measured SEV boot with kernel/initrd/cmdline

[Lendacky, Thomas]

On 7/6/21 3:54 AM, Dov Murik wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3457

This BZ link should be part of all the commit messages in the series.

Thanks,
Tom

> 
> 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 reserves an area in MEMFD (previously the last 1KB of
> the launch secret page) 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 QEMU support [1].
> 
> 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.
> 
> Relevant part of OVMF serial log during boot with AmdSevX86 build and QEMU with
> -kernel/-initrd/-append:
> 
>   ...
>   SevHashesBlobVerifierLibConstructor: found injected hashes table in secure location
>   Select Item: 0x17
>   Select Item: 0x8
>   FetchBlob: loading 7379328 bytes for "kernel"
>   Select Item: 0x18
>   Select Item: 0x11
>   VerifyBlob: Found GUID 4DE79437-ABD2-427F-B835-D5B172D2045B in table
>   VerifyBlob: Hash comparison succeeded for entry 'kernel'
>   Select Item: 0xB
>   FetchBlob: loading 12483878 bytes for "initrd"
>   Select Item: 0x12
>   VerifyBlob: Found GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table
>   VerifyBlob: Hash comparison succeeded for entry 'initrd'
>   Select Item: 0x14
>   FetchBlob: loading 86 bytes for "cmdline"
>   Select Item: 0x15
>   VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in table
>   VerifyBlob: Hash comparison succeeded for entry 'cmdline'
>   ...
> 
> The patch series is organized as follows:
> 
> 1:     Simple comment fix in adjacent area in the code.
> 2:     Use GenericQemuLoadImageLib to gain one location for fw_cfg blob
>        fetching.
> 3:     Allow the (previously blocked) usage of -kernel in AmdSevX64.
> 4-7:   Add BlobVerifierLib with null implementation and use it in the correct
>        location in QemuKernelLoaderFsDxe.
> 8-9:   Reserve memory for hashes table, declare this area in the reset vector.
> 10-11: Add the secure implementation SevHashesBlobVerifierLib and use it in
>        AmdSevX64 builds.
> 
> [1] https://lore.kernel.org/qemu-devel/20210624102040.2015280-1-dovmurik@linux.ibm.com/
> 
> Code is at
> https://github.com/confidential-containers-demo/edk2/tree/sev-hashes-v2
> 
> v2 changes:
>  - Use the last 1KB of the existing SEV launch secret page for hashes table
>    (instead of reserving a whole new MEMFD page).
>  - Build on top of commit cf203024745f ("OvmfPkg/GenericQemuLoadImageLib: Read
>    cmdline from QemuKernelLoaderFs", 2021-06-28) to have a single location in
>    which all of kernel/initrd/cmdline are fetched from QEMU.
>  - Use static linking of the two BlobVerifierLib implemenatations.
>  - Reorganize series.
> 
> v1: https://edk2.groups.io/g/devel/message/75567
> 
> 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>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> 
> Dov Murik (8):
>   OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
>   OvmfPkg: add library class BlobVerifierLib with null implementation
>   OvmfPkg: add NullBlobVerifierLib to DSC
>   ArmVirtPkg: add NullBlobVerifierLib to DSC
>   OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
>   OvmfPkg/AmdSev/SecretPei: build hob for full page
>   OvmfPkg: add SevHashesBlobVerifierLib
>   OvmfPkg/AmdSev: Enforce hash verification of kernel blobs
> 
> James Bottomley (3):
>   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
>   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
>   OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes
> 
>  OvmfPkg/OvmfPkg.dec                                                                 |   9 +
>  ArmVirtPkg/ArmVirtQemu.dsc                                                          |   5 +-
>  ArmVirtPkg/ArmVirtQemuKernel.dsc                                                    |   5 +-
>  OvmfPkg/AmdSev/AmdSevX64.dsc                                                        |   9 +-
>  OvmfPkg/OvmfPkgIa32.dsc                                                             |   5 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc                                                          |   5 +-
>  OvmfPkg/OvmfPkgX64.dsc                                                              |   5 +-
>  OvmfPkg/AmdSev/AmdSevX64.fdf                                                        |   5 +-
>  OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf                             |  27 +++
>  OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf                        |  36 ++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf           |   2 +
>  OvmfPkg/ResetVector/ResetVector.inf                                                 |   2 +
>  OvmfPkg/Include/Library/BlobVerifierLib.h                                           |  38 ++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                            |  11 ++
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                                |   2 +-
>  OvmfPkg/AmdSev/SecretPei/SecretPei.c                                                |   9 +-
>  OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c                                  |  34 ++++
>  OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c                             | 199 ++++++++++++++++++++
>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                            |   5 +
>  OvmfPkg/Library/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c |   0
>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                               |   9 +
>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                                        |  20 ++
>  OvmfPkg/ResetVector/ResetVector.nasmb                                               |   2 +
>  23 files changed, 434 insertions(+), 10 deletions(-)
>  create mode 100644 OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
>  create mode 100644 OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
>  create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h
>  create mode 100644 OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c
>  create mode 100644 OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
>  copy OvmfPkg/Library/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c (100%)
> 

Re: [PATCH v2 07/11] OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg

[Brijesh Singh]

On 7/19/21 7:22 AM, Dov Murik wrote:
>> The patch itself is okay. Just curious, do we also need to add a
>> verification for the QEMU FW cfg file ?
>>
> 
> I don't really understand.  This patch adds the VerifyBlob() call on
> blobs that were read by FetchBlob(), which in turn reads the contents of
> kernel/initrd/cmdline from QEMU FW cfg (using QemuFwCfgReadBytes for
> example).
> 
> We currently *don't* add verification for all other FW cfg settings,
> like number of CPUs, E820 memory entries, ... similar to what we (don't)
> do in SEV boot with encrypted root image (in which only OVMF is measured).
> 
> What else do you think we should verify?
> 

As I understand that your series is attempting to add more security 
checks in the SEV boot sequence; i.e. after this series is merged, we 
can verify the kernel,cmdline and initrd passed through qemu. But there 
are several other configuration parameters (such as e820, acpi) that 
gets passed by the qemu and consumed by the ovmf. Are you considering to 
add the checks to cover those blobs in the future series? To me it seems 
that the framework built here can be extended to cover those as well.

Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

thanks!

Re: [PATCH v2 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg

[Lendacky, Thomas]

On 7/6/21 3:54 AM, Dov Murik wrote:
> 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 .

Just a nit, but this confused me initially. Maybe it should say something
along the lines of create a QemuKernel.c for PlatformBootManagerLibGrub
that is an exact copy of the file from PlatformBootManagerLib.

Is there any way that the two libraries can use the same file rather than
making an exact copy?

Thanks,
Tom

> 
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> 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/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c |  0
>  5 files changed, 19 insertions(+)
> 
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index a2f1324c40a6..aefdcf881c99 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -281,6 +281,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/PlatformBootManagerLib/QemuKernel.c b/OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
> similarity index 100%
> copy from OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
> copy to OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
> 

Re: [PATCH v2 04/11] OvmfPkg: add library class BlobVerifierLib with null implementation

[Lendacky, Thomas]

On 7/6/21 3:54 AM, Dov Murik wrote:
> BlobVerifierLib will be used to verify blobs fetching them from QEMU's
> firmware config (fw_cfg) in platforms that enable such verification.
> 
> The null implementation NullBlobVerifierLib treats all blobs as valid.
> 
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  OvmfPkg/OvmfPkg.dec                                     |  3 ++
>  OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf | 27 ++++++++++++++
>  OvmfPkg/Include/Library/BlobVerifierLib.h               | 38 ++++++++++++++++++++
>  OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c      | 34 ++++++++++++++++++
>  4 files changed, 102 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 6ae733f6e39f..f82228d69cc2 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -23,6 +23,9 @@ [LibraryClasses]
>    ##  @libraryclass  Access bhyve's firmware control interface.
>    BhyveFwCtlLib|Include/Library/BhyveFwCtlLib.h
>  
> +  ##  @libraryclass  Verify blobs read from the VMM
> +  BlobVerifierLib|Include/Library/BlobVerifierLib.h
> +
>    ##  @libraryclass  Loads and boots a Linux kernel image
>    #
>    LoadLinuxLib|Include/Library/LoadLinuxLib.h
> diff --git a/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf b/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
> new file mode 100644
> index 000000000000..c8942ad05d96
> --- /dev/null
> +++ b/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
> @@ -0,0 +1,27 @@
> +## @file
> +#
> +#  Null implementation of the blob verifier library.
> +#
> +#  Copyright (C) 2021, IBM Corp
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005

You can specify the INF_VERSION using x.y format now, and I believe the
latest is 1.29.

> +  BASE_NAME                      = NullBlobVerifierLib

Typically, the NULL libraries would be named BlobVerifierLibNull.

> +  FILE_GUID                      = b1b5533e-e01a-43bb-9e54-414f00ca036e
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = BlobVerifierLib
> +
> +[Sources]
> +  NullBlobVerifier.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  DebugLib

Is this library (and associated include below) needed?

> diff --git a/OvmfPkg/Include/Library/BlobVerifierLib.h b/OvmfPkg/Include/Library/BlobVerifierLib.h
> new file mode 100644
> index 000000000000..667024766681
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/BlobVerifierLib.h
> @@ -0,0 +1,38 @@
> +/** @file
> +
> +  Blob verification library
> +
> +  This library class allows verifiying whether blobs from external sources
> +  (such as QEMU's firmware config) are trusted.
> +
> +  Copyright (C) 2021, IBM Corporation
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef BLOB_VERIFIER_LIB_H__
> +#define BLOB_VERIFIER_LIB_H__
> +
> +#include <Uefi/UefiBaseType.h>
> +#include <Base.h>
> +
> +/**
> +  Verify blob from an external source.
> +
> +  @param BlobName               The name of the blob

I believe this is supposed to be @param[in]

> +  @param Buf                    The data of the blob
> +  @param BufSize                The size of the blob in bytes
> +
> +  @retval EFI_SUCCESS           The blob was verified successfully.
> +  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore
> +                                should be considered non-secure.
> +**/
> +EFI_STATUS
> +EFIAPI
> +VerifyBlob (
> +  IN  CONST CHAR16    *BlobName,
> +  IN  CONST VOID      *Buf,
> +  UINT32              BufSize

Missing "IN" here (same below for these).

Thanks,
Tom

> +  );
> +
> +#endif
> diff --git a/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c b/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c
> new file mode 100644
> index 000000000000..7b31b6ec767d
> --- /dev/null
> +++ b/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c
> @@ -0,0 +1,34 @@
> +/** @file
> +
> +  Null implementation of the blob verifier library.
> +
> +  Copyright (C) 2021, IBM Corporation
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/BlobVerifierLib.h>
> +
> +/**
> +  Verify blob from an external source.
> +
> +  @param BlobName               The name of the blob
> +  @param Buf                    The data of the blob
> +  @param BufSize                The size of the blob in bytes
> +
> +  @retval EFI_SUCCESS           The blob was verified successfully.
> +  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore
> +                                should be considered non-secure.
> +**/
> +EFI_STATUS
> +EFIAPI
> +VerifyBlob (
> +  IN  CONST CHAR16    *BlobName,
> +  IN  CONST VOID      *Buf,
> +  UINT32              BufSize
> +  )
> +{
> +  return EFI_SUCCESS;
> +}
> 

Re: [PATCH v2 07/11] OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg

[Lendacky, Thomas]

On 7/6/21 3:54 AM, Dov Murik wrote:
> In QemuKernelLoaderFsDxeEntrypoint we use FetchBlob to read the content
> of the kernel/initrd/cmdline from the QEMU fw_cfg interface.  Insert a
> call to VerifyBlob after fetching to allow BlobVerifierLib
> implementations to add a verification step for these blobs.
> 
> This will allow confidential computing OVMF builds to add verification
> mechanisms for these blobs that originate from an untrusted source
> (QEMU).
> 
> The null implementation of BlobVerifierLib does nothing in VerifyBlob,
> and therefore no functional change is expected.
> 
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> index c7ddd86f5c75..b43330d23b80 100644
> --- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> +++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
> @@ -17,6 +17,7 @@
>  #include <Guid/QemuKernelLoaderFsMedia.h>
>  #include <Library/BaseLib.h>
>  #include <Library/BaseMemoryLib.h>
> +#include <Library/BlobVerifierLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/DevicePathLib.h>
>  #include <Library/MemoryAllocationLib.h>
> @@ -1039,6 +1040,14 @@ QemuKernelLoaderFsDxeEntrypoint (
>      if (EFI_ERROR (Status)) {
>        goto FreeBlobs;
>      }
> +    Status = VerifyBlob (
> +               CurrentBlob->Name,
> +               CurrentBlob->Data,
> +               CurrentBlob->Size
> +             );

Just a nit, but the ");" should be under the "C" in CurrentBlob.

Thanks,
Tom

> +    if (EFI_ERROR (Status)) {
> +      goto FreeBlobs;
> +    }
>      mTotalBlobBytes += CurrentBlob->Size;
>    }
>    KernelBlob      = &mKernelBlob[KernelBlobTypeKernel];
> 

Re: [PATCH v2 08/11] OvmfPkg/AmdSev/SecretPei: build hob for full page

[Lendacky, Thomas]

On 7/6/21 3:54 AM, Dov Murik wrote:
> Round up the size of the SEV launch secret area to a whole page, as
> required by BuildMemoryAllocationHob.  This will allow the secret
> area defined in the MEMFD to take less than a whole 4KB page.
> 
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  OvmfPkg/AmdSev/SecretPei/SecretPei.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
> index ad491515dd5d..db4267428e5a 100644
> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
> @@ -15,9 +15,16 @@ InitializeSecretPei (
>    IN CONST EFI_PEI_SERVICES     **PeiServices
>    )
>  {
> +  UINT64 RoundedSize;
> +
> +  RoundedSize = PcdGet32 (PcdSevLaunchSecretSize);

Can you just unconditionally perform:

  RoundedSize = ALIGN_VALUE (RoundedSize, EFI_PAGE_SIZE);

Or use ALIGN_VALUE () in the if statement if you don't want to do it
unconditionally?

Or even use ALIGN_VALUE on size value in the BuildMemoryAllocationHob()
call below.

Thanks,
Tom

> +  if (RoundedSize % EFI_PAGE_SIZE != 0) {
> +    RoundedSize = (RoundedSize / EFI_PAGE_SIZE + 1) * EFI_PAGE_SIZE;
> +  }
> +
>    BuildMemoryAllocationHob (
>      PcdGet32 (PcdSevLaunchSecretBase),
> -    PcdGet32 (PcdSevLaunchSecretSize),
> +    RoundedSize,
>      EfiBootServicesData
>      );
>  
> 

Re: [PATCH v2 09/11] OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes

[Lendacky, Thomas]

On 7/6/21 3:54 AM, Dov Murik wrote:
> From: James Bottomley <jejb@linux.ibm.com>
> 
> Split the existing 4KB page reserved for SEV launch secrets into two
> parts: first 3KB for SEV launch secrets and last 1KB for firmware
> config hashes.
> 
> The area of the firmware config hashes will be attested (measured) by
> the PSP and thus the untrusted VMM can't pass in different files from
> what the guest owner allows.
> 
> 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 (similar to the structure used to declare the launch
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Co-developed-by: Dov Murik <dovmurik@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>

Reviewed by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  OvmfPkg/OvmfPkg.dec                          |  6 ++++++
>  OvmfPkg/AmdSev/AmdSevX64.fdf                 |  5 ++++-
>  OvmfPkg/ResetVector/ResetVector.inf          |  2 ++
>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 20 ++++++++++++++++++++
>  OvmfPkg/ResetVector/ResetVector.nasmb        |  2 ++
>  5 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index f82228d69cc2..2ab27f0c73c2 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -324,6 +324,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 9977b0f00a18..0a89749700c3 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.fdf
> +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
> @@ -59,9 +59,12 @@ [FD.MEMFD]
>  0x00B000|0x001000
>  gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize
>  
> -0x00C000|0x001000
> +0x00C000|0x000C00
>  gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
>  
> +0x00CC00|0x000400
> +gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
> +
>  0x00D000|0x001000
>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
>  
> 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"
>  
> 

Re: [PATCH v2 10/11] OvmfPkg: add SevHashesBlobVerifierLib

[Lendacky, Thomas]

On 7/6/21 3:55 AM, Dov Murik wrote:
> Add an implementation for BlobVerifierLib that locates the SEV hashes
> table and verifies that the calculated hashes of the kernel, initrd, and
> cmdline blobs indeed match the expected hashes stated in the hashes
> table.
> 
> If there's a missing hash or a hash mismatch then EFI_ACCESS_DENIED is
> returned which will cause a failure to load a kernel image.
> 
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf |  36 ++++
>  OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c      | 199 ++++++++++++++++++++
>  2 files changed, 235 insertions(+)
> 
> diff --git a/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
> new file mode 100644
> index 000000000000..b060d6a1b545
> --- /dev/null
> +++ b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
> @@ -0,0 +1,36 @@
> +## @file
> +#
> +#  Blob verifier library that uses SEV hashes table.
> +#
> +#  Copyright (C) 2021, IBM Corp
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +

Same comments here as were made on the Null library instance.

> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = SevHashesBlobVerifierLib
> +  FILE_GUID                      = 59e713b5-eff3-46a7-8d8b-46f4c004ad7b
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = BlobVerifierLib
> +  CONSTRUCTOR                    = SevHashesBlobVerifierLibConstructor
> +
> +[Sources]
> +  SevHashesBlobVerifier.c
> +
> +[Packages]
> +  CryptoPkg/CryptoPkg.dec
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  BaseCryptLib
> +  BaseMemoryLib
> +  DebugLib
> +  PcdLib
> +
> +[FixedPcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase
> +  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
> diff --git a/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
> new file mode 100644
> index 000000000000..961ee29f5df3
> --- /dev/null
> +++ b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
> @@ -0,0 +1,199 @@
> +/** @file
> +
> +  Blob verifier library that uses SEV hashes table.
> +
> +  Copyright (C) 2021, IBM Corporation
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <Library/BaseCryptLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/BlobVerifierLib.h>
> +
> +/**
> +  The SEV Hashes 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 } }
> +
> +STATIC CONST EFI_GUID mSevKernelHashGuid = SEV_KERNEL_HASH_GUID;
> +STATIC CONST EFI_GUID mSevInitrdHashGuid = SEV_INITRD_HASH_GUID;
> +STATIC CONST EFI_GUID mSevCmdlineHashGuid = SEV_CMDLINE_HASH_GUID;
> +
> +#pragma pack (1)
> +typedef struct {
> +  GUID   Guid;
> +  UINT16 Len;
> +  UINT8  Data[];
> +} HASH_TABLE;
> +#pragma pack ()
> +
> +STATIC HASH_TABLE *mHashesTable;
> +STATIC UINT16 mHashesTableSize;
> +
> +STATIC
> +CONST GUID*
> +FindBlobEntryGuid (
> +  IN  CONST CHAR16    *BlobName
> +  )
> +{
> +  if (StrCmp (BlobName, L"kernel") == 0) {
> +    return &mSevKernelHashGuid;
> +  } else if (StrCmp (BlobName, L"initrd") == 0) {
> +    return &mSevInitrdHashGuid;
> +  } else if (StrCmp (BlobName, L"cmdline") == 0) {
> +    return &mSevCmdlineHashGuid;
> +  } else {
> +    return NULL;
> +  }
> +}
> +
> +/**
> +  Verify blob from an external source.
> +
> +  @param BlobName               The name of the blob
> +  @param Buf                    The data of the blob
> +  @param BufSize                The size of the blob in bytes
> +
> +  @retval EFI_SUCCESS           The blob was verified successfully.
> +  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore
> +                                should be considered non-secure.
> +**/
> +EFI_STATUS
> +EFIAPI
> +VerifyBlob (
> +  IN  CONST CHAR16    *BlobName,
> +  IN  CONST VOID      *Buf,
> +  UINT32              BufSize
> +  )
> +{
> +  CONST GUID *Guid;
> +  INT32 Len;

Any reason for this not to be a UINT16 like the struct or mHashesTableSize?

> +  HASH_TABLE *Entry;
> +  UINT8 Hash[SHA256_DIGEST_SIZE];
> +
> +  if (mHashesTable == NULL || mHashesTableSize == 0) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Verifier called but no hashes table discoverd in MEMFD\n",
> +      __FUNCTION__));
> +    return EFI_ACCESS_DENIED;
> +  }
> +
> +  Guid = FindBlobEntryGuid (BlobName);
> +  if (Guid == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Unknown blob name \"%s\"\n", __FUNCTION__,
> +      BlobName));
> +    return EFI_ACCESS_DENIED;
> +  }
> +
> +  Sha256HashAll (Buf, BufSize, Hash);

Maybe search for and find the Guid (done in the for loop below) before
calling Sha256HashAll?

Thanks,
Tom

> +
> +  for (Entry = mHashesTable, Len = 0;
> +       Len < (INT32)mHashesTableSize;
> +       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 calculated hash is identical to the expected
> +    // 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 for \"%s\"\n",
> +        __FUNCTION__, BlobName));
> +    } else {
> +      Status = EFI_ACCESS_DENIED;
> +      DEBUG ((DEBUG_ERROR, "%a: Hash comparison failed for \"%s\"\n",
> +        __FUNCTION__, BlobName));
> +    }
> +    return Status;
> +  }
> +
> +  DEBUG ((DEBUG_ERROR, "%a: Hash GUID %g not found in table\n", __FUNCTION__,
> +    Guid));
> +  return EFI_ACCESS_DENIED;
> +}
> +
> +/**
> +  Locate the SEV hashes table.
> +
> +  This function always returns success, even if the table can't be found.  The
> +  subsequent VerifyBlob calls will fail if no table was found.
> +
> +  @retval RETURN_SUCCESS   The verifier tables were set up correctly
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SevHashesBlobVerifierLibConstructor (
> +  VOID
> +  )
> +{
> +  HASH_TABLE *Ptr = (void *)(UINTN)FixedPcdGet64 (PcdQemuHashTableBase);
> +  UINT32 Size = FixedPcdGet32 (PcdQemuHashTableSize);
> +
> +  mHashesTable = NULL;
> +  mHashesTableSize = 0;
> +
> +  if (Ptr == NULL || Size == 0) {
> +    return RETURN_SUCCESS;
> +  }
> +
> +  if (!CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID)) {
> +    return RETURN_SUCCESS;
> +  }
> +
> +  if (Ptr->Len < (sizeof Ptr->Guid + sizeof Ptr->Len)) {
> +    return RETURN_SUCCESS;
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "%a: Found injected hashes table in secure location\n",
> +    __FUNCTION__));
> +
> +  mHashesTable = (HASH_TABLE *)Ptr->Data;
> +  mHashesTableSize = Ptr->Len - sizeof Ptr->Guid - sizeof Ptr->Len;
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: mHashesTable=0x%p, Size=%u\n", __FUNCTION__,
> +    mHashesTable, mHashesTableSize));
> +
> +  return RETURN_SUCCESS;
> +}
> 

Re: [PATCH v2 11/11] OvmfPkg/AmdSev: Enforce hash verification of kernel blobs

[Lendacky, Thomas]

On 7/6/21 3:55 AM, Dov Murik wrote:
> In the AmdSevX86 build, use SevHashesBlobVerifierLib to enforce
> verification of hashes of the kernel/initrd/cmdline blobs fetched from
> firmware config.
> 
> This allows for secure (measured) boot of SEV guests with QEMU's
> -kernel/-initrd/-append switches (with the corresponding QEMU support
> for injecting the hashes table into initial measured guest memory).
> 
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  OvmfPkg/AmdSev/AmdSevX64.dsc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index 8b260df114e3..d1ed0abbd0fb 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -173,7 +173,7 @@ [LibraryClasses]
>    LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>    CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf
>    FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf
> -  BlobVerifierLib|OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
> +  BlobVerifierLib|OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
>  
>  !if $(SOURCE_DEBUG_ENABLE) == TRUE
>    PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLibDebug.inf
> @@ -696,7 +696,7 @@ [Components]
>    }
>    OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.inf {
>      <LibraryClasses>
> -      NULL|OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
> +      NULL|OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
>    }
>    OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
>    OvmfPkg/Virtio10Dxe/Virtio10.inf
> 

Re: [edk2-devel] [PATCH v2 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg

[Dov Murik]

On 19/07/2021 15:56, Christoph Willing wrote:
> Thanks for the clarification Dov.
> 
> I've been trying with just "normal" VMs, not SEV. I did already find and try the confidential-containers-demo sev-hashes-v2 branch but it didn't help - not surprising if it's not relevant to normal VMs.
> 
> Do you know whether this functionality (-kernel, -initrd, -append options) is actually supposed to work in normal VMs at the moment? The only conditions under which it works here with qemu-6.0.0 is with vUDK2017 & 2018 and an old ovmf binary package from kraxel.og dated 2017. Anything built from the edk2 master branch has failed when using those qemu options, although all the same builds work perfectly using the VMs' internal kernels & initrds. I've also extracted OVMF files from the current kraxel.org package as well as Ubuntu's (hirsute) package and these also fail the same way i.e. kernel boots and initrd works (loads modules) but then the VM filesystem doesn't seem to be found (no /dev/sdX exists to mount the filesystem root).
> 
> I guess this could be a qemu problem but since it works with some (old) udk/edk2 versions, I thought I'd look here first.
> 


Can you please try with edk2 commit d1fc3d7ef3cb - just before we did
some changes around this QEMU-interop code in OVMF?

Thanks,
Dov

> Thanks for any help or pointers,
> chris
> 

Re: [PATCH v2 00/11] Measured SEV boot with kernel/initrd/cmdline

[Dov Murik]

On 19/07/2021 18:14, Tom Lendacky wrote:
> On 7/6/21 3:54 AM, Dov Murik wrote:
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
> 
> This BZ link should be part of all the commit messages in the series.
> 

Oh I missed a few.  I'll fix.  Thanks.


> Thanks,
> Tom
> 
>>
>> 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 reserves an area in MEMFD (previously the last 1KB of
>> the launch secret page) 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 QEMU support [1].
>>
>> 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.
>>
>> Relevant part of OVMF serial log during boot with AmdSevX86 build and QEMU with
>> -kernel/-initrd/-append:
>>
>>   ...
>>   SevHashesBlobVerifierLibConstructor: found injected hashes table in secure location
>>   Select Item: 0x17
>>   Select Item: 0x8
>>   FetchBlob: loading 7379328 bytes for "kernel"
>>   Select Item: 0x18
>>   Select Item: 0x11
>>   VerifyBlob: Found GUID 4DE79437-ABD2-427F-B835-D5B172D2045B in table
>>   VerifyBlob: Hash comparison succeeded for entry 'kernel'
>>   Select Item: 0xB
>>   FetchBlob: loading 12483878 bytes for "initrd"
>>   Select Item: 0x12
>>   VerifyBlob: Found GUID 44BAF731-3A2F-4BD7-9AF1-41E29169781D in table
>>   VerifyBlob: Hash comparison succeeded for entry 'initrd'
>>   Select Item: 0x14
>>   FetchBlob: loading 86 bytes for "cmdline"
>>   Select Item: 0x15
>>   VerifyBlob: Found GUID 97D02DD8-BD20-4C94-AA78-E7714D36AB2A in table
>>   VerifyBlob: Hash comparison succeeded for entry 'cmdline'
>>   ...
>>
>> The patch series is organized as follows:
>>
>> 1:     Simple comment fix in adjacent area in the code.
>> 2:     Use GenericQemuLoadImageLib to gain one location for fw_cfg blob
>>        fetching.
>> 3:     Allow the (previously blocked) usage of -kernel in AmdSevX64.
>> 4-7:   Add BlobVerifierLib with null implementation and use it in the correct
>>        location in QemuKernelLoaderFsDxe.
>> 8-9:   Reserve memory for hashes table, declare this area in the reset vector.
>> 10-11: Add the secure implementation SevHashesBlobVerifierLib and use it in
>>        AmdSevX64 builds.
>>
>> [1] https://lore.kernel.org/qemu-devel/20210624102040.2015280-1-dovmurik@linux.ibm.com/
>>
>> Code is at
>> https://github.com/confidential-containers-demo/edk2/tree/sev-hashes-v2
>>
>> v2 changes:
>>  - Use the last 1KB of the existing SEV launch secret page for hashes table
>>    (instead of reserving a whole new MEMFD page).
>>  - Build on top of commit cf203024745f ("OvmfPkg/GenericQemuLoadImageLib: Read
>>    cmdline from QemuKernelLoaderFs", 2021-06-28) to have a single location in
>>    which all of kernel/initrd/cmdline are fetched from QEMU.
>>  - Use static linking of the two BlobVerifierLib implemenatations.
>>  - Reorganize series.
>>
>> v1: https://edk2.groups.io/g/devel/message/75567
>>
>> 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>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>
>> Dov Murik (8):
>>   OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
>>   OvmfPkg: add library class BlobVerifierLib with null implementation
>>   OvmfPkg: add NullBlobVerifierLib to DSC
>>   ArmVirtPkg: add NullBlobVerifierLib to DSC
>>   OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
>>   OvmfPkg/AmdSev/SecretPei: build hob for full page
>>   OvmfPkg: add SevHashesBlobVerifierLib
>>   OvmfPkg/AmdSev: Enforce hash verification of kernel blobs
>>
>> James Bottomley (3):
>>   OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
>>   OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
>>   OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes
>>
>>  OvmfPkg/OvmfPkg.dec                                                                 |   9 +
>>  ArmVirtPkg/ArmVirtQemu.dsc                                                          |   5 +-
>>  ArmVirtPkg/ArmVirtQemuKernel.dsc                                                    |   5 +-
>>  OvmfPkg/AmdSev/AmdSevX64.dsc                                                        |   9 +-
>>  OvmfPkg/OvmfPkgIa32.dsc                                                             |   5 +-
>>  OvmfPkg/OvmfPkgIa32X64.dsc                                                          |   5 +-
>>  OvmfPkg/OvmfPkgX64.dsc                                                              |   5 +-
>>  OvmfPkg/AmdSev/AmdSevX64.fdf                                                        |   5 +-
>>  OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf                             |  27 +++
>>  OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf                        |  36 ++++
>>  OvmfPkg/Library/PlatformBootManagerLibGrub/PlatformBootManagerLibGrub.inf           |   2 +
>>  OvmfPkg/ResetVector/ResetVector.inf                                                 |   2 +
>>  OvmfPkg/Include/Library/BlobVerifierLib.h                                           |  38 ++++
>>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.h                            |  11 ++
>>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                                                |   2 +-
>>  OvmfPkg/AmdSev/SecretPei/SecretPei.c                                                |   9 +-
>>  OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c                                  |  34 ++++
>>  OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c                             | 199 ++++++++++++++++++++
>>  OvmfPkg/Library/PlatformBootManagerLibGrub/BdsPlatform.c                            |   5 +
>>  OvmfPkg/Library/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c |   0
>>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c                               |   9 +
>>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm                                        |  20 ++
>>  OvmfPkg/ResetVector/ResetVector.nasmb                                               |   2 +
>>  23 files changed, 434 insertions(+), 10 deletions(-)
>>  create mode 100644 OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
>>  create mode 100644 OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
>>  create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h
>>  create mode 100644 OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c
>>  create mode 100644 OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
>>  copy OvmfPkg/Library/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c (100%)
>>

Re: [PATCH v2 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg

[Dov Murik]

On 19/07/2021 18:21, Tom Lendacky wrote:
> On 7/6/21 3:54 AM, Dov Murik wrote:
>> 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 .
> 
> Just a nit, but this confused me initially. Maybe it should say something
> along the lines of create a QemuKernel.c for PlatformBootManagerLibGrub
> that is an exact copy of the file from PlatformBootManagerLib.
> 

You're right; I'll write it clearer.


> Is there any way that the two libraries can use the same file rather than
> making an exact copy?

I guess it's possible by extracting the file into its own library?  I'll
need to take a deeper look.

-Dov


> 
> Thanks,
> Tom
> 
>>
>> 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>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
>> 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/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c |  0
>>  5 files changed, 19 insertions(+)
>>
>> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
>> index a2f1324c40a6..aefdcf881c99 100644
>> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
>> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
>> @@ -281,6 +281,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/PlatformBootManagerLib/QemuKernel.c b/OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
>> similarity index 100%
>> copy from OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
>> copy to OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
>>

Re: [PATCH v2 04/11] OvmfPkg: add library class BlobVerifierLib with null implementation

[Dov Murik]

On 19/07/2021 18:50, Tom Lendacky wrote:
> On 7/6/21 3:54 AM, Dov Murik wrote:
>> BlobVerifierLib will be used to verify blobs fetching them from QEMU's
>> firmware config (fw_cfg) in platforms that enable such verification.
>>
>> The null implementation NullBlobVerifierLib treats all blobs as valid.
>>
>> 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>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  OvmfPkg/OvmfPkg.dec                                     |  3 ++
>>  OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf | 27 ++++++++++++++
>>  OvmfPkg/Include/Library/BlobVerifierLib.h               | 38 ++++++++++++++++++++
>>  OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c      | 34 ++++++++++++++++++
>>  4 files changed, 102 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> index 6ae733f6e39f..f82228d69cc2 100644
>> --- a/OvmfPkg/OvmfPkg.dec
>> +++ b/OvmfPkg/OvmfPkg.dec
>> @@ -23,6 +23,9 @@ [LibraryClasses]
>>    ##  @libraryclass  Access bhyve's firmware control interface.
>>    BhyveFwCtlLib|Include/Library/BhyveFwCtlLib.h
>>  
>> +  ##  @libraryclass  Verify blobs read from the VMM
>> +  BlobVerifierLib|Include/Library/BlobVerifierLib.h
>> +
>>    ##  @libraryclass  Loads and boots a Linux kernel image
>>    #
>>    LoadLinuxLib|Include/Library/LoadLinuxLib.h
>> diff --git a/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf b/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
>> new file mode 100644
>> index 000000000000..c8942ad05d96
>> --- /dev/null
>> +++ b/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifierLib.inf
>> @@ -0,0 +1,27 @@
>> +## @file
>> +#
>> +#  Null implementation of the blob verifier library.
>> +#
>> +#  Copyright (C) 2021, IBM Corp
>> +#
>> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010005
> 
> You can specify the INF_VERSION using x.y format now, and I believe the
> latest is 1.29.

Thanks, I'll change that.

> 
>> +  BASE_NAME                      = NullBlobVerifierLib
> 
> Typically, the NULL libraries would be named BlobVerifierLibNull.

You're right; I'll rename.


> 
>> +  FILE_GUID                      = b1b5533e-e01a-43bb-9e54-414f00ca036e
>> +  MODULE_TYPE                    = BASE
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = BlobVerifierLib
>> +
>> +[Sources]
>> +  NullBlobVerifier.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  OvmfPkg/OvmfPkg.dec
>> +
>> +[LibraryClasses]
>> +  DebugLib
> 
> Is this library (and associated include below) needed?

Probably not; I'll remove.

> 
>> diff --git a/OvmfPkg/Include/Library/BlobVerifierLib.h b/OvmfPkg/Include/Library/BlobVerifierLib.h
>> new file mode 100644
>> index 000000000000..667024766681
>> --- /dev/null
>> +++ b/OvmfPkg/Include/Library/BlobVerifierLib.h
>> @@ -0,0 +1,38 @@
>> +/** @file
>> +
>> +  Blob verification library
>> +
>> +  This library class allows verifiying whether blobs from external sources
>> +  (such as QEMU's firmware config) are trusted.
>> +
>> +  Copyright (C) 2021, IBM Corporation
>> +
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +**/
>> +
>> +#ifndef BLOB_VERIFIER_LIB_H__
>> +#define BLOB_VERIFIER_LIB_H__
>> +
>> +#include <Uefi/UefiBaseType.h>
>> +#include <Base.h>
>> +
>> +/**
>> +  Verify blob from an external source.
>> +
>> +  @param BlobName               The name of the blob
> 
> I believe this is supposed to be @param[in]
> 

OK.

>> +  @param Buf                    The data of the blob
>> +  @param BufSize                The size of the blob in bytes
>> +
>> +  @retval EFI_SUCCESS           The blob was verified successfully.
>> +  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore
>> +                                should be considered non-secure.
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +VerifyBlob (
>> +  IN  CONST CHAR16    *BlobName,
>> +  IN  CONST VOID      *Buf,
>> +  UINT32              BufSize
> 
> Missing "IN" here (same below for these).
> 

You're right. I'll add it.

Thanks,
-Dov


> Thanks,
> Tom
> 
>> +  );
>> +
>> +#endif
>> diff --git a/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c b/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c
>> new file mode 100644
>> index 000000000000..7b31b6ec767d
>> --- /dev/null
>> +++ b/OvmfPkg/Library/BlobVerifierLib/NullBlobVerifier.c
>> @@ -0,0 +1,34 @@
>> +/** @file
>> +
>> +  Null implementation of the blob verifier library.
>> +
>> +  Copyright (C) 2021, IBM Corporation
>> +
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +**/
>> +
>> +#include <Library/BaseLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/BlobVerifierLib.h>
>> +
>> +/**
>> +  Verify blob from an external source.
>> +
>> +  @param BlobName               The name of the blob
>> +  @param Buf                    The data of the blob
>> +  @param BufSize                The size of the blob in bytes
>> +
>> +  @retval EFI_SUCCESS           The blob was verified successfully.
>> +  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore
>> +                                should be considered non-secure.
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +VerifyBlob (
>> +  IN  CONST CHAR16    *BlobName,
>> +  IN  CONST VOID      *Buf,
>> +  UINT32              BufSize
>> +  )
>> +{
>> +  return EFI_SUCCESS;
>> +}
>>

Re: [PATCH v2 07/11] OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg

[Dov Murik]

On 19/07/2021 18:57, Tom Lendacky wrote:
> On 7/6/21 3:54 AM, Dov Murik wrote:
>> In QemuKernelLoaderFsDxeEntrypoint we use FetchBlob to read the content
>> of the kernel/initrd/cmdline from the QEMU fw_cfg interface.  Insert a
>> call to VerifyBlob after fetching to allow BlobVerifierLib
>> implementations to add a verification step for these blobs.
>>
>> This will allow confidential computing OVMF builds to add verification
>> mechanisms for these blobs that originate from an untrusted source
>> (QEMU).
>>
>> The null implementation of BlobVerifierLib does nothing in VerifyBlob,
>> and therefore no functional change is expected.
>>
>> 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>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
>> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
>> index c7ddd86f5c75..b43330d23b80 100644
>> --- a/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
>> +++ b/OvmfPkg/QemuKernelLoaderFsDxe/QemuKernelLoaderFsDxe.c
>> @@ -17,6 +17,7 @@
>>  #include <Guid/QemuKernelLoaderFsMedia.h>
>>  #include <Library/BaseLib.h>
>>  #include <Library/BaseMemoryLib.h>
>> +#include <Library/BlobVerifierLib.h>
>>  #include <Library/DebugLib.h>
>>  #include <Library/DevicePathLib.h>
>>  #include <Library/MemoryAllocationLib.h>
>> @@ -1039,6 +1040,14 @@ QemuKernelLoaderFsDxeEntrypoint (
>>      if (EFI_ERROR (Status)) {
>>        goto FreeBlobs;
>>      }
>> +    Status = VerifyBlob (
>> +               CurrentBlob->Name,
>> +               CurrentBlob->Data,
>> +               CurrentBlob->Size
>> +             );
> 
> Just a nit, but the ");" should be under the "C" in CurrentBlob.

It's a sad winking face... I'll fix.

Thanks,
-Dov


> 
> Thanks,
> Tom
> 
>> +    if (EFI_ERROR (Status)) {
>> +      goto FreeBlobs;
>> +    }
>>      mTotalBlobBytes += CurrentBlob->Size;
>>    }
>>    KernelBlob      = &mKernelBlob[KernelBlobTypeKernel];
>>

Re: [PATCH v2 08/11] OvmfPkg/AmdSev/SecretPei: build hob for full page

[Dov Murik]

On 19/07/2021 19:19, Tom Lendacky wrote:
> On 7/6/21 3:54 AM, Dov Murik wrote:
>> Round up the size of the SEV launch secret area to a whole page, as
>> required by BuildMemoryAllocationHob.  This will allow the secret
>> area defined in the MEMFD to take less than a whole 4KB page.
>>
>> 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>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  OvmfPkg/AmdSev/SecretPei/SecretPei.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>> index ad491515dd5d..db4267428e5a 100644
>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>> @@ -15,9 +15,16 @@ InitializeSecretPei (
>>    IN CONST EFI_PEI_SERVICES     **PeiServices
>>    )
>>  {
>> +  UINT64 RoundedSize;
>> +
>> +  RoundedSize = PcdGet32 (PcdSevLaunchSecretSize);
> 
> Can you just unconditionally perform:
> 
>   RoundedSize = ALIGN_VALUE (RoundedSize, EFI_PAGE_SIZE);
> 
> Or use ALIGN_VALUE () in the if statement if you don't want to do it
> unconditionally?
> 
> Or even use ALIGN_VALUE on size value in the BuildMemoryAllocationHob()
> call below.

Yes, that's much better.  Thanks for introducing me to this macro.

-Dov


> 
> Thanks,
> Tom
> 
>> +  if (RoundedSize % EFI_PAGE_SIZE != 0) {
>> +    RoundedSize = (RoundedSize / EFI_PAGE_SIZE + 1) * EFI_PAGE_SIZE;
>> +  }
>> +
>>    BuildMemoryAllocationHob (
>>      PcdGet32 (PcdSevLaunchSecretBase),
>> -    PcdGet32 (PcdSevLaunchSecretSize),
>> +    RoundedSize,
>>      EfiBootServicesData
>>      );
>>  
>>

Re: [PATCH v2 10/11] OvmfPkg: add SevHashesBlobVerifierLib

[Dov Murik]

On 19/07/2021 20:28, Tom Lendacky wrote:
> On 7/6/21 3:55 AM, Dov Murik wrote:
>> Add an implementation for BlobVerifierLib that locates the SEV hashes
>> table and verifies that the calculated hashes of the kernel, initrd, and
>> cmdline blobs indeed match the expected hashes stated in the hashes
>> table.
>>
>> If there's a missing hash or a hash mismatch then EFI_ACCESS_DENIED is
>> returned which will cause a failure to load a kernel image.
>>
>> 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>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457
>> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf |  36 ++++
>>  OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c      | 199 ++++++++++++++++++++
>>  2 files changed, 235 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
>> new file mode 100644
>> index 000000000000..b060d6a1b545
>> --- /dev/null
>> +++ b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifierLib.inf
>> @@ -0,0 +1,36 @@
>> +## @file
>> +#
>> +#  Blob verifier library that uses SEV hashes table.
>> +#
>> +#  Copyright (C) 2021, IBM Corp
>> +#
>> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +##
>> +
> 
> Same comments here as were made on the Null library instance.
> 

Indeed.


>> +[Defines]
>> +  INF_VERSION                    = 0x00010005
>> +  BASE_NAME                      = SevHashesBlobVerifierLib

But is this BASE_NAME okay?

Or should it be BlobVerifierLibSevHashes ?


>> +  FILE_GUID                      = 59e713b5-eff3-46a7-8d8b-46f4c004ad7b
>> +  MODULE_TYPE                    = BASE
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = BlobVerifierLib
>> +  CONSTRUCTOR                    = SevHashesBlobVerifierLibConstructor
>> +
>> +[Sources]
>> +  SevHashesBlobVerifier.c
>> +
>> +[Packages]
>> +  CryptoPkg/CryptoPkg.dec
>> +  MdePkg/MdePkg.dec
>> +  OvmfPkg/OvmfPkg.dec
>> +
>> +[LibraryClasses]
>> +  BaseCryptLib
>> +  BaseMemoryLib
>> +  DebugLib
>> +  PcdLib
>> +
>> +[FixedPcd]
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
>> diff --git a/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
>> new file mode 100644
>> index 000000000000..961ee29f5df3
>> --- /dev/null
>> +++ b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
>> @@ -0,0 +1,199 @@
>> +/** @file
>> +
>> +  Blob verifier library that uses SEV hashes table.
>> +
>> +  Copyright (C) 2021, IBM Corporation
>> +
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +**/
>> +
>> +#include <Library/BaseCryptLib.h>
>> +#include <Library/BaseLib.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/BlobVerifierLib.h>
>> +
>> +/**
>> +  The SEV Hashes 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 } }
>> +
>> +STATIC CONST EFI_GUID mSevKernelHashGuid = SEV_KERNEL_HASH_GUID;
>> +STATIC CONST EFI_GUID mSevInitrdHashGuid = SEV_INITRD_HASH_GUID;
>> +STATIC CONST EFI_GUID mSevCmdlineHashGuid = SEV_CMDLINE_HASH_GUID;
>> +
>> +#pragma pack (1)
>> +typedef struct {
>> +  GUID   Guid;
>> +  UINT16 Len;
>> +  UINT8  Data[];
>> +} HASH_TABLE;
>> +#pragma pack ()
>> +
>> +STATIC HASH_TABLE *mHashesTable;
>> +STATIC UINT16 mHashesTableSize;
>> +
>> +STATIC
>> +CONST GUID*
>> +FindBlobEntryGuid (
>> +  IN  CONST CHAR16    *BlobName
>> +  )
>> +{
>> +  if (StrCmp (BlobName, L"kernel") == 0) {
>> +    return &mSevKernelHashGuid;
>> +  } else if (StrCmp (BlobName, L"initrd") == 0) {
>> +    return &mSevInitrdHashGuid;
>> +  } else if (StrCmp (BlobName, L"cmdline") == 0) {
>> +    return &mSevCmdlineHashGuid;
>> +  } else {
>> +    return NULL;
>> +  }
>> +}
>> +
>> +/**
>> +  Verify blob from an external source.
>> +
>> +  @param BlobName               The name of the blob
>> +  @param Buf                    The data of the blob
>> +  @param BufSize                The size of the blob in bytes
>> +
>> +  @retval EFI_SUCCESS           The blob was verified successfully.
>> +  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore
>> +                                should be considered non-secure.
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +VerifyBlob (
>> +  IN  CONST CHAR16    *BlobName,
>> +  IN  CONST VOID      *Buf,
>> +  UINT32              BufSize
>> +  )
>> +{
>> +  CONST GUID *Guid;
>> +  INT32 Len;
> 
> Any reason for this not to be a UINT16 like the struct or mHashesTableSize?
> 

Detect overflows in the `for` loop below?

If a (bad) Entry->Len is 0xffff, then adding it to Len will overflow the
UINT16 and the Len < mHashesTableSize condition is still true.


>> +  HASH_TABLE *Entry;
>> +  UINT8 Hash[SHA256_DIGEST_SIZE];
>> +
>> +  if (mHashesTable == NULL || mHashesTableSize == 0) {
>> +    DEBUG ((DEBUG_ERROR,
>> +      "%a: Verifier called but no hashes table discoverd in MEMFD\n",
>> +      __FUNCTION__));
>> +    return EFI_ACCESS_DENIED;
>> +  }
>> +
>> +  Guid = FindBlobEntryGuid (BlobName);
>> +  if (Guid == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "%a: Unknown blob name \"%s\"\n", __FUNCTION__,
>> +      BlobName));
>> +    return EFI_ACCESS_DENIED;
>> +  }
>> +
>> +  Sha256HashAll (Buf, BufSize, Hash);
> 
> Maybe search for and find the Guid (done in the for loop below) before
> calling Sha256HashAll?
> 

Yep; I'll move it just before CompareMem below.

Thanks,
-Dov


> Thanks,
> Tom
> 
>> +
>> +  for (Entry = mHashesTable, Len = 0;
>> +       Len < (INT32)mHashesTableSize;
>> +       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 calculated hash is identical to the expected
>> +    // 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 for \"%s\"\n",
>> +        __FUNCTION__, BlobName));
>> +    } else {
>> +      Status = EFI_ACCESS_DENIED;
>> +      DEBUG ((DEBUG_ERROR, "%a: Hash comparison failed for \"%s\"\n",
>> +        __FUNCTION__, BlobName));
>> +    }
>> +    return Status;
>> +  }
>> +
>> +  DEBUG ((DEBUG_ERROR, "%a: Hash GUID %g not found in table\n", __FUNCTION__,
>> +    Guid));
>> +  return EFI_ACCESS_DENIED;
>> +}
>> +
>> +/**
>> +  Locate the SEV hashes table.
>> +
>> +  This function always returns success, even if the table can't be found.  The
>> +  subsequent VerifyBlob calls will fail if no table was found.
>> +
>> +  @retval RETURN_SUCCESS   The verifier tables were set up correctly
>> +**/
>> +RETURN_STATUS
>> +EFIAPI
>> +SevHashesBlobVerifierLibConstructor (
>> +  VOID
>> +  )
>> +{
>> +  HASH_TABLE *Ptr = (void *)(UINTN)FixedPcdGet64 (PcdQemuHashTableBase);
>> +  UINT32 Size = FixedPcdGet32 (PcdQemuHashTableSize);
>> +
>> +  mHashesTable = NULL;
>> +  mHashesTableSize = 0;
>> +
>> +  if (Ptr == NULL || Size == 0) {
>> +    return RETURN_SUCCESS;
>> +  }
>> +
>> +  if (!CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID)) {
>> +    return RETURN_SUCCESS;
>> +  }
>> +
>> +  if (Ptr->Len < (sizeof Ptr->Guid + sizeof Ptr->Len)) {
>> +    return RETURN_SUCCESS;
>> +  }
>> +
>> +  DEBUG ((DEBUG_INFO, "%a: Found injected hashes table in secure location\n",
>> +    __FUNCTION__));
>> +
>> +  mHashesTable = (HASH_TABLE *)Ptr->Data;
>> +  mHashesTableSize = Ptr->Len - sizeof Ptr->Guid - sizeof Ptr->Len;
>> +
>> +  DEBUG ((DEBUG_VERBOSE, "%a: mHashesTable=0x%p, Size=%u\n", __FUNCTION__,
>> +    mHashesTable, mHashesTableSize));
>> +
>> +  return RETURN_SUCCESS;
>> +}
>>

Re: [PATCH v2 07/11] OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg

[Dov Murik]

On 19/07/2021 18:19, Brijesh Singh wrote:
> 
> 
> On 7/19/21 7:22 AM, Dov Murik wrote:
>>> The patch itself is okay. Just curious, do we also need to add a
>>> verification for the QEMU FW cfg file ?
>>>
>>
>> I don't really understand.  This patch adds the VerifyBlob() call on
>> blobs that were read by FetchBlob(), which in turn reads the contents of
>> kernel/initrd/cmdline from QEMU FW cfg (using QemuFwCfgReadBytes for
>> example).
>>
>> We currently *don't* add verification for all other FW cfg settings,
>> like number of CPUs, E820 memory entries, ... similar to what we (don't)
>> do in SEV boot with encrypted root image (in which only OVMF is
>> measured).
>>
>> What else do you think we should verify?
>>
> 
> As I understand that your series is attempting to add more security
> checks in the SEV boot sequence; i.e. after this series is merged, we
> can verify the kernel,cmdline and initrd passed through qemu. But there
> are several other configuration parameters (such as e820, acpi) that
> gets passed by the qemu and consumed by the ovmf. Are you considering to
> add the checks to cover those blobs in the future series? To me it seems
> that the framework built here can be extended to cover those as well.
> 

You're right -- it can be extended.  Currently that's not the plan; the
Guest Owner should be able to verify the measurement, which, with this
patch series, is a combination of the OVMF, kernel, initrd, and cmdline.
Adding the other QEMU FW CFG values will make that even harder for the
Guest Owner.  Also, the measurement will be different if, for example,
the guest is launched with 8GB memory instead of 4GB, or with 8 vcpus
instead of 4 vcpus.  If there's an obvious attack possible via one of
those fw_cfg settings, we can think how to extend the measurement to
cover the problematic settings (or not support them at all, if possible).


> Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
> 

Thanks!
-Dov

Re: [PATCH v2 10/11] OvmfPkg: add SevHashesBlobVerifierLib

[Lendacky, Thomas]

On 7/19/21 2:47 PM, Dov Murik wrote:
> On 19/07/2021 20:28, Tom Lendacky wrote:
>> On 7/6/21 3:55 AM, Dov Murik wrote:
> 
>>> +[Defines]
>>> +  INF_VERSION                    = 0x00010005
>>> +  BASE_NAME                      = SevHashesBlobVerifierLib
> 
> But is this BASE_NAME okay?
> 
> Or should it be BlobVerifierLibSevHashes ?

I guess that it should probably be BlobVerifierLibSevHashes just for
consistency, but I'm not sure whether there's a convention for BASE_NAME.

Thanks,
Tom

> 
> 
>>> +  FILE_GUID                      = 59e713b5-eff3-46a7-8d8b-46f4c004ad7b
>>> +  MODULE_TYPE                    = BASE
>>> +  VERSION_STRING                 = 1.0
>>> +  LIBRARY_CLASS                  = BlobVerifierLib
>>> +  CONSTRUCTOR                    = SevHashesBlobVerifierLibConstructor
>>> +
>>> +[Sources]
>>> +  SevHashesBlobVerifier.c
>>> +
>>> +[Packages]
>>> +  CryptoPkg/CryptoPkg.dec
>>> +  MdePkg/MdePkg.dec
>>> +  OvmfPkg/OvmfPkg.dec
>>> +
>>> +[LibraryClasses]
>>> +  BaseCryptLib
>>> +  BaseMemoryLib
>>> +  DebugLib
>>> +  PcdLib
>>> +
>>> +[FixedPcd]
>>> +  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase
>>> +  gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
>>> diff --git a/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
>>> new file mode 100644
>>> index 000000000000..961ee29f5df3
>>> --- /dev/null
>>> +++ b/OvmfPkg/Library/BlobVerifierLib/SevHashesBlobVerifier.c
>>> @@ -0,0 +1,199 @@
>>> +/** @file
>>> +
>>> +  Blob verifier library that uses SEV hashes table.
>>> +
>>> +  Copyright (C) 2021, IBM Corporation
>>> +
>>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>>> +**/
>>> +
>>> +#include <Library/BaseCryptLib.h>
>>> +#include <Library/BaseLib.h>
>>> +#include <Library/BaseMemoryLib.h>
>>> +#include <Library/DebugLib.h>
>>> +#include <Library/BlobVerifierLib.h>
>>> +
>>> +/**
>>> +  The SEV Hashes 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 } }
>>> +
>>> +STATIC CONST EFI_GUID mSevKernelHashGuid = SEV_KERNEL_HASH_GUID;
>>> +STATIC CONST EFI_GUID mSevInitrdHashGuid = SEV_INITRD_HASH_GUID;
>>> +STATIC CONST EFI_GUID mSevCmdlineHashGuid = SEV_CMDLINE_HASH_GUID;
>>> +
>>> +#pragma pack (1)
>>> +typedef struct {
>>> +  GUID   Guid;
>>> +  UINT16 Len;
>>> +  UINT8  Data[];
>>> +} HASH_TABLE;
>>> +#pragma pack ()
>>> +
>>> +STATIC HASH_TABLE *mHashesTable;
>>> +STATIC UINT16 mHashesTableSize;
>>> +
>>> +STATIC
>>> +CONST GUID*
>>> +FindBlobEntryGuid (
>>> +  IN  CONST CHAR16    *BlobName
>>> +  )
>>> +{
>>> +  if (StrCmp (BlobName, L"kernel") == 0) {
>>> +    return &mSevKernelHashGuid;
>>> +  } else if (StrCmp (BlobName, L"initrd") == 0) {
>>> +    return &mSevInitrdHashGuid;
>>> +  } else if (StrCmp (BlobName, L"cmdline") == 0) {
>>> +    return &mSevCmdlineHashGuid;
>>> +  } else {
>>> +    return NULL;
>>> +  }
>>> +}
>>> +
>>> +/**
>>> +  Verify blob from an external source.
>>> +
>>> +  @param BlobName               The name of the blob
>>> +  @param Buf                    The data of the blob
>>> +  @param BufSize                The size of the blob in bytes
>>> +
>>> +  @retval EFI_SUCCESS           The blob was verified successfully.
>>> +  @retval EFI_ACCESS_DENIED     The blob could not be verified, and therefore
>>> +                                should be considered non-secure.
>>> +**/
>>> +EFI_STATUS
>>> +EFIAPI
>>> +VerifyBlob (
>>> +  IN  CONST CHAR16    *BlobName,
>>> +  IN  CONST VOID      *Buf,
>>> +  UINT32              BufSize
>>> +  )
>>> +{
>>> +  CONST GUID *Guid;
>>> +  INT32 Len;
>>
>> Any reason for this not to be a UINT16 like the struct or mHashesTableSize?
>>
> 
> Detect overflows in the `for` loop below?
> 
> If a (bad) Entry->Len is 0xffff, then adding it to Len will overflow the
> UINT16 and the Len < mHashesTableSize condition is still true.
> 
> 
>>> +  HASH_TABLE *Entry;
>>> +  UINT8 Hash[SHA256_DIGEST_SIZE];
>>> +
>>> +  if (mHashesTable == NULL || mHashesTableSize == 0) {
>>> +    DEBUG ((DEBUG_ERROR,
>>> +      "%a: Verifier called but no hashes table discoverd in MEMFD\n",
>>> +      __FUNCTION__));
>>> +    return EFI_ACCESS_DENIED;
>>> +  }
>>> +
>>> +  Guid = FindBlobEntryGuid (BlobName);
>>> +  if (Guid == NULL) {
>>> +    DEBUG ((DEBUG_ERROR, "%a: Unknown blob name \"%s\"\n", __FUNCTION__,
>>> +      BlobName));
>>> +    return EFI_ACCESS_DENIED;
>>> +  }
>>> +
>>> +  Sha256HashAll (Buf, BufSize, Hash);
>>
>> Maybe search for and find the Guid (done in the for loop below) before
>> calling Sha256HashAll?
>>
> 
> Yep; I'll move it just before CompareMem below.
> 
> Thanks,
> -Dov
> 
> 
>> Thanks,
>> Tom
>>
>>> +
>>> +  for (Entry = mHashesTable, Len = 0;
>>> +       Len < (INT32)mHashesTableSize;
>>> +       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 calculated hash is identical to the expected
>>> +    // 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 for \"%s\"\n",
>>> +        __FUNCTION__, BlobName));
>>> +    } else {
>>> +      Status = EFI_ACCESS_DENIED;
>>> +      DEBUG ((DEBUG_ERROR, "%a: Hash comparison failed for \"%s\"\n",
>>> +        __FUNCTION__, BlobName));
>>> +    }
>>> +    return Status;
>>> +  }
>>> +
>>> +  DEBUG ((DEBUG_ERROR, "%a: Hash GUID %g not found in table\n", __FUNCTION__,
>>> +    Guid));
>>> +  return EFI_ACCESS_DENIED;
>>> +}
>>> +
>>> +/**
>>> +  Locate the SEV hashes table.
>>> +
>>> +  This function always returns success, even if the table can't be found.  The
>>> +  subsequent VerifyBlob calls will fail if no table was found.
>>> +
>>> +  @retval RETURN_SUCCESS   The verifier tables were set up correctly
>>> +**/
>>> +RETURN_STATUS
>>> +EFIAPI
>>> +SevHashesBlobVerifierLibConstructor (
>>> +  VOID
>>> +  )
>>> +{
>>> +  HASH_TABLE *Ptr = (void *)(UINTN)FixedPcdGet64 (PcdQemuHashTableBase);
>>> +  UINT32 Size = FixedPcdGet32 (PcdQemuHashTableSize);
>>> +
>>> +  mHashesTable = NULL;
>>> +  mHashesTableSize = 0;
>>> +
>>> +  if (Ptr == NULL || Size == 0) {
>>> +    return RETURN_SUCCESS;
>>> +  }
>>> +
>>> +  if (!CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID)) {
>>> +    return RETURN_SUCCESS;
>>> +  }
>>> +
>>> +  if (Ptr->Len < (sizeof Ptr->Guid + sizeof Ptr->Len)) {
>>> +    return RETURN_SUCCESS;
>>> +  }
>>> +
>>> +  DEBUG ((DEBUG_INFO, "%a: Found injected hashes table in secure location\n",
>>> +    __FUNCTION__));
>>> +
>>> +  mHashesTable = (HASH_TABLE *)Ptr->Data;
>>> +  mHashesTableSize = Ptr->Len - sizeof Ptr->Guid - sizeof Ptr->Len;
>>> +
>>> +  DEBUG ((DEBUG_VERBOSE, "%a: mHashesTable=0x%p, Size=%u\n", __FUNCTION__,
>>> +    mHashesTable, mHashesTableSize));
>>> +
>>> +  return RETURN_SUCCESS;
>>> +}
>>>

Re: [edk2-devel] [PATCH v2 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg

[Christoph Willing]

On 20/7/21 3:58 am, Dov Murik wrote:
> 
> 
> On 19/07/2021 15:56, Christoph Willing wrote:
>> Thanks for the clarification Dov.
>>
>> I've been trying with just "normal" VMs, not SEV. I did already find and try the confidential-containers-demo sev-hashes-v2 branch but it didn't help - not surprising if it's not relevant to normal VMs.
>>
>> Do you know whether this functionality (-kernel, -initrd, -append options) is actually supposed to work in normal VMs at the moment? The only conditions under which it works here with qemu-6.0.0 is with vUDK2017 & 2018 and an old ovmf binary package from kraxel.og dated 2017. Anything built from the edk2 master branch has failed when using those qemu options, although all the same builds work perfectly using the VMs' internal kernels & initrds. I've also extracted OVMF files from the current kraxel.org package as well as Ubuntu's (hirsute) package and these also fail the same way i.e. kernel boots and initrd works (loads modules) but then the VM filesystem doesn't seem to be found (no /dev/sdX exists to mount the filesystem root).
>>
>> I guess this could be a qemu problem but since it works with some (old) udk/edk2 versions, I thought I'd look here first.
>>
> 
> 
> Can you please try with edk2 commit d1fc3d7ef3cb - just before we did
> some changes around this QEMU-interop code in OVMF?
> 

I just tried a build at d1fc3d7ef3cb... with the same result. Works with
VM's internal kernel & initrd but not with external (using -kernel,
-initrd & -append options).

As soon as I revert to OVMF files (CODE & VARS) from vUDK2018, all works
as expected with external kernel & initrd.

Since this problem seems to go back to around 2018, is it better to
report in bugzilla?

Thanks,
chris

Re: [edk2-devel] [PATCH v2 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg

[Dov Murik]

On 20/07/2021 1:36, Christoph Willing wrote:
> On 20/7/21 3:58 am, Dov Murik wrote:
>>
>>
>> On 19/07/2021 15:56, Christoph Willing wrote:
>>> Thanks for the clarification Dov.
>>>
>>> I've been trying with just "normal" VMs, not SEV. I did already find and try the confidential-containers-demo sev-hashes-v2 branch but it didn't help - not surprising if it's not relevant to normal VMs.
>>>
>>> Do you know whether this functionality (-kernel, -initrd, -append options) is actually supposed to work in normal VMs at the moment? The only conditions under which it works here with qemu-6.0.0 is with vUDK2017 & 2018 and an old ovmf binary package from kraxel.og dated 2017. Anything built from the edk2 master branch has failed when using those qemu options, although all the same builds work perfectly using the VMs' internal kernels & initrds. I've also extracted OVMF files from the current kraxel.org package as well as Ubuntu's (hirsute) package and these also fail the same way i.e. kernel boots and initrd works (loads modules) but then the VM filesystem doesn't seem to be found (no /dev/sdX exists to mount the filesystem root).
>>>
>>> I guess this could be a qemu problem but since it works with some (old) udk/edk2 versions, I thought I'd look here first.
>>>
>>
>>
>> Can you please try with edk2 commit d1fc3d7ef3cb - just before we did
>> some changes around this QEMU-interop code in OVMF?
>>
> 
> I just tried a build at d1fc3d7ef3cb... with the same result. Works with
> VM's internal kernel & initrd but not with external (using -kernel,
> -initrd & -append options).
> 
> As soon as I revert to OVMF files (CODE & VARS) from vUDK2018, all works
> as expected with external kernel & initrd.
> 
> Since this problem seems to go back to around 2018, is it better to
> report in bugzilla?

I think so.

Be sure to include full logs as much as possible and details about the
image you're trying to start; it seems to me that if the kernel starts
and initrd is mounted etc then both QEMU and OVMF are doing their part,
and there's something else that fails (but then again, reverting to an
old OVMF does solve it... IDK).

-Dov

> 
> Thanks,
> chris
> 

Re: [PATCH v2 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg

[Dov Murik]

On 19/07/2021 22:14, Dov Murik wrote:
> 
> 
> On 19/07/2021 18:21, Tom Lendacky wrote:
>> On 7/6/21 3:54 AM, Dov Murik wrote:
>>> 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 .
>>
>> Just a nit, but this confused me initially. Maybe it should say something
>> along the lines of create a QemuKernel.c for PlatformBootManagerLibGrub
>> that is an exact copy of the file from PlatformBootManagerLib.
>>
> 
> You're right; I'll write it clearer.
> 
> 
>> Is there any way that the two libraries can use the same file rather than
>> making an exact copy?
> 
> I guess it's possible by extracting the file into its own library?  I'll
> need to take a deeper look.
> 


With this patch we'll have two identical files:

  OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
  OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c

but there's another QemuKernel.c, which is *almost* identical:

  ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c

so a proper fix should consolidate all three into one library used by
all three libs.

I suggest postponing this to a separate refactoring series.

Thanks,
-Dov

Re: [PATCH v2 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg

[Ard Biesheuvel]

On Tue, 20 Jul 2021 at 09:33, Dov Murik <dovmurik@linux.ibm.com> wrote:
>
>
>
> On 19/07/2021 22:14, Dov Murik wrote:
> >
> >
> > On 19/07/2021 18:21, Tom Lendacky wrote:
> >> On 7/6/21 3:54 AM, Dov Murik wrote:
> >>> 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 .
> >>
> >> Just a nit, but this confused me initially. Maybe it should say something
> >> along the lines of create a QemuKernel.c for PlatformBootManagerLibGrub
> >> that is an exact copy of the file from PlatformBootManagerLib.
> >>
> >
> > You're right; I'll write it clearer.
> >
> >
> >> Is there any way that the two libraries can use the same file rather than
> >> making an exact copy?
> >
> > I guess it's possible by extracting the file into its own library?  I'll
> > need to take a deeper look.
> >
>
>
> With this patch we'll have two identical files:
>
>   OvmfPkg/Library/PlatformBootManagerLib/QemuKernel.c
>   OvmfPkg/Library/PlatformBootManagerLibGrub/QemuKernel.c
>
> but there's another QemuKernel.c, which is *almost* identical:
>
>   ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c
>
> so a proper fix should consolidate all three into one library used by
> all three libs.
>
> I suggest postponing this to a separate refactoring series.
>

That is fine with me.