* [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline
@ 2021-07-22 8:42 Dov Murik
2021-07-22 8:42 ` [PATCH v4 01/11] OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming Dov Murik
` (11 more replies)
0 siblings, 12 replies; 22+ messages in thread
From: Dov Murik @ 2021-07-22 8:42 UTC (permalink / raw)
To: devel
Cc: Dov Murik, Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum,
Jim Cadden, James Bottomley, Hubertus Franke, Ard Biesheuvel,
Jordan Justen, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Jiewen Yao, Min Xu, Tom Lendacky, Leif Lindholm, Sami Mujawar
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 these hashes are part of
the SEV measurement (which has to be approved by the Guest Owner for
secret injection, for example). Note that populating the hashes table
requires QEMU support [1].
OVMF parses the table of hashes populated by QEMU (patch 10), and as it
reads the fw_cfg blobs from QEMU, it will verify each one against the
expected hash. 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:
...
BlobVerifierLibSevHashesConstructor: 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 "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 "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 "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 BlobVerifierLibSevHashes 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-v4
v4 changes:
- BlobVerifierSevHashes (patch 10): more comprehensive overflow tests
when parsing the SEV hashes table structure
v3: https://edk2.groups.io/g/devel/message/77955
v3 changes:
- Rename to BlobVerifierLibNull, use decimal INF_VERSION, remove unused
DebugLib reference, fix doxygen comments, add missing IN attribute
- Rename to BlobVerifierLibSevHashes, use decimal INF_VERSION, fix
doxygen comments, add missing IN attribute,
calculate buffer hash only when the guid is found in hashes table
- SecretPei: use ALIGN_VALUE to round the hob size
- Coding style fixes
- Add missing 'Ref:' in patch 1 commit message
- Fix phrasing and typos in commit messages
- Remove Cc: Laszlo from series
v2: https://edk2.groups.io/g/devel/message/77505
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: 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>
---
Ard: please review patch 6 (ArmVirtPkg). Thanks.
Tom, Brijesh: I'll also send the diff for patch 10. Thanks.
---
Dov Murik (8):
OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
OvmfPkg: add library class BlobVerifierLib with null implementation
OvmfPkg: add BlobVerifierLibNull to DSC
ArmVirtPkg: add BlobVerifierLibNull to DSC
OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
OvmfPkg/AmdSev/SecretPei: build hob for full page
OvmfPkg: add BlobVerifierLibSevHashes
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/BlobVerifierLibNull.inf | 24 +++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf | 37 ++++
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 | 3 +-
OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c | 33 ++++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.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, 425 insertions(+), 10 deletions(-)
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c
create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
copy OvmfPkg/Library/{PlatformBootManagerLib => PlatformBootManagerLibGrub}/QemuKernel.c (100%)
--
2.25.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4 01/11] OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming
2021-07-22 8:42 [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
@ 2021-07-22 8:42 ` Dov Murik
2021-07-22 8:42 ` [PATCH v4 02/11] OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds Dov Murik
` (10 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Dov Murik @ 2021-07-22 8:42 UTC (permalink / raw)
To: devel
Cc: Dov Murik, Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum,
Jim Cadden, James Bottomley, Hubertus Franke, Ard Biesheuvel,
Jordan Justen, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Jiewen Yao, Min Xu, Tom Lendacky
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: 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>
---
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 02/11] OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
2021-07-22 8:42 [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
2021-07-22 8:42 ` [PATCH v4 01/11] OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming Dov Murik
@ 2021-07-22 8:42 ` Dov Murik
2021-07-22 8:42 ` [PATCH v4 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg Dov Murik
` (9 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Dov Murik @ 2021-07-22 8:42 UTC (permalink / raw)
To: devel
Cc: Dov Murik, Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum,
Jim Cadden, James Bottomley, Hubertus Franke, Ard Biesheuvel,
Jordan Justen, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Jiewen Yao, Min Xu, Tom Lendacky
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/cmdline blobs will be added only to
the GenericQemuLoadImageLib implementation, so use that for SEV builds.
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>
---
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg
2021-07-22 8:42 [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
2021-07-22 8:42 ` [PATCH v4 01/11] OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming Dov Murik
2021-07-22 8:42 ` [PATCH v4 02/11] OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds Dov Murik
@ 2021-07-22 8:42 ` Dov Murik
2021-07-22 8:43 ` [PATCH v4 04/11] OvmfPkg: add library class BlobVerifierLib with null implementation Dov Murik
` (8 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Dov Murik @ 2021-07-22 8:42 UTC (permalink / raw)
To: devel
Cc: Dov Murik, Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum,
Jim Cadden, James Bottomley, Hubertus Franke, Ard Biesheuvel,
Jordan Justen, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Jiewen Yao, Min Xu, Tom Lendacky
From: James Bottomley <jejb@linux.ibm.com>
Support QEMU's -kernel option.
Create a QemuKernel.c for PlatformBootManagerLibGrub
which is an exact copy of the file
PlatformBootManagerLib/QemuKernel.c .
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>
---
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 04/11] OvmfPkg: add library class BlobVerifierLib with null implementation
2021-07-22 8:42 [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
` (2 preceding siblings ...)
2021-07-22 8:42 ` [PATCH v4 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg Dov Murik
@ 2021-07-22 8:43 ` Dov Murik
2021-07-22 8:43 ` [PATCH v4 05/11] OvmfPkg: add BlobVerifierLibNull to DSC Dov Murik
` (7 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Dov Murik @ 2021-07-22 8:43 UTC (permalink / raw)
To: devel
Cc: Dov Murik, Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum,
Jim Cadden, James Bottomley, Hubertus Franke, Ard Biesheuvel,
Jordan Justen, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Jiewen Yao, Min Xu, Tom Lendacky
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 BlobVerifierLibNull treats all blobs as valid.
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/OvmfPkg.dec | 3 ++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf | 24 +++++++++++++
OvmfPkg/Include/Library/BlobVerifierLib.h | 38 ++++++++++++++++++++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c | 33 +++++++++++++++++
4 files changed, 98 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/BlobVerifierLibNull.inf b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf
new file mode 100644
index 000000000000..850d398e65a4
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf
@@ -0,0 +1,24 @@
+## @file
+#
+# Null implementation of the blob verifier library.
+#
+# Copyright (C) 2021, IBM Corp
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 1.29
+ BASE_NAME = BlobVerifierLibNull
+ FILE_GUID = b1b5533e-e01a-43bb-9e54-414f00ca036e
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = BlobVerifierLib
+
+[Sources]
+ BlobVerifierNull.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
diff --git a/OvmfPkg/Include/Library/BlobVerifierLib.h b/OvmfPkg/Include/Library/BlobVerifierLib.h
new file mode 100644
index 000000000000..db122684f76c
--- /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[in] BlobName The name of the blob
+ @param[in] Buf The data of the blob
+ @param[in] 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,
+ IN UINT32 BufSize
+ );
+
+#endif
diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c
new file mode 100644
index 000000000000..975d4dd52f80
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c
@@ -0,0 +1,33 @@
+/** @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/BlobVerifierLib.h>
+
+/**
+ Verify blob from an external source.
+
+ @param[in] BlobName The name of the blob
+ @param[in] Buf The data of the blob
+ @param[in] 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,
+ IN UINT32 BufSize
+ )
+{
+ return EFI_SUCCESS;
+}
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 05/11] OvmfPkg: add BlobVerifierLibNull to DSC
2021-07-22 8:42 [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
` (3 preceding siblings ...)
2021-07-22 8:43 ` [PATCH v4 04/11] OvmfPkg: add library class BlobVerifierLib with null implementation Dov Murik
@ 2021-07-22 8:43 ` Dov Murik
2021-07-22 8:43 ` [PATCH v4 06/11] ArmVirtPkg: " Dov Murik
` (6 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Dov Murik @ 2021-07-22 8:43 UTC (permalink / raw)
To: devel
Cc: Dov Murik, Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum,
Jim Cadden, James Bottomley, Hubertus Franke, Ard Biesheuvel,
Jordan Justen, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Jiewen Yao, Min Xu, Tom Lendacky
This prepares the ground for calling VerifyBlob() in
QemuKernelLoaderFsDxe.
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 | 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..b2cc96cc5a97 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/BlobVerifierLibNull.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/BlobVerifierLibNull.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..7613abab6a7f 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/BlobVerifierLibNull.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..8b35aaf4b44c 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/BlobVerifierLibNull.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..0c95c74ad1a8 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/BlobVerifierLibNull.inf
+ }
OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
OvmfPkg/Virtio10Dxe/Virtio10.inf
OvmfPkg/VirtioBlkDxe/VirtioBlk.inf
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 06/11] ArmVirtPkg: add BlobVerifierLibNull to DSC
2021-07-22 8:42 [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
` (4 preceding siblings ...)
2021-07-22 8:43 ` [PATCH v4 05/11] OvmfPkg: add BlobVerifierLibNull to DSC Dov Murik
@ 2021-07-22 8:43 ` Dov Murik
2021-07-22 8:43 ` [PATCH v4 07/11] OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg Dov Murik
` (5 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Dov Murik @ 2021-07-22 8:43 UTC (permalink / raw)
To: devel
Cc: Dov Murik, Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum,
Jim Cadden, James Bottomley, Hubertus Franke, Ard Biesheuvel,
Leif Lindholm, Sami Mujawar, Jordan Justen, Ashish Kalra,
Brijesh Singh, Erdem Aktas, Jiewen Yao, Min Xu, Tom Lendacky
This prepares the ground for calling VerifyBlob() in
QemuKernelLoaderFsDxe.
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..bf8bb1ec9578 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/BlobVerifierLibNull.inf
+ }
#
# Networking stack
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index a542fcb157e9..af34cb47a12d 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/BlobVerifierLibNull.inf
+ }
#
# Networking stack
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 07/11] OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
2021-07-22 8:42 [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
` (5 preceding siblings ...)
2021-07-22 8:43 ` [PATCH v4 06/11] ArmVirtPkg: " Dov Murik
@ 2021-07-22 8:43 ` Dov Murik
2021-07-22 8:43 ` [PATCH v4 08/11] OvmfPkg/AmdSev/SecretPei: build hob for full page Dov Murik
` (4 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Dov Murik @ 2021-07-22 8:43 UTC (permalink / raw)
To: devel
Cc: Dov Murik, Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum,
Jim Cadden, James Bottomley, Hubertus Franke, Ard Biesheuvel,
Jordan Justen, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Jiewen Yao, Min Xu, Tom Lendacky
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: 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>
Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.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..6832d563bcb0 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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 08/11] OvmfPkg/AmdSev/SecretPei: build hob for full page
2021-07-22 8:42 [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
` (6 preceding siblings ...)
2021-07-22 8:43 ` [PATCH v4 07/11] OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg Dov Murik
@ 2021-07-22 8:43 ` Dov Murik
2021-07-22 8:43 ` [PATCH v4 09/11] OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes Dov Murik
` (3 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Dov Murik @ 2021-07-22 8:43 UTC (permalink / raw)
To: devel
Cc: Dov Murik, Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum,
Jim Cadden, James Bottomley, Hubertus Franke, Ard Biesheuvel,
Jordan Justen, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Jiewen Yao, Min Xu, Tom Lendacky
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: 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>
Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/AmdSev/SecretPei/SecretPei.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
index ad491515dd5d..db94c26b54d1 100644
--- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
+++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
@@ -4,6 +4,7 @@
Copyright (C) 2020 James Bottomley, IBM Corporation.
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
+#include <Base.h>
#include <PiPei.h>
#include <Library/HobLib.h>
#include <Library/PcdLib.h>
@@ -17,7 +18,7 @@ InitializeSecretPei (
{
BuildMemoryAllocationHob (
PcdGet32 (PcdSevLaunchSecretBase),
- PcdGet32 (PcdSevLaunchSecretSize),
+ ALIGN_VALUE (PcdGet32 (PcdSevLaunchSecretSize), EFI_PAGE_SIZE),
EfiBootServicesData
);
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 09/11] OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes
2021-07-22 8:42 [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
` (7 preceding siblings ...)
2021-07-22 8:43 ` [PATCH v4 08/11] OvmfPkg/AmdSev/SecretPei: build hob for full page Dov Murik
@ 2021-07-22 8:43 ` Dov Murik
2021-07-22 8:43 ` [PATCH v4 10/11] OvmfPkg: add BlobVerifierLibSevHashes Dov Murik
` (2 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Dov Murik @ 2021-07-22 8:43 UTC (permalink / raw)
To: devel
Cc: Dov Murik, Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum,
Jim Cadden, James Bottomley, Hubertus Franke, Ard Biesheuvel,
Jordan Justen, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Jiewen Yao, Min Xu, Tom Lendacky
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: 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>
Reviewed-by: Brijesh Singh <brijesh.singh@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"
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 10/11] OvmfPkg: add BlobVerifierLibSevHashes
2021-07-22 8:42 [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
` (8 preceding siblings ...)
2021-07-22 8:43 ` [PATCH v4 09/11] OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes Dov Murik
@ 2021-07-22 8:43 ` Dov Murik
2021-07-22 8:45 ` Dov Murik
2021-07-22 8:43 ` [PATCH v4 11/11] OvmfPkg/AmdSev: Enforce hash verification of kernel blobs Dov Murik
2021-07-25 2:43 ` [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Yao, Jiewen
11 siblings, 1 reply; 22+ messages in thread
From: Dov Murik @ 2021-07-22 8:43 UTC (permalink / raw)
To: devel
Cc: Dov Murik, Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum,
Jim Cadden, James Bottomley, Hubertus Franke, Ard Biesheuvel,
Jordan Justen, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Jiewen Yao, Min Xu, Tom Lendacky
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: 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/BlobVerifierLibSevHashes.inf | 37 ++++
OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c | 199 ++++++++++++++++++++
2 files changed, 236 insertions(+)
diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
new file mode 100644
index 000000000000..76ca0b8154ce
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
@@ -0,0 +1,37 @@
+## @file
+#
+# Blob verifier library that uses SEV hashes table. The hashes table holds the
+# allowed hashes of the kernel, initrd, and cmdline blobs.
+#
+# Copyright (C) 2021, IBM Corp
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 1.29
+ BASE_NAME = BlobVerifierLibSevHashes
+ FILE_GUID = 59e713b5-eff3-46a7-8d8b-46f4c004ad7b
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = BlobVerifierLib
+ CONSTRUCTOR = BlobVerifierLibSevHashesConstructor
+
+[Sources]
+ BlobVerifierSevHashes.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/BlobVerifierSevHashes.c b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
new file mode 100644
index 000000000000..372ae6f469e7
--- /dev/null
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
@@ -0,0 +1,199 @@
+/** @file
+
+ Blob verifier library that uses SEV hashes table. The hashes table holds the
+ allowed hashes of the kernel, initrd, and cmdline blobs.
+
+ 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[in] BlobName The name of the blob
+ @param[in] Buf The data of the blob
+ @param[in] 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,
+ IN UINT32 BufSize
+ )
+{
+ CONST GUID *Guid;
+ INT32 Remaining;
+ HASH_TABLE *Entry;
+
+ 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;
+ }
+
+ //
+ // Remaining is INT32 to catch underflow in case Entry->Len has a
+ // very high UINT16 value
+ //
+ for (Entry = mHashesTable, Remaining = mHashesTableSize;
+ Remaining >= sizeof *Entry && Remaining >= Entry->Len;
+ Remaining -= Entry->Len,
+ Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {
+ UINTN EntrySize;
+ EFI_STATUS Status;
+ UINT8 Hash[SHA256_DIGEST_SIZE];
+
+ if (!CompareGuid (&Entry->Guid, Guid)) {
+ continue;
+ }
+
+ DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__, Guid));
+
+ 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;
+ }
+
+ //
+ // Calculate the buffer's hash and verify that it is identical to the
+ // expected hash table entry
+ //
+ Sha256HashAll (Buf, BufSize, Hash);
+
+ 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 hashes table is set up correctly, or there is no
+ hashes table
+**/
+RETURN_STATUS
+EFIAPI
+BlobVerifierLibSevHashesConstructor (
+ VOID
+ )
+{
+ HASH_TABLE *Ptr = (void *)(UINTN)FixedPcdGet64 (PcdQemuHashTableBase);
+ UINT32 Size = FixedPcdGet32 (PcdQemuHashTableSize);
+
+ mHashesTable = NULL;
+ mHashesTableSize = 0;
+
+ if (Ptr == NULL || Size < sizeof *Ptr ||
+ !CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID) ||
+ Ptr->Len < sizeof *Ptr || Ptr->Len > Size) {
+ 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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 11/11] OvmfPkg/AmdSev: Enforce hash verification of kernel blobs
2021-07-22 8:42 [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
` (9 preceding siblings ...)
2021-07-22 8:43 ` [PATCH v4 10/11] OvmfPkg: add BlobVerifierLibSevHashes Dov Murik
@ 2021-07-22 8:43 ` Dov Murik
2021-07-25 2:43 ` [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Yao, Jiewen
11 siblings, 0 replies; 22+ messages in thread
From: Dov Murik @ 2021-07-22 8:43 UTC (permalink / raw)
To: devel
Cc: Dov Murik, Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum,
Jim Cadden, James Bottomley, Hubertus Franke, Ard Biesheuvel,
Jordan Justen, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Jiewen Yao, Min Xu, Tom Lendacky
In the AmdSevX64 build, use BlobVerifierLibSevHashes 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: 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 b2cc96cc5a97..c01599ea354f 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/BlobVerifierLibNull.inf
+ BlobVerifierLib|OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.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/BlobVerifierLibNull.inf
+ NULL|OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
}
OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
OvmfPkg/Virtio10Dxe/Virtio10.inf
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v4 10/11] OvmfPkg: add BlobVerifierLibSevHashes
2021-07-22 8:43 ` [PATCH v4 10/11] OvmfPkg: add BlobVerifierLibSevHashes Dov Murik
@ 2021-07-22 8:45 ` Dov Murik
2021-07-25 2:47 ` [edk2-devel] " Yao, Jiewen
0 siblings, 1 reply; 22+ messages in thread
From: Dov Murik @ 2021-07-22 8:45 UTC (permalink / raw)
To: devel
Cc: Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum, Jim Cadden,
James Bottomley, Hubertus Franke, Ard Biesheuvel, Jordan Justen,
Ashish Kalra, Brijesh Singh, Erdem Aktas, Jiewen Yao, Min Xu,
Tom Lendacky, Dov Murik
Here's the diff from the v3 of this patch. It's supposed to catch
more cases of bad length fields overflowing the reserved MEMFD space or
the declared length of the table.
diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
index 797d63d18067..372ae6f469e7 100644
--- a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
+++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
@@ -94,7 +94,7 @@ VerifyBlob (
)
{
CONST GUID *Guid;
- INT32 Len;
+ INT32 Remaining;
HASH_TABLE *Entry;
if (mHashesTable == NULL || mHashesTableSize == 0) {
@@ -111,9 +111,13 @@ VerifyBlob (
return EFI_ACCESS_DENIED;
}
- for (Entry = mHashesTable, Len = 0;
- Len < (INT32)mHashesTableSize;
- Len += Entry->Len,
+ //
+ // Remaining is INT32 to catch underflow in case Entry->Len has a
+ // very high UINT16 value
+ //
+ for (Entry = mHashesTable, Remaining = mHashesTableSize;
+ Remaining >= sizeof *Entry && Remaining >= Entry->Len;
+ Remaining -= Entry->Len,
Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {
UINTN EntrySize;
EFI_STATUS Status;
@@ -125,7 +129,7 @@ VerifyBlob (
DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__,
Guid));
- EntrySize = Entry->Len - sizeof (Entry->Guid) - sizeof (Entry->Len);
+ 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));
@@ -161,7 +165,8 @@ VerifyBlob (
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
+ @retval RETURN_SUCCESS The hashes table is set up correctly, or
there is no
+ hashes table
**/
RETURN_STATUS
EFIAPI
@@ -175,15 +180,9 @@ BlobVerifierLibSevHashesConstructor (
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)) {
+ if (Ptr == NULL || Size < sizeof *Ptr ||
+ !CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID) ||
+ Ptr->Len < sizeof *Ptr || Ptr->Len > Size) {
return RETURN_SUCCESS;
}
On 22/07/2021 11:43, 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: 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/BlobVerifierLibSevHashes.inf | 37 ++++
> OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c | 199 ++++++++++++++++++++
> 2 files changed, 236 insertions(+)
>
> diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
> new file mode 100644
> index 000000000000..76ca0b8154ce
> --- /dev/null
> +++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
> @@ -0,0 +1,37 @@
> +## @file
>
> +#
>
> +# Blob verifier library that uses SEV hashes table. The hashes table holds the
>
> +# allowed hashes of the kernel, initrd, and cmdline blobs.
>
> +#
>
> +# Copyright (C) 2021, IBM Corp
>
> +#
>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
>
> +#
>
> +##
>
> +
>
> +[Defines]
>
> + INF_VERSION = 1.29
>
> + BASE_NAME = BlobVerifierLibSevHashes
>
> + FILE_GUID = 59e713b5-eff3-46a7-8d8b-46f4c004ad7b
>
> + MODULE_TYPE = BASE
>
> + VERSION_STRING = 1.0
>
> + LIBRARY_CLASS = BlobVerifierLib
>
> + CONSTRUCTOR = BlobVerifierLibSevHashesConstructor
>
> +
>
> +[Sources]
>
> + BlobVerifierSevHashes.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/BlobVerifierSevHashes.c b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> new file mode 100644
> index 000000000000..372ae6f469e7
> --- /dev/null
> +++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> @@ -0,0 +1,199 @@
> +/** @file
>
> +
>
> + Blob verifier library that uses SEV hashes table. The hashes table holds the
>
> + allowed hashes of the kernel, initrd, and cmdline blobs.
>
> +
>
> + 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[in] BlobName The name of the blob
>
> + @param[in] Buf The data of the blob
>
> + @param[in] 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,
>
> + IN UINT32 BufSize
>
> + )
>
> +{
>
> + CONST GUID *Guid;
>
> + INT32 Remaining;
>
> + HASH_TABLE *Entry;
>
> +
>
> + 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;
>
> + }
>
> +
>
> + //
>
> + // Remaining is INT32 to catch underflow in case Entry->Len has a
>
> + // very high UINT16 value
>
> + //
>
> + for (Entry = mHashesTable, Remaining = mHashesTableSize;
>
> + Remaining >= sizeof *Entry && Remaining >= Entry->Len;
>
> + Remaining -= Entry->Len,
>
> + Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {
>
> + UINTN EntrySize;
>
> + EFI_STATUS Status;
>
> + UINT8 Hash[SHA256_DIGEST_SIZE];
>
> +
>
> + if (!CompareGuid (&Entry->Guid, Guid)) {
>
> + continue;
>
> + }
>
> +
>
> + DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__, Guid));
>
> +
>
> + 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;
>
> + }
>
> +
>
> + //
>
> + // Calculate the buffer's hash and verify that it is identical to the
>
> + // expected hash table entry
>
> + //
>
> + Sha256HashAll (Buf, BufSize, Hash);
>
> +
>
> + 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 hashes table is set up correctly, or there is no
>
> + hashes table
>
> +**/
>
> +RETURN_STATUS
>
> +EFIAPI
>
> +BlobVerifierLibSevHashesConstructor (
>
> + VOID
>
> + )
>
> +{
>
> + HASH_TABLE *Ptr = (void *)(UINTN)FixedPcdGet64 (PcdQemuHashTableBase);
>
> + UINT32 Size = FixedPcdGet32 (PcdQemuHashTableSize);
>
> +
>
> + mHashesTable = NULL;
>
> + mHashesTableSize = 0;
>
> +
>
> + if (Ptr == NULL || Size < sizeof *Ptr ||
>
> + !CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID) ||
>
> + Ptr->Len < sizeof *Ptr || Ptr->Len > Size) {
>
> + 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;
>
> +}
>
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline
2021-07-22 8:42 [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
` (10 preceding siblings ...)
2021-07-22 8:43 ` [PATCH v4 11/11] OvmfPkg/AmdSev: Enforce hash verification of kernel blobs Dov Murik
@ 2021-07-25 2:43 ` Yao, Jiewen
2021-07-25 7:52 ` Dov Murik
11 siblings, 1 reply; 22+ messages in thread
From: Yao, Jiewen @ 2021-07-25 2:43 UTC (permalink / raw)
To: devel@edk2.groups.io, dovmurik@linux.ibm.com
Cc: Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum, Jim Cadden,
James Bottomley, Hubertus Franke, Ard Biesheuvel,
Justen, Jordan L, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Xu, Min M, Tom Lendacky, Leif Lindholm, Sami Mujawar, Yao, Jiewen
Hi
I am reviewing this patch series. I am OK with most parts.
And I do have one question:
May I know what is criteria to put a SEV module to OvmfPkg\AmdSev or OvmfPkg directly?
My original understanding is:
If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf}, then it should be OvmfPkg.
If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf}, Then it should be in OvmfPkg\AmdSev.
Am I right?
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dov Murik
> Sent: Thursday, July 22, 2021 4:43 PM
> To: devel@edk2.groups.io
> Cc: Dov Murik <dovmurik@linux.ibm.com>; Tobin Feldman-Fitzthum
> <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum <tobin@ibm.com>; Jim
> Cadden <jcadden@ibm.com>; James Bottomley <jejb@linux.ibm.com>;
> Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami
> Mujawar <sami.mujawar@arm.com>
> Subject: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
> kernel/initrd/cmdline
>
> 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 these hashes are part of
> the SEV measurement (which has to be approved by the Guest Owner for
> secret injection, for example). Note that populating the hashes table
> requires QEMU support [1].
>
> OVMF parses the table of hashes populated by QEMU (patch 10), and as it
> reads the fw_cfg blobs from QEMU, it will verify each one against the
> expected hash. 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:
>
> ...
> BlobVerifierLibSevHashesConstructor: 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 "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 "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 "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 BlobVerifierLibSevHashes 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-v4
>
> v4 changes:
> - BlobVerifierSevHashes (patch 10): more comprehensive overflow tests
> when parsing the SEV hashes table structure
>
> v3: https://edk2.groups.io/g/devel/message/77955
> v3 changes:
> - Rename to BlobVerifierLibNull, use decimal INF_VERSION, remove unused
> DebugLib reference, fix doxygen comments, add missing IN attribute
> - Rename to BlobVerifierLibSevHashes, use decimal INF_VERSION, fix
> doxygen comments, add missing IN attribute,
> calculate buffer hash only when the guid is found in hashes table
> - SecretPei: use ALIGN_VALUE to round the hob size
> - Coding style fixes
> - Add missing 'Ref:' in patch 1 commit message
> - Fix phrasing and typos in commit messages
> - Remove Cc: Laszlo from series
>
> v2: https://edk2.groups.io/g/devel/message/77505
> 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: 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>
>
> ---
>
> Ard: please review patch 6 (ArmVirtPkg). Thanks.
>
> Tom, Brijesh: I'll also send the diff for patch 10. Thanks.
>
> ---
>
> Dov Murik (8):
> OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
> OvmfPkg: add library class BlobVerifierLib with null implementation
> OvmfPkg: add BlobVerifierLibNull to DSC
> ArmVirtPkg: add BlobVerifierLibNull to DSC
> OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
> OvmfPkg/AmdSev/SecretPei: build hob for full page
> OvmfPkg: add BlobVerifierLibSevHashes
> 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/BlobVerifierLibNull.inf | 24
> +++
> OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf | 37
> ++++
>
> 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 | 3 +-
> OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c | 33 ++++
> OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.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, 425 insertions(+), 10 deletions(-)
> create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf
> create mode 100644
> OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
> create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h
> create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c
> create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> copy OvmfPkg/Library/{PlatformBootManagerLib =>
> PlatformBootManagerLibGrub}/QemuKernel.c (100%)
>
> --
> 2.25.1
>
>
>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 10/11] OvmfPkg: add BlobVerifierLibSevHashes
2021-07-22 8:45 ` Dov Murik
@ 2021-07-25 2:47 ` Yao, Jiewen
0 siblings, 0 replies; 22+ messages in thread
From: Yao, Jiewen @ 2021-07-25 2:47 UTC (permalink / raw)
To: devel@edk2.groups.io, dovmurik@linux.ibm.com
Cc: Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum, Jim Cadden,
James Bottomley, Hubertus Franke, Ard Biesheuvel,
Justen, Jordan L, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Xu, Min M, Tom Lendacky
Hi Dov
If this library is only needed by AmdSev build, I feel it should be in AmdSev dir, instead of Ovmf dir. (See my question in previous email).
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dov Murik
> Sent: Thursday, July 22, 2021 4:45 PM
> To: devel@edk2.groups.io
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum
> <tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>; James Bottomley
> <jejb@linux.ibm.com>; Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Dov Murik <dovmurik@linux.ibm.com>
> Subject: Re: [edk2-devel] [PATCH v4 10/11] OvmfPkg: add
> BlobVerifierLibSevHashes
>
>
> Here's the diff from the v3 of this patch. It's supposed to catch
> more cases of bad length fields overflowing the reserved MEMFD space or
> the declared length of the table.
>
>
>
> diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> index 797d63d18067..372ae6f469e7 100644
> --- a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> +++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> @@ -94,7 +94,7 @@ VerifyBlob (
> )
> {
> CONST GUID *Guid;
> - INT32 Len;
> + INT32 Remaining;
> HASH_TABLE *Entry;
>
> if (mHashesTable == NULL || mHashesTableSize == 0) {
> @@ -111,9 +111,13 @@ VerifyBlob (
> return EFI_ACCESS_DENIED;
> }
>
> - for (Entry = mHashesTable, Len = 0;
> - Len < (INT32)mHashesTableSize;
> - Len += Entry->Len,
> + //
> + // Remaining is INT32 to catch underflow in case Entry->Len has a
> + // very high UINT16 value
> + //
> + for (Entry = mHashesTable, Remaining = mHashesTableSize;
> + Remaining >= sizeof *Entry && Remaining >= Entry->Len;
> + Remaining -= Entry->Len,
> Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {
> UINTN EntrySize;
> EFI_STATUS Status;
> @@ -125,7 +129,7 @@ VerifyBlob (
>
> DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__,
> Guid));
>
> - EntrySize = Entry->Len - sizeof (Entry->Guid) - sizeof (Entry->Len);
> + 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));
> @@ -161,7 +165,8 @@ VerifyBlob (
> 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
> + @retval RETURN_SUCCESS The hashes table is set up correctly, or
> there is no
> + hashes table
> **/
> RETURN_STATUS
> EFIAPI
> @@ -175,15 +180,9 @@ BlobVerifierLibSevHashesConstructor (
> 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)) {
> + if (Ptr == NULL || Size < sizeof *Ptr ||
> + !CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID) ||
> + Ptr->Len < sizeof *Ptr || Ptr->Len > Size) {
> return RETURN_SUCCESS;
> }
>
>
>
>
>
> On 22/07/2021 11:43, 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: 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/BlobVerifierLibSevHashes.inf | 37 ++++
> > OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c | 199
> ++++++++++++++++++++
> > 2 files changed, 236 insertions(+)
> >
> > diff --git a/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
> b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
> > new file mode 100644
> > index 000000000000..76ca0b8154ce
> > --- /dev/null
> > +++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
> > @@ -0,0 +1,37 @@
> > +## @file
> >
> > +#
> >
> > +# Blob verifier library that uses SEV hashes table. The hashes table holds the
> >
> > +# allowed hashes of the kernel, initrd, and cmdline blobs.
> >
> > +#
> >
> > +# Copyright (C) 2021, IBM Corp
> >
> > +#
> >
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > +#
> >
> > +##
> >
> > +
> >
> > +[Defines]
> >
> > + INF_VERSION = 1.29
> >
> > + BASE_NAME = BlobVerifierLibSevHashes
> >
> > + FILE_GUID = 59e713b5-eff3-46a7-8d8b-46f4c004ad7b
> >
> > + MODULE_TYPE = BASE
> >
> > + VERSION_STRING = 1.0
> >
> > + LIBRARY_CLASS = BlobVerifierLib
> >
> > + CONSTRUCTOR = BlobVerifierLibSevHashesConstructor
> >
> > +
> >
> > +[Sources]
> >
> > + BlobVerifierSevHashes.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/BlobVerifierSevHashes.c
> b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> > new file mode 100644
> > index 000000000000..372ae6f469e7
> > --- /dev/null
> > +++ b/OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> > @@ -0,0 +1,199 @@
> > +/** @file
> >
> > +
> >
> > + Blob verifier library that uses SEV hashes table. The hashes table holds the
> >
> > + allowed hashes of the kernel, initrd, and cmdline blobs.
> >
> > +
> >
> > + 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[in] BlobName The name of the blob
> >
> > + @param[in] Buf The data of the blob
> >
> > + @param[in] 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,
> >
> > + IN UINT32 BufSize
> >
> > + )
> >
> > +{
> >
> > + CONST GUID *Guid;
> >
> > + INT32 Remaining;
> >
> > + HASH_TABLE *Entry;
> >
> > +
> >
> > + 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;
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // Remaining is INT32 to catch underflow in case Entry->Len has a
> >
> > + // very high UINT16 value
> >
> > + //
> >
> > + for (Entry = mHashesTable, Remaining = mHashesTableSize;
> >
> > + Remaining >= sizeof *Entry && Remaining >= Entry->Len;
> >
> > + Remaining -= Entry->Len,
> >
> > + Entry = (HASH_TABLE *)((UINT8 *)Entry + Entry->Len)) {
> >
> > + UINTN EntrySize;
> >
> > + EFI_STATUS Status;
> >
> > + UINT8 Hash[SHA256_DIGEST_SIZE];
> >
> > +
> >
> > + if (!CompareGuid (&Entry->Guid, Guid)) {
> >
> > + continue;
> >
> > + }
> >
> > +
> >
> > + DEBUG ((DEBUG_INFO, "%a: Found GUID %g in table\n", __FUNCTION__,
> Guid));
> >
> > +
> >
> > + 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;
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // Calculate the buffer's hash and verify that it is identical to the
> >
> > + // expected hash table entry
> >
> > + //
> >
> > + Sha256HashAll (Buf, BufSize, Hash);
> >
> > +
> >
> > + 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 hashes table is set up correctly, or there is
> no
> >
> > + hashes table
> >
> > +**/
> >
> > +RETURN_STATUS
> >
> > +EFIAPI
> >
> > +BlobVerifierLibSevHashesConstructor (
> >
> > + VOID
> >
> > + )
> >
> > +{
> >
> > + HASH_TABLE *Ptr = (void *)(UINTN)FixedPcdGet64
> (PcdQemuHashTableBase);
> >
> > + UINT32 Size = FixedPcdGet32 (PcdQemuHashTableSize);
> >
> > +
> >
> > + mHashesTable = NULL;
> >
> > + mHashesTableSize = 0;
> >
> > +
> >
> > + if (Ptr == NULL || Size < sizeof *Ptr ||
> >
> > + !CompareGuid (&Ptr->Guid, &SEV_HASH_TABLE_GUID) ||
> >
> > + Ptr->Len < sizeof *Ptr || Ptr->Len > Size) {
> >
> > + 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;
> >
> > +}
> >
>
>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline
2021-07-25 2:43 ` [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Yao, Jiewen
@ 2021-07-25 7:52 ` Dov Murik
2021-07-25 8:17 ` Yao, Jiewen
2021-07-25 21:10 ` James Bottomley
0 siblings, 2 replies; 22+ messages in thread
From: Dov Murik @ 2021-07-25 7:52 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io
Cc: Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum, Jim Cadden,
James Bottomley, Hubertus Franke, Ard Biesheuvel,
Justen, Jordan L, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Xu, Min M, Tom Lendacky, Leif Lindholm, Sami Mujawar, Dov Murik
Hi Jiewen,
On 25/07/2021 5:43, Yao, Jiewen wrote:
> Hi
> I am reviewing this patch series. I am OK with most parts.
Thank you.
>
> And I do have one question:
> May I know what is criteria to put a SEV module to OvmfPkg\AmdSev or OvmfPkg directly?
>
> My original understanding is:
> If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf}, then it should be OvmfPkg.
> If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf}, Then it should be in OvmfPkg\AmdSev.
>
> Am I right?
>
I actually don't know the criteria. What you say sounds reasonable.
I'll also let James (who introduced the AmdSevX64 target) say what he
thinks.
If that is indeed the case, then I need to:
1. Modify patch 4: put the code of the null implementation in
OvmfPkf/BlobVerifierLibNull/
2. Modify patches 5+6: use the new path of the BlobVerifierLibNull inf file
3. Modify patches 10+11: put the code of the SevHashes implementation in
OvmfPkg/AmdSev/BlobVerifierLibSevHashes/
Jiewen, is that what you had in mind?
Two other things to consider:
A. The blob verification by hashes just uses initially-measured memory,
and no other features of SEV. It might be useful for other Confidential
Computing implementations. But maybe if that need arises then we'll
extract it from the AmdSev folder.
B. There were talks about reducing the number of targets, and maybe
unifying AmdSevX64 into OvmfPkgX64. If this is indeed the direction
we're going to, then there's no need to separate the code.
Thanks,
-Dov
> Thank you
> Yao Jiewen
>
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dov Murik
>> Sent: Thursday, July 22, 2021 4:43 PM
>> To: devel@edk2.groups.io
>> Cc: Dov Murik <dovmurik@linux.ibm.com>; Tobin Feldman-Fitzthum
>> <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum <tobin@ibm.com>; Jim
>> Cadden <jcadden@ibm.com>; James Bottomley <jejb@linux.ibm.com>;
>> Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
>> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
>> Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
>> Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen <jiewen.yao@intel.com>;
>> Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
>> <thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami
>> Mujawar <sami.mujawar@arm.com>
>> Subject: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
>> kernel/initrd/cmdline
>>
>> 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 these hashes are part of
>> the SEV measurement (which has to be approved by the Guest Owner for
>> secret injection, for example). Note that populating the hashes table
>> requires QEMU support [1].
>>
>> OVMF parses the table of hashes populated by QEMU (patch 10), and as it
>> reads the fw_cfg blobs from QEMU, it will verify each one against the
>> expected hash. 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:
>>
>> ...
>> BlobVerifierLibSevHashesConstructor: 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 "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 "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 "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 BlobVerifierLibSevHashes 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-v4
>>
>> v4 changes:
>> - BlobVerifierSevHashes (patch 10): more comprehensive overflow tests
>> when parsing the SEV hashes table structure
>>
>> v3: https://edk2.groups.io/g/devel/message/77955
>> v3 changes:
>> - Rename to BlobVerifierLibNull, use decimal INF_VERSION, remove unused
>> DebugLib reference, fix doxygen comments, add missing IN attribute
>> - Rename to BlobVerifierLibSevHashes, use decimal INF_VERSION, fix
>> doxygen comments, add missing IN attribute,
>> calculate buffer hash only when the guid is found in hashes table
>> - SecretPei: use ALIGN_VALUE to round the hob size
>> - Coding style fixes
>> - Add missing 'Ref:' in patch 1 commit message
>> - Fix phrasing and typos in commit messages
>> - Remove Cc: Laszlo from series
>>
>> v2: https://edk2.groups.io/g/devel/message/77505
>> 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: 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>
>>
>> ---
>>
>> Ard: please review patch 6 (ArmVirtPkg). Thanks.
>>
>> Tom, Brijesh: I'll also send the diff for patch 10. Thanks.
>>
>> ---
>>
>> Dov Murik (8):
>> OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
>> OvmfPkg: add library class BlobVerifierLib with null implementation
>> OvmfPkg: add BlobVerifierLibNull to DSC
>> ArmVirtPkg: add BlobVerifierLibNull to DSC
>> OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
>> OvmfPkg/AmdSev/SecretPei: build hob for full page
>> OvmfPkg: add BlobVerifierLibSevHashes
>> 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/BlobVerifierLibNull.inf | 24
>> +++
>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf | 37
>> ++++
>>
>> 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 | 3 +-
>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c | 33 ++++
>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.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, 425 insertions(+), 10 deletions(-)
>> create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf
>> create mode 100644
>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
>> create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h
>> create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c
>> create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
>> copy OvmfPkg/Library/{PlatformBootManagerLib =>
>> PlatformBootManagerLibGrub}/QemuKernel.c (100%)
>>
>> --
>> 2.25.1
>>
>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline
2021-07-25 7:52 ` Dov Murik
@ 2021-07-25 8:17 ` Yao, Jiewen
2021-07-25 10:06 ` Dov Murik
2021-07-25 21:10 ` James Bottomley
1 sibling, 1 reply; 22+ messages in thread
From: Yao, Jiewen @ 2021-07-25 8:17 UTC (permalink / raw)
To: Dov Murik, devel@edk2.groups.io
Cc: Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum, Jim Cadden,
James Bottomley, Hubertus Franke, Ard Biesheuvel,
Justen, Jordan L, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Xu, Min M, Tom Lendacky, Leif Lindholm, Sami Mujawar
Thank you Dov.
Comment below:
> -----Original Message-----
> From: Dov Murik <dovmurik@linux.ibm.com>
> Sent: Sunday, July 25, 2021 3:53 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum
> <tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>; James Bottomley
> <jejb@linux.ibm.com>; Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Erdem Aktas <erdemaktas@google.com>; Xu, Min M <min.m.xu@intel.com>;
> Tom Lendacky <thomas.lendacky@amd.com>; Leif Lindholm
> <leif@nuviainc.com>; Sami Mujawar <sami.mujawar@arm.com>; Dov Murik
> <dovmurik@linux.ibm.com>
> Subject: Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
> kernel/initrd/cmdline
>
> Hi Jiewen,
>
> On 25/07/2021 5:43, Yao, Jiewen wrote:
> > Hi
> > I am reviewing this patch series. I am OK with most parts.
>
> Thank you.
>
> >
> > And I do have one question:
> > May I know what is criteria to put a SEV module to OvmfPkg\AmdSev or
> OvmfPkg directly?
> >
> > My original understanding is:
> > If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf}, then it should
> be OvmfPkg.
> > If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf}, Then it
> should be in OvmfPkg\AmdSev.
> >
> > Am I right?
> >
>
> I actually don't know the criteria. What you say sounds reasonable.
> I'll also let James (who introduced the AmdSevX64 target) say what he
> thinks.
[Jiewen] OK.
>
>
> If that is indeed the case, then I need to:
>
> 1. Modify patch 4: put the code of the null implementation in
> OvmfPkf/BlobVerifierLibNull/
[Jiewen] Since this is a library, I expect to be OvmfPkf/Library/BlobVerifierLibNull/
>
> 2. Modify patches 5+6: use the new path of the BlobVerifierLibNull inf file
>
> 3. Modify patches 10+11: put the code of the SevHashes implementation in
> OvmfPkg/AmdSev/BlobVerifierLibSevHashes/
>
> Jiewen, is that what you had in mind?
[Jiewen] Let's comply with the exiting rule.
1) A library in a packet should be in XXXPkg/Library/AAALib in general. (For example, OvmfPkg/Library)
2) If a library in a packet belongs to a feature, then it should be XXXPkg/FeatureYYY/AAALib. (For example, OvmfPkg/Csm/CsmSupportLib)
>
>
>
> Two other things to consider:
>
> A. The blob verification by hashes just uses initially-measured memory,
> and no other features of SEV. It might be useful for other Confidential
> Computing implementations. But maybe if that need arises then we'll
> extract it from the AmdSev folder.
[Jiewen] I think so. Because it is generic, that is why I agree that the *library class* name does not include SEV keyword.
The *library instance* should add *Sev* keyword, because the implementation does include SEV specific, such as SEV_HASH_TABLE_GUID, SEV_KERNEL_HASH_GUID.
If you want to make it for generic confidential computing, then more work should be done. For example:
1) The instance name should be BlobVerificationLibTeeLinuxModuleHash. (TEE to replace SEV. We need Linux keyword, because I don’t think it is applicable for Windows)
2) We need consider crypto agile. The current instance only consider SHA256, which is not allowed in TDX.
3) The GUID definition should be in OvmfPkg.dec, as the interface.
Since we don’t have a TEE folder yet, I prefer we defer that work.
Later, when we finish all Sev/Tdx work, we can consider create a common Tee dir or event a package. But that may happen in next year.
>
> B. There were talks about reducing the number of targets, and maybe
> unifying AmdSevX64 into OvmfPkgX64. If this is indeed the direction
> we're going to, then there's no need to separate the code.
[Jiewen] I think we are following what Laslo suggested.
1) The OvmfPkg includes the *basic* feature at first.
2) The advanced feature is checked into SEV folder or TDX folder, at first.
3) We can revisit those advanced feature once we think they are mature.
We agree that direction, and we should follow that.
Let's keep #1 and #2 at first to finish the feature at first (in this year). Then we can see how to enhance in #3 (maybe next year).
The more we know each other, the better we can create an architecture to support TEE.
Thank you
Yao Jiewen
>
>
> Thanks,
> -Dov
>
>
> > Thank you
> > Yao Jiewen
> >
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dov
> Murik
> >> Sent: Thursday, July 22, 2021 4:43 PM
> >> To: devel@edk2.groups.io
> >> Cc: Dov Murik <dovmurik@linux.ibm.com>; Tobin Feldman-Fitzthum
> >> <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum <tobin@ibm.com>; Jim
> >> Cadden <jcadden@ibm.com>; James Bottomley <jejb@linux.ibm.com>;
> >> Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
> >> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> >> Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh
> <brijesh.singh@amd.com>;
> >> Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen
> <jiewen.yao@intel.com>;
> >> Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
> >> <thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami
> >> Mujawar <sami.mujawar@arm.com>
> >> Subject: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
> >> kernel/initrd/cmdline
> >>
> >> 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 these hashes are part of
> >> the SEV measurement (which has to be approved by the Guest Owner for
> >> secret injection, for example). Note that populating the hashes table
> >> requires QEMU support [1].
> >>
> >> OVMF parses the table of hashes populated by QEMU (patch 10), and as it
> >> reads the fw_cfg blobs from QEMU, it will verify each one against the
> >> expected hash. 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:
> >>
> >> ...
> >> BlobVerifierLibSevHashesConstructor: 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 "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 "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 "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 BlobVerifierLibSevHashes 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-v4
> >>
> >> v4 changes:
> >> - BlobVerifierSevHashes (patch 10): more comprehensive overflow tests
> >> when parsing the SEV hashes table structure
> >>
> >> v3: https://edk2.groups.io/g/devel/message/77955
> >> v3 changes:
> >> - Rename to BlobVerifierLibNull, use decimal INF_VERSION, remove unused
> >> DebugLib reference, fix doxygen comments, add missing IN attribute
> >> - Rename to BlobVerifierLibSevHashes, use decimal INF_VERSION, fix
> >> doxygen comments, add missing IN attribute,
> >> calculate buffer hash only when the guid is found in hashes table
> >> - SecretPei: use ALIGN_VALUE to round the hob size
> >> - Coding style fixes
> >> - Add missing 'Ref:' in patch 1 commit message
> >> - Fix phrasing and typos in commit messages
> >> - Remove Cc: Laszlo from series
> >>
> >> v2: https://edk2.groups.io/g/devel/message/77505
> >> 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: 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>
> >>
> >> ---
> >>
> >> Ard: please review patch 6 (ArmVirtPkg). Thanks.
> >>
> >> Tom, Brijesh: I'll also send the diff for patch 10. Thanks.
> >>
> >> ---
> >>
> >> Dov Murik (8):
> >> OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
> >> OvmfPkg: add library class BlobVerifierLib with null implementation
> >> OvmfPkg: add BlobVerifierLibNull to DSC
> >> ArmVirtPkg: add BlobVerifierLibNull to DSC
> >> OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
> >> OvmfPkg/AmdSev/SecretPei: build hob for full page
> >> OvmfPkg: add BlobVerifierLibSevHashes
> >> 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/BlobVerifierLibNull.inf | 24
> >> +++
> >> OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf |
> 37
> >> ++++
> >>
> >>
> 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 | 3 +-
> >> OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c | 33
> ++++
> >> OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.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, 425 insertions(+), 10 deletions(-)
> >> create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf
> >> create mode 100644
> >> OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
> >> create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h
> >> create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c
> >> create mode 100644
> OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
> >> copy OvmfPkg/Library/{PlatformBootManagerLib =>
> >> PlatformBootManagerLibGrub}/QemuKernel.c (100%)
> >>
> >> --
> >> 2.25.1
> >>
> >>
> >>
> >>
> >>
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline
2021-07-25 8:17 ` Yao, Jiewen
@ 2021-07-25 10:06 ` Dov Murik
0 siblings, 0 replies; 22+ messages in thread
From: Dov Murik @ 2021-07-25 10:06 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io
Cc: Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum, Jim Cadden,
James Bottomley, Hubertus Franke, Ard Biesheuvel,
Justen, Jordan L, Ashish Kalra, Brijesh Singh, Erdem Aktas,
Xu, Min M, Tom Lendacky, Leif Lindholm, Sami Mujawar, Dov Murik
Thanks for the explanations. Comments below:
On 25/07/2021 11:17, Yao, Jiewen wrote:
> Thank you Dov.
> Comment below:
>
>> -----Original Message-----
>> From: Dov Murik <dovmurik@linux.ibm.com>
>> Sent: Sunday, July 25, 2021 3:53 PM
>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
>> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum
>> <tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>; James Bottomley
>> <jejb@linux.ibm.com>; Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
>> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
>> Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh <brijesh.singh@amd.com>;
>> Erdem Aktas <erdemaktas@google.com>; Xu, Min M <min.m.xu@intel.com>;
>> Tom Lendacky <thomas.lendacky@amd.com>; Leif Lindholm
>> <leif@nuviainc.com>; Sami Mujawar <sami.mujawar@arm.com>; Dov Murik
>> <dovmurik@linux.ibm.com>
>> Subject: Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
>> kernel/initrd/cmdline
>>
>> Hi Jiewen,
>>
>> On 25/07/2021 5:43, Yao, Jiewen wrote:
>>> Hi
>>> I am reviewing this patch series. I am OK with most parts.
>>
>> Thank you.
>>
>>>
>>> And I do have one question:
>>> May I know what is criteria to put a SEV module to OvmfPkg\AmdSev or
>> OvmfPkg directly?
>>>
>>> My original understanding is:
>>> If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf}, then it should
>> be OvmfPkg.
>>> If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf}, Then it
>> should be in OvmfPkg\AmdSev.
>>>
>>> Am I right?
>>>
>>
>> I actually don't know the criteria. What you say sounds reasonable.
>> I'll also let James (who introduced the AmdSevX64 target) say what he
>> thinks.
> [Jiewen] OK.
>
>
>>
>>
>> If that is indeed the case, then I need to:
>>
>> 1. Modify patch 4: put the code of the null implementation in
>> OvmfPkf/BlobVerifierLibNull/
> [Jiewen] Since this is a library, I expect to be OvmfPkf/Library/BlobVerifierLibNull/
>
Yes, you're right, I missed that "/Library/" in my description.
>>
>> 2. Modify patches 5+6: use the new path of the BlobVerifierLibNull inf file
>>
>> 3. Modify patches 10+11: put the code of the SevHashes implementation in
>> OvmfPkg/AmdSev/BlobVerifierLibSevHashes/
>>
>> Jiewen, is that what you had in mind?
> [Jiewen] Let's comply with the exiting rule.
> 1) A library in a packet should be in XXXPkg/Library/AAALib in general. (For example, OvmfPkg/Library)
> 2) If a library in a packet belongs to a feature, then it should be XXXPkg/FeatureYYY/AAALib. (For example, OvmfPkg/Csm/CsmSupportLib)
>
OK. I'll send a new version with the paths corrected.
>
>>
>>
>>
>> Two other things to consider:
>>
>> A. The blob verification by hashes just uses initially-measured memory,
>> and no other features of SEV. It might be useful for other Confidential
>> Computing implementations. But maybe if that need arises then we'll
>> extract it from the AmdSev folder.
> [Jiewen] I think so. Because it is generic, that is why I agree that the *library class* name does not include SEV keyword.
> The *library instance* should add *Sev* keyword, because the implementation does include SEV specific, such as SEV_HASH_TABLE_GUID, SEV_KERNEL_HASH_GUID.
>
> If you want to make it for generic confidential computing, then more work should be done. For example:
> 1) The instance name should be BlobVerificationLibTeeLinuxModuleHash. (TEE to replace SEV. We need Linux keyword, because I don’t think it is applicable for Windows)
> 2) We need consider crypto agile. The current instance only consider SHA256, which is not allowed in TDX.
> 3) The GUID definition should be in OvmfPkg.dec, as the interface.
>
> Since we don’t have a TEE folder yet, I prefer we defer that work.
> Later, when we finish all Sev/Tdx work, we can consider create a common Tee dir or event a package. But that may happen in next year.
>
OK I'll keep that in mind for later. Also note that these require
corresponding changes in QEMU.
>
>
>>
>> B. There were talks about reducing the number of targets, and maybe
>> unifying AmdSevX64 into OvmfPkgX64. If this is indeed the direction
>> we're going to, then there's no need to separate the code.
> [Jiewen] I think we are following what Laslo suggested.
> 1) The OvmfPkg includes the *basic* feature at first.
> 2) The advanced feature is checked into SEV folder or TDX folder, at first.
> 3) We can revisit those advanced feature once we think they are mature.
>
> We agree that direction, and we should follow that.
> Let's keep #1 and #2 at first to finish the feature at first (in this year). Then we can see how to enhance in #3 (maybe next year).
> The more we know each other, the better we can create an architecture to support TEE.
>
Sounds good.
Thanks,
-Dov
> Thank you
> Yao Jiewen
>
>>
>>
>> Thanks,
>> -Dov
>>
>>
>>> Thank you
>>> Yao Jiewen
>>>
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dov
>> Murik
>>>> Sent: Thursday, July 22, 2021 4:43 PM
>>>> To: devel@edk2.groups.io
>>>> Cc: Dov Murik <dovmurik@linux.ibm.com>; Tobin Feldman-Fitzthum
>>>> <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum <tobin@ibm.com>; Jim
>>>> Cadden <jcadden@ibm.com>; James Bottomley <jejb@linux.ibm.com>;
>>>> Hubertus Franke <frankeh@us.ibm.com>; Ard Biesheuvel
>>>> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
>>>> Ashish Kalra <ashish.kalra@amd.com>; Brijesh Singh
>> <brijesh.singh@amd.com>;
>>>> Erdem Aktas <erdemaktas@google.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>;
>>>> Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
>>>> <thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami
>>>> Mujawar <sami.mujawar@arm.com>
>>>> Subject: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
>>>> kernel/initrd/cmdline
>>>>
>>>> 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 these hashes are part of
>>>> the SEV measurement (which has to be approved by the Guest Owner for
>>>> secret injection, for example). Note that populating the hashes table
>>>> requires QEMU support [1].
>>>>
>>>> OVMF parses the table of hashes populated by QEMU (patch 10), and as it
>>>> reads the fw_cfg blobs from QEMU, it will verify each one against the
>>>> expected hash. 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:
>>>>
>>>> ...
>>>> BlobVerifierLibSevHashesConstructor: 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 "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 "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 "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 BlobVerifierLibSevHashes 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-v4
>>>>
>>>> v4 changes:
>>>> - BlobVerifierSevHashes (patch 10): more comprehensive overflow tests
>>>> when parsing the SEV hashes table structure
>>>>
>>>> v3: https://edk2.groups.io/g/devel/message/77955
>>>> v3 changes:
>>>> - Rename to BlobVerifierLibNull, use decimal INF_VERSION, remove unused
>>>> DebugLib reference, fix doxygen comments, add missing IN attribute
>>>> - Rename to BlobVerifierLibSevHashes, use decimal INF_VERSION, fix
>>>> doxygen comments, add missing IN attribute,
>>>> calculate buffer hash only when the guid is found in hashes table
>>>> - SecretPei: use ALIGN_VALUE to round the hob size
>>>> - Coding style fixes
>>>> - Add missing 'Ref:' in patch 1 commit message
>>>> - Fix phrasing and typos in commit messages
>>>> - Remove Cc: Laszlo from series
>>>>
>>>> v2: https://edk2.groups.io/g/devel/message/77505
>>>> 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: 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>
>>>>
>>>> ---
>>>>
>>>> Ard: please review patch 6 (ArmVirtPkg). Thanks.
>>>>
>>>> Tom, Brijesh: I'll also send the diff for patch 10. Thanks.
>>>>
>>>> ---
>>>>
>>>> Dov Murik (8):
>>>> OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds
>>>> OvmfPkg: add library class BlobVerifierLib with null implementation
>>>> OvmfPkg: add BlobVerifierLibNull to DSC
>>>> ArmVirtPkg: add BlobVerifierLibNull to DSC
>>>> OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg
>>>> OvmfPkg/AmdSev/SecretPei: build hob for full page
>>>> OvmfPkg: add BlobVerifierLibSevHashes
>>>> 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/BlobVerifierLibNull.inf | 24
>>>> +++
>>>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf |
>> 37
>>>> ++++
>>>>
>>>>
>> 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 | 3 +-
>>>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c | 33
>> ++++
>>>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.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, 425 insertions(+), 10 deletions(-)
>>>> create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibNull.inf
>>>> create mode 100644
>>>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierLibSevHashes.inf
>>>> create mode 100644 OvmfPkg/Include/Library/BlobVerifierLib.h
>>>> create mode 100644 OvmfPkg/Library/BlobVerifierLib/BlobVerifierNull.c
>>>> create mode 100644
>> OvmfPkg/Library/BlobVerifierLib/BlobVerifierSevHashes.c
>>>> copy OvmfPkg/Library/{PlatformBootManagerLib =>
>>>> PlatformBootManagerLibGrub}/QemuKernel.c (100%)
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline
2021-07-25 7:52 ` Dov Murik
2021-07-25 8:17 ` Yao, Jiewen
@ 2021-07-25 21:10 ` James Bottomley
2021-07-26 0:55 ` Yao, Jiewen
1 sibling, 1 reply; 22+ messages in thread
From: James Bottomley @ 2021-07-25 21:10 UTC (permalink / raw)
To: devel, dovmurik, Yao, Jiewen
Cc: Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum, Jim Cadden,
Hubertus Franke, Ard Biesheuvel, Justen, Jordan L, Ashish Kalra,
Brijesh Singh, Erdem Aktas, Xu, Min M, Tom Lendacky,
Leif Lindholm, Sami Mujawar
On Sun, 2021-07-25 at 10:52 +0300, Dov Murik wrote:
> And I do have one question:
> > May I know what is criteria to put a SEV module to OvmfPkg\AmdSev
> > or OvmfPkg directly?
> >
> > My original understanding is:
> > If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf},
> > then it should be OvmfPkg.
> > If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf},
> > Then it should be in OvmfPkg\AmdSev.
> >
> > Am I right?
> >
>
> I actually don't know the criteria. What you say sounds reasonable.
> I'll also let James (who introduced the AmdSevX64 target) say what he
> thinks.
The original reason for the AmdSev package was actually for
attestation: The only way to get attested boot using a standard VM
image for SEV and SEV-ES was to pull grub inside the measurement
envelope and have a stripped down hard failing boot path, so if the key
didn't decode the encrypted boot volume for some reason, the whole
thing would fail without revealing the injected secret. This stripped
down hard failing boot path is much easier to construct as a separate
target.
Essentially that means that lots of SEV exists outside the AmdSev
directory and things should only be in it if they're either modified to
support the encrypted volume boot path or are only required by it.
However, this ran into problems when it was decided AmdSev shouldn't
have it's own Library, so the modified boot path now lives in
OvmfPkg/Library/PlatformBootManagerLibGrub, so now it's unclear even to
me what the criteria are.
James
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline
2021-07-25 21:10 ` James Bottomley
@ 2021-07-26 0:55 ` Yao, Jiewen
2021-07-26 14:50 ` James Bottomley
0 siblings, 1 reply; 22+ messages in thread
From: Yao, Jiewen @ 2021-07-26 0:55 UTC (permalink / raw)
To: jejb@linux.ibm.com, devel@edk2.groups.io, dovmurik@linux.ibm.com
Cc: Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum, Jim Cadden,
Hubertus Franke, Ard Biesheuvel, Justen, Jordan L, Ashish Kalra,
Brijesh Singh, Erdem Aktas, Xu, Min M, Tom Lendacky,
Leif Lindholm, Sami Mujawar, Yao, Jiewen
Hi James
"However, this ran into problems when it was decided AmdSev shouldn't have it's own Library."
I am not clear on the history. Would you please clarify why AmdSev should not have its own library?
It looks not reasonable to me. AmdSev is just a feature. A feature may have its own library. We have enough examples.
Also, the instance name "Grub" is very confusing. I compared PlatformBootManagerLib and PlatformBootManagerLibGrub. This is just a customized PlatformBootManagerLib.
For example, XEN feature removing and PIIX4 difference has nothing to do with Grub...
=================
PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x60), 0x0b); // A
PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x61), 0x0b); // B
PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x62), 0x0a); // C
PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x63), 0x0a); // D
=================
It is a big misleading. Can we move the PlatformBootManagerLibGrub To AmdSev now?
> -----Original Message-----
> From: James Bottomley <jejb@linux.ibm.com>
> Sent: Monday, July 26, 2021 5:10 AM
> To: devel@edk2.groups.io; dovmurik@linux.ibm.com; Yao, Jiewen
> <jiewen.yao@intel.com>
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum
> <tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>; Hubertus Franke
> <frankeh@us.ibm.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Ashish Kalra <ashish.kalra@amd.com>;
> Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas
> <erdemaktas@google.com>; Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami
> Mujawar <sami.mujawar@arm.com>
> Subject: Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
> kernel/initrd/cmdline
>
> On Sun, 2021-07-25 at 10:52 +0300, Dov Murik wrote:
> > And I do have one question:
> > > May I know what is criteria to put a SEV module to OvmfPkg\AmdSev
> > > or OvmfPkg directly?
> > >
> > > My original understanding is:
> > > If a module is required by OvmfPkg{Ia32,Ia32X64,X64}.{dsc,fdf},
> > > then it should be OvmfPkg.
> > > If a module is only required by OvmfPkg\AmdSev\AmdSevX64.{dsc,fdf},
> > > Then it should be in OvmfPkg\AmdSev.
> > >
> > > Am I right?
> > >
> >
> > I actually don't know the criteria. What you say sounds reasonable.
> > I'll also let James (who introduced the AmdSevX64 target) say what he
> > thinks.
>
> The original reason for the AmdSev package was actually for
> attestation: The only way to get attested boot using a standard VM
> image for SEV and SEV-ES was to pull grub inside the measurement
> envelope and have a stripped down hard failing boot path, so if the key
> didn't decode the encrypted boot volume for some reason, the whole
> thing would fail without revealing the injected secret. This stripped
> down hard failing boot path is much easier to construct as a separate
> target.
>
> Essentially that means that lots of SEV exists outside the AmdSev
> directory and things should only be in it if they're either modified to
> support the encrypted volume boot path or are only required by it.
> However, this ran into problems when it was decided AmdSev shouldn't
> have it's own Library, so the modified boot path now lives in
> OvmfPkg/Library/PlatformBootManagerLibGrub, so now it's unclear even to
> me what the criteria are.
>
> James
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline
2021-07-26 0:55 ` Yao, Jiewen
@ 2021-07-26 14:50 ` James Bottomley
2021-07-26 15:31 ` Yao, Jiewen
0 siblings, 1 reply; 22+ messages in thread
From: James Bottomley @ 2021-07-26 14:50 UTC (permalink / raw)
To: Yao, Jiewen, devel@edk2.groups.io, dovmurik@linux.ibm.com
Cc: Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum, Jim Cadden,
Hubertus Franke, Ard Biesheuvel, Justen, Jordan L, Ashish Kalra,
Brijesh Singh, Erdem Aktas, Xu, Min M, Tom Lendacky,
Leif Lindholm, Sami Mujawar
On Mon, 2021-07-26 at 00:55 +0000, Yao, Jiewen wrote:
> Hi James
> "However, this ran into problems when it was decided AmdSev shouldn't
> have it's own Library."
>
> I am not clear on the history. Would you please clarify why AmdSev
> should not have its own library?
The history predates me. It was already done for the Bhyve package
which also has a modified PlatformBootManagerLib when I came along with
this. However, only having Library in the top level package seems to
be a common edk2 pattern if you run a find.
> It looks not reasonable to me. AmdSev is just a feature. A feature
> may have its own library. We have enough examples.
We do? Running
find . -name Library -print
only turns up
./FmpDevicePkg/Test/UnitTest/Library
As not following the top level package only pattern.
> Also, the instance name "Grub" is very confusing. I compared
> PlatformBootManagerLib and PlatformBootManagerLibGrub. This is just a
> customized PlatformBootManagerLib.
It's called Grub because it places Grub in the Fv for combined pre-
attestation. Either SEV or TDX could use this (Although TDX looks
likely not to want to).
> For example, XEN feature removing and PIIX4 difference has nothing to
> do with Grub...
> =================
> PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x60), 0x0b); // A
> PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x61), 0x0b); // B
> PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x62), 0x0a); // C
> PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x63), 0x0a); // D
> =================
It's part of the boot path stripping to make sure there's a hard
failure if Grub fails to execute. There's a Bugzilla requiring more of
this because a grub only booting platform library needs fewer
extraneous things which could constitute an attack surface for the
injected secret.
> It is a big misleading. Can we move the PlatformBootManagerLibGrub To
> AmdSev now?
I think you probably want to ask around older edk2 package maintainers
and see if there's any reason for this pattern, which seems to be
strongly enforced. If no-one can remember, then likely it can be
broken.
James
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline
2021-07-26 14:50 ` James Bottomley
@ 2021-07-26 15:31 ` Yao, Jiewen
0 siblings, 0 replies; 22+ messages in thread
From: Yao, Jiewen @ 2021-07-26 15:31 UTC (permalink / raw)
To: jejb@linux.ibm.com, devel@edk2.groups.io, dovmurik@linux.ibm.com
Cc: Tobin Feldman-Fitzthum, Tobin Feldman-Fitzthum, Jim Cadden,
Hubertus Franke, Ard Biesheuvel, Justen, Jordan L, Ashish Kalra,
Brijesh Singh, Erdem Aktas, Xu, Min M, Tom Lendacky,
Leif Lindholm, Sami Mujawar
Hi James
You are right that initially EDKII did recommend to put a lib under Library dir.
But with more and more feature, people start realizing that it is NOT efficient way to identify a *feature*.
We changed the direction later - if we can define a feature dir, we can lib to feature dir.
I used " find . -name *Lib*.inf -print", because I cannot assume the Lib is under Library dir for feature.
I can find lots of stuff. Most of them are under Library, below is NOT in *Pkg/Library/
=============
./edk2/ArmPkg/Drivers/ArmGic/ArmGicLib.inf
./edk2/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
./edk2/ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
./edk2/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/TestBaseCryptLibHost.inf
./edk2/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/TestBaseCryptLibShell.inf
./edk2/FmpDevicePkg/FmpDxe/FmpDxeLib.inf
./edk2/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/FmpDependencyLibUnitTestsHost.inf
./edk2/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/FmpDependencyLibUnitTestsUefi.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseLib/BaseLibUnitTestsHost.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseLib/BaseLibUnitTestsUefi.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibDxe.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibHost.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibPei.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibSmm.inf
./edk2/MdePkg/Test/UnitTest/Library/BaseSafeIntLib/TestBaseSafeIntLibUefiShell.inf
./edk2/OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf
./edk2/OvmfPkg/Csm/LegacyBootMaintUiLib/LegacyBootMaintUiLib.inf
./edk2/OvmfPkg/Csm/LegacyBootManagerLib/LegacyBootManagerLib.inf
============
If I search on edk2-platform (https://github.com/tianocore/edk2-platforms), I can find more:
============
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BaseFuncLib/BaseFuncLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BaseGpioCheckConflictLib/BaseGpioCheckConflictLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BaseGpioCheckConflictLibNull/BaseGpioCheckConflictLibNull.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BasePlatformHookLib/BasePlatformHookLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardAcpiLib/SmmBoardAcpiEnableLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardAcpiLib/SmmMultiBoardAcpiSupportLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardInitLib/PeiBoardInitPostMemLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardInitLib/PeiBoardInitPreMemLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardInitLib/PeiMultiBoardInitPostMemLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/BoardInitLib/PeiMultiBoardInitPreMemLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/DxePolicyBoardConfigLib/DxePolicyBoardConfigLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/CometlakeURvp/Library/PeiPolicyBoardConfigLib/PeiPolicyBoardConfigLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/Features/Tbt/Library/DxeTbtPolicyLib/DxeTbtPolicyLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/Features/Tbt/Library/PeiDxeSmmTbtCommonLib/TbtCommonLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/Features/Tbt/Library/PeiTbtPolicyLib/PeiTbtPolicyLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/Features/Tbt/Library/Private/PeiDTbtInitLib/PeiDTbtInitLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/FspWrapper/Library/PeiFspPolicyInitLib/PeiFspPolicyInitLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/FspWrapper/Library/PeiSiliconPolicyUpdateLibFsp/PeiSiliconPolicyUpdateLibFsp.inf
./Platform/Intel/CometlakeOpenBoardPkg/Policy/Library/DxePolicyUpdateLib/DxePolicyUpdateLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/Policy/Library/PeiPolicyInitLib/PeiPolicyInitLib.inf
./Platform/Intel/CometlakeOpenBoardPkg/Policy/Library/PeiPolicyUpdateLib/PeiPolicyUpdateLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/Features/Tbt/Library/DxeTbtPolicyLib/DxeTbtPolicyLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/Features/Tbt/Library/PeiDxeSmmTbtCommonLib/TbtCommonLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/Features/Tbt/Library/PeiTbtPolicyLib/PeiTbtPolicyLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/Features/Tbt/Library/Private/PeiDTbtInitLib/PeiDTbtInitLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/FspWrapper/Library/PeiSiliconPolicyNotifyLib/PeiPreMemSiliconPolicyNotifyLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/FspWrapper/Library/PeiSiliconPolicyNotifyLib/PeiPreMemSiliconPolicyNotifyLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/FspWrapper/Library/PeiSiliconPolicyUpdateLibFsp/PeiSiliconPolicyUpdateLibFsp.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BasePlatformHookLib/BasePlatformHookLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardAcpiLib/DxeBoardAcpiTableLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardAcpiLib/DxeMultiBoardAcpiSupportLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardAcpiLib/SmmBoardAcpiEnableLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardAcpiLib/SmmMultiBoardAcpiSupportLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardInitLib/PeiBoardInitPostMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardInitLib/PeiBoardInitPreMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardInitLib/PeiMultiBoardInitPostMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Library/BoardInitLib/PeiMultiBoardInitPreMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/GalagoPro3/Policy/Library/DxeSiliconPolicyUpdateLib/DxeSiliconPolicyUpdateLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/FspWrapper/Library/PeiSiliconPolicyUpdateLibFsp/PeiSiliconPolicyUpdateLibFsp.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BasePlatformHookLib/BasePlatformHookLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardAcpiLib/DxeBoardAcpiTableLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardAcpiLib/DxeMultiBoardAcpiSupportLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardAcpiLib/SmmBoardAcpiEnableLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardAcpiLib/SmmMultiBoardAcpiSupportLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardInitLib/PeiBoardInitPostMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardInitLib/PeiBoardInitPreMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardInitLib/PeiMultiBoardInitPostMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Library/BoardInitLib/PeiMultiBoardInitPreMemLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Policy/Library/DxeSiliconPolicyUpdateLib/DxeSiliconPolicyUpdateLib.inf
./Platform/Intel/KabylakeOpenBoardPkg/KabylakeRvp3/Policy/Library/PeiSiliconPolicyUpdateLib/PeiSiliconPolicyUpdateLib.inf
./Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiEnableLibNull/BoardAcpiEnableLibNull.inf
./Platform/Intel/MinPlatformPkg/Acpi/Library/BoardAcpiTableLibNull/BoardAcpiTableLibNull.inf
./Platform/Intel/MinPlatformPkg/Acpi/Library/DxeAslUpdateLib/DxeAslUpdateLib.inf
./Platform/Intel/MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/DxeMultiBoardAcpiSupportLib.inf
./Platform/Intel/MinPlatformPkg/Acpi/Library/MultiBoardAcpiSupportLib/SmmMultiBoardAcpiSupportLib.inf
./Platform/Intel/MinPlatformPkg/Bds/Library/BoardBootManagerLibNull/BoardBootManagerLibNull.inf
./Platform/Intel/MinPlatformPkg/Bds/Library/DxePlatformBootManagerLib/DxePlatformBootManagerLib.inf
./Platform/Intel/MinPlatformPkg/Flash/Library/SpiFlashCommonLibNull/SpiFlashCommonLibNull.inf
./Platform/Intel/MinPlatformPkg/FspWrapper/Library/DxeFspWrapperPlatformLib/DxeFspWrapperPlatformLib.inf
./Platform/Intel/MinPlatformPkg/FspWrapper/Library/PeiFspWrapperHobProcessLib/PeiFspWrapperHobProcessLib.inf
./Platform/Intel/MinPlatformPkg/FspWrapper/Library/PeiFspWrapperPlatformLib/PeiFspWrapperPlatformLib.inf
./Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatformSecLib/SecFspWrapperPlatformSecLib.inf
./Platform/Intel/MinPlatformPkg/Pci/Library/PciHostBridgeLibSimple/PciHostBridgeLibSimple.inf
./Platform/Intel/MinPlatformPkg/Pci/Library/PciSegmentInfoLibSimple/PciSegmentInfoLibSimple.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/BoardInitLibNull/BoardInitLibNull.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/MultiBoardInitSupportLib/DxeMultiBoardInitSupportLib.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/MultiBoardInitSupportLib/PeiMultiBoardInitSupportLib.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/PeiReportFvLib/PeiReportFvLib.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/ReportCpuHobLib/ReportCpuHobLib.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/SecBoardInitLibNull/SecBoardInitLibNull.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/SiliconPolicyInitLibNull/SiliconPolicyInitLibNull.inf
./Platform/Intel/MinPlatformPkg/PlatformInit/Library/SiliconPolicyUpdateLibNull/SiliconPolicyUpdateLibNull.inf
./Platform/Intel/MinPlatformPkg/Tcg/Library/PeiDxeTpmPlatformHierarchyLib/PeiDxeTpmPlatformHierarchyLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/PeiTestPointCheckLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SecTestPointCheckLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLibNull/TestPointCheckLibNull.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/DxeTestPointLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/PeiTestPointLib.inf
./Platform/Intel/MinPlatformPkg/Test/Library/TestPointLib/SmmTestPointLib.inf
......
============
> -----Original Message-----
> From: James Bottomley <jejb@linux.ibm.com>
> Sent: Monday, July 26, 2021 10:50 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
> dovmurik@linux.ibm.com
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>; Tobin Feldman-Fitzthum
> <tobin@ibm.com>; Jim Cadden <jcadden@ibm.com>; Hubertus Franke
> <frankeh@us.ibm.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Ashish Kalra <ashish.kalra@amd.com>;
> Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas
> <erdemaktas@google.com>; Xu, Min M <min.m.xu@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Leif Lindholm <leif@nuviainc.com>; Sami
> Mujawar <sami.mujawar@arm.com>
> Subject: Re: [edk2-devel] [PATCH v4 00/11] Measured SEV boot with
> kernel/initrd/cmdline
>
> On Mon, 2021-07-26 at 00:55 +0000, Yao, Jiewen wrote:
> > Hi James
> > "However, this ran into problems when it was decided AmdSev shouldn't
> > have it's own Library."
> >
> > I am not clear on the history. Would you please clarify why AmdSev
> > should not have its own library?
>
> The history predates me. It was already done for the Bhyve package
> which also has a modified PlatformBootManagerLib when I came along with
> this. However, only having Library in the top level package seems to
> be a common edk2 pattern if you run a find.
>
> > It looks not reasonable to me. AmdSev is just a feature. A feature
> > may have its own library. We have enough examples.
>
> We do? Running
>
> find . -name Library -print
>
> only turns up
>
> ./FmpDevicePkg/Test/UnitTest/Library
>
> As not following the top level package only pattern.
>
> > Also, the instance name "Grub" is very confusing. I compared
> > PlatformBootManagerLib and PlatformBootManagerLibGrub. This is just a
> > customized PlatformBootManagerLib.
>
> It's called Grub because it places Grub in the Fv for combined pre-
> attestation. Either SEV or TDX could use this (Although TDX looks
> likely not to want to).
>
> > For example, XEN feature removing and PIIX4 difference has nothing to
> > do with Grub...
> > =================
> > PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x60), 0x0b); // A
> > PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x61), 0x0b); // B
> > PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x62), 0x0a); // C
> > PciWrite8 (PCI_LIB_ADDRESS (0, 1, 0, 0x63), 0x0a); // D
> > =================
>
> It's part of the boot path stripping to make sure there's a hard
> failure if Grub fails to execute. There's a Bugzilla requiring more of
> this because a grub only booting platform library needs fewer
> extraneous things which could constitute an attack surface for the
> injected secret.
>
> > It is a big misleading. Can we move the PlatformBootManagerLibGrub To
> > AmdSev now?
>
> I think you probably want to ask around older edk2 package maintainers
> and see if there's any reason for this pattern, which seems to be
> strongly enforced. If no-one can remember, then likely it can be
> broken.
>
> James
>
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-07-26 15:31 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-07-22 8:42 [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Dov Murik
2021-07-22 8:42 ` [PATCH v4 01/11] OvmfPkg/AmdSev/SecretDxe: fix header comment to generic naming Dov Murik
2021-07-22 8:42 ` [PATCH v4 02/11] OvmfPkg/AmdSev: use GenericQemuLoadImageLib in AmdSev builds Dov Murik
2021-07-22 8:42 ` [PATCH v4 03/11] OvmfPkg: PlatformBootManagerLibGrub: Allow executing kernel via fw_cfg Dov Murik
2021-07-22 8:43 ` [PATCH v4 04/11] OvmfPkg: add library class BlobVerifierLib with null implementation Dov Murik
2021-07-22 8:43 ` [PATCH v4 05/11] OvmfPkg: add BlobVerifierLibNull to DSC Dov Murik
2021-07-22 8:43 ` [PATCH v4 06/11] ArmVirtPkg: " Dov Murik
2021-07-22 8:43 ` [PATCH v4 07/11] OvmfPkg/QemuKernelLoaderFsDxe: call VerifyBlob after fetch from fw_cfg Dov Murik
2021-07-22 8:43 ` [PATCH v4 08/11] OvmfPkg/AmdSev/SecretPei: build hob for full page Dov Murik
2021-07-22 8:43 ` [PATCH v4 09/11] OvmfPkg/AmdSev: reserve MEMFD space for for firmware config hashes Dov Murik
2021-07-22 8:43 ` [PATCH v4 10/11] OvmfPkg: add BlobVerifierLibSevHashes Dov Murik
2021-07-22 8:45 ` Dov Murik
2021-07-25 2:47 ` [edk2-devel] " Yao, Jiewen
2021-07-22 8:43 ` [PATCH v4 11/11] OvmfPkg/AmdSev: Enforce hash verification of kernel blobs Dov Murik
2021-07-25 2:43 ` [edk2-devel] [PATCH v4 00/11] Measured SEV boot with kernel/initrd/cmdline Yao, Jiewen
2021-07-25 7:52 ` Dov Murik
2021-07-25 8:17 ` Yao, Jiewen
2021-07-25 10:06 ` Dov Murik
2021-07-25 21:10 ` James Bottomley
2021-07-26 0:55 ` Yao, Jiewen
2021-07-26 14:50 ` James Bottomley
2021-07-26 15:31 ` Yao, Jiewen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox