* [PATCH V5 1/8] EmbeddedPkg: Add AllocateRuntimePages in PrePiMemoryAllocationLib
2022-09-06 4:35 [PATCH V5 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
@ 2022-09-06 4:35 ` Min Xu
2022-09-06 4:35 ` [PATCH V5 2/8] OvmfPkg/PeilessStartupLib: Delete TdxValidateCfv Min Xu
` (7 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Min Xu @ 2022-09-06 4:35 UTC (permalink / raw)
To: devel
Cc: Min M Xu, Leif Lindholm, Ard Biesheuvel, Abner Chang,
Daniel Schaefer, Gerd Hoffmann
From: Min M Xu <min.m.xu@intel.com>
AllocateRuntimePages is used to allocate one or more 4KB pages of
type EfiRuntimeServicesData.
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Abner Chang <abner.chang@hpe.com>
Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Ard Biesheuvel <ardb+tianocore@kernel.org>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
EmbeddedPkg/Include/Library/PrePiLib.h | 19 ++++++
.../MemoryAllocationLib.c | 65 ++++++++++++++-----
2 files changed, 68 insertions(+), 16 deletions(-)
diff --git a/EmbeddedPkg/Include/Library/PrePiLib.h b/EmbeddedPkg/Include/Library/PrePiLib.h
index 7b2cea296f1c..3741b08c4478 100644
--- a/EmbeddedPkg/Include/Library/PrePiLib.h
+++ b/EmbeddedPkg/Include/Library/PrePiLib.h
@@ -665,6 +665,25 @@ AllocatePages (
IN UINTN Pages
);
+/**
+ Allocates one or more 4KB pages of type EfiRuntimeServicesData.
+
+ Allocates the number of 4KB pages of type EfiRuntimeServicesData and returns a pointer to the
+ allocated buffer. The buffer returned is aligned on a 4KB boundary. If Pages is 0, then NULL
+ is returned. If there is not enough memory remaining to satisfy the request, then NULL is
+ returned.
+
+ @param Pages The number of 4 KB pages to allocate.
+
+ @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+VOID *
+EFIAPI
+AllocateRuntimePages (
+ IN UINTN Pages
+ );
+
/**
Allocates a buffer of type EfiBootServicesData.
diff --git a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
index 78f8da5e9527..2cc2a7112197 100644
--- a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
+++ b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
@@ -14,23 +14,12 @@
#include <Library/PrePiLib.h>
#include <Library/DebugLib.h>
-/**
- Allocates one or more 4KB pages of type EfiBootServicesData.
-
- Allocates the number of 4KB pages of MemoryType and returns a pointer to the
- allocated buffer. The buffer returned is aligned on a 4KB boundary. If Pages is 0, then NULL
- is returned. If there is not enough memory remaining to satisfy the request, then NULL is
- returned.
-
- @param Pages The number of 4 KB pages to allocate.
-
- @return A pointer to the allocated buffer or NULL if allocation fails.
-
-**/
+STATIC
VOID *
EFIAPI
-AllocatePages (
- IN UINTN Pages
+InternalAllocatePages (
+ IN UINTN Pages,
+ IN EFI_MEMORY_TYPE MemoryType
)
{
EFI_PEI_HOB_POINTERS Hob;
@@ -65,12 +54,56 @@ AllocatePages (
BuildMemoryAllocationHob (
Hob.HandoffInformationTable->EfiFreeMemoryTop,
Pages * EFI_PAGE_SIZE,
- EfiBootServicesData
+ MemoryType
);
return (VOID *)(UINTN)Hob.HandoffInformationTable->EfiFreeMemoryTop;
}
}
+/**
+ Allocates one or more 4KB pages of type EfiBootServicesData.
+
+ Allocates the number of 4KB pages of MemoryType and returns a pointer to the
+ allocated buffer. The buffer returned is aligned on a 4KB boundary. If Pages is 0, then NULL
+ is returned. If there is not enough memory remaining to satisfy the request, then NULL is
+ returned.
+
+ @param Pages The number of 4 KB pages to allocate.
+
+ @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+VOID *
+EFIAPI
+AllocatePages (
+ IN UINTN Pages
+ )
+{
+ return InternalAllocatePages (Pages, EfiBootServicesData);
+}
+
+/**
+ Allocates one or more 4KB pages of type EfiRuntimeServicesData.
+
+ Allocates the number of 4KB pages of type EfiRuntimeServicesData and returns a pointer to the
+ allocated buffer. The buffer returned is aligned on a 4KB boundary. If Pages is 0, then NULL
+ is returned. If there is not enough memory remaining to satisfy the request, then NULL is
+ returned.
+
+ @param Pages The number of 4 KB pages to allocate.
+
+ @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+VOID *
+EFIAPI
+AllocateRuntimePages (
+ IN UINTN Pages
+ )
+{
+ return InternalAllocatePages (Pages, EfiRuntimeServicesData);
+}
+
/**
Allocates one or more 4KB pages of type EfiBootServicesData at a specified alignment.
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH V5 2/8] OvmfPkg/PeilessStartupLib: Delete TdxValidateCfv
2022-09-06 4:35 [PATCH V5 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
2022-09-06 4:35 ` [PATCH V5 1/8] EmbeddedPkg: Add AllocateRuntimePages in PrePiMemoryAllocationLib Min Xu
@ 2022-09-06 4:35 ` Min Xu
2022-09-06 4:35 ` [PATCH V5 3/8] OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore Min Xu
` (6 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Min Xu @ 2022-09-06 4:35 UTC (permalink / raw)
To: devel
Cc: Min M Xu, Erdem Aktas, James Bottomley, Jiewen Yao, Tom Lendacky,
Gerd Hoffmann
From: Min M Xu <min.m.xu@intel.com>
TdxValidateCfv is used to validate the integrity of FlashNvVarStore
(PcdOvmfFlashNvStorageVariableBase) and it is not Tdx specific.
So it will be moved to PlatformInitLib and be renamed to
PlatformValidateNvVarStore in the following patch. And it will be called
before EmuVaribleNvStore is initialized with the content in
FlashNvVarStore.
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/Library/PeilessStartupLib/IntelTdx.c | 153 ------------------
.../PeilessStartupLib/PeilessStartup.c | 8 -
.../PeilessStartupInternal.h | 17 --
3 files changed, 178 deletions(-)
diff --git a/OvmfPkg/Library/PeilessStartupLib/IntelTdx.c b/OvmfPkg/Library/PeilessStartupLib/IntelTdx.c
index 484fd21057c8..216c413caad5 100644
--- a/OvmfPkg/Library/PeilessStartupLib/IntelTdx.c
+++ b/OvmfPkg/Library/PeilessStartupLib/IntelTdx.c
@@ -7,8 +7,6 @@
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
-#include <Guid/VariableFormat.h>
-#include <Guid/SystemNvDataGuid.h>
#include <IndustryStandard/Tpm20.h>
#include <IndustryStandard/UefiTcgPlatform.h>
#include <Library/HobLib.h>
@@ -37,157 +35,6 @@ typedef struct {
#pragma pack()
-/**
- Check padding data all bit should be 1.
-
- @param[in] Buffer - A pointer to buffer header
- @param[in] BufferSize - Buffer size
-
- @retval TRUE - The padding data is valid.
- @retval TRUE - The padding data is invalid.
-
-**/
-BOOLEAN
-CheckPaddingData (
- IN UINT8 *Buffer,
- IN UINT32 BufferSize
- )
-{
- UINT32 index;
-
- for (index = 0; index < BufferSize; index++) {
- if (Buffer[index] != 0xFF) {
- return FALSE;
- }
- }
-
- return TRUE;
-}
-
-/**
- Check the integrity of CFV data.
-
- @param[in] TdxCfvBase - A pointer to CFV header
- @param[in] TdxCfvSize - CFV data size
-
- @retval TRUE - The CFV data is valid.
- @retval FALSE - The CFV data is invalid.
-
-**/
-BOOLEAN
-EFIAPI
-TdxValidateCfv (
- IN UINT8 *TdxCfvBase,
- IN UINT32 TdxCfvSize
- )
-{
- UINT16 Checksum;
- UINTN VariableBase;
- UINT32 VariableOffset;
- UINT32 VariableOffsetBeforeAlign;
- EFI_FIRMWARE_VOLUME_HEADER *CfvFvHeader;
- VARIABLE_STORE_HEADER *CfvVariableStoreHeader;
- AUTHENTICATED_VARIABLE_HEADER *VariableHeader;
-
- static EFI_GUID FvHdrGUID = EFI_SYSTEM_NV_DATA_FV_GUID;
- static EFI_GUID VarStoreHdrGUID = EFI_AUTHENTICATED_VARIABLE_GUID;
-
- VariableOffset = 0;
-
- if (TdxCfvBase == NULL) {
- DEBUG ((DEBUG_ERROR, "TDX CFV: CFV pointer is NULL\n"));
- return FALSE;
- }
-
- //
- // Verify the header zerovetor, filesystemguid,
- // revision, signature, attributes, fvlength, checksum
- // HeaderLength cannot be an odd number
- //
- CfvFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)TdxCfvBase;
-
- if ((!IsZeroBuffer (CfvFvHeader->ZeroVector, 16)) ||
- (!CompareGuid (&FvHdrGUID, &CfvFvHeader->FileSystemGuid)) ||
- (CfvFvHeader->Signature != EFI_FVH_SIGNATURE) ||
- (CfvFvHeader->Attributes != 0x4feff) ||
- (CfvFvHeader->Revision != EFI_FVH_REVISION) ||
- (CfvFvHeader->FvLength != TdxCfvSize)
- )
- {
- DEBUG ((DEBUG_ERROR, "TDX CFV: Basic FV headers were invalid\n"));
- return FALSE;
- }
-
- //
- // Verify the header checksum
- //
- Checksum = CalculateSum16 ((VOID *)CfvFvHeader, CfvFvHeader->HeaderLength);
-
- if (Checksum != 0) {
- DEBUG ((DEBUG_ERROR, "TDX CFV: FV checksum was invalid\n"));
- return FALSE;
- }
-
- //
- // Verify the header signature, size, format, state
- //
- CfvVariableStoreHeader = (VARIABLE_STORE_HEADER *)(TdxCfvBase + CfvFvHeader->HeaderLength);
- if ((!CompareGuid (&VarStoreHdrGUID, &CfvVariableStoreHeader->Signature)) ||
- (CfvVariableStoreHeader->Format != VARIABLE_STORE_FORMATTED) ||
- (CfvVariableStoreHeader->State != VARIABLE_STORE_HEALTHY) ||
- (CfvVariableStoreHeader->Size > (CfvFvHeader->FvLength - CfvFvHeader->HeaderLength)) ||
- (CfvVariableStoreHeader->Size < sizeof (VARIABLE_STORE_HEADER))
- )
- {
- DEBUG ((DEBUG_ERROR, "TDX CFV: Variable Store header was invalid\n"));
- return FALSE;
- }
-
- //
- // Verify the header startId, state
- // Verify data to the end
- //
- VariableBase = (UINTN)TdxCfvBase + CfvFvHeader->HeaderLength + sizeof (VARIABLE_STORE_HEADER);
- while (VariableOffset < (CfvVariableStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER))) {
- VariableHeader = (AUTHENTICATED_VARIABLE_HEADER *)(VariableBase + VariableOffset);
- if (VariableHeader->StartId != VARIABLE_DATA) {
- if (!CheckPaddingData ((UINT8 *)VariableHeader, CfvVariableStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER) - VariableOffset)) {
- DEBUG ((DEBUG_ERROR, "TDX CFV: Variable header was invalid\n"));
- return FALSE;
- }
-
- VariableOffset = CfvVariableStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER);
- } else {
- if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) ||
- (VariableHeader->State == VAR_DELETED) ||
- (VariableHeader->State == VAR_HEADER_VALID_ONLY) ||
- (VariableHeader->State == VAR_ADDED)))
- {
- DEBUG ((DEBUG_ERROR, "TDX CFV: Variable header was invalid\n"));
- return FALSE;
- }
-
- VariableOffset += sizeof (AUTHENTICATED_VARIABLE_HEADER) + VariableHeader->NameSize + VariableHeader->DataSize;
- // Verify VariableOffset should be less than or equal CfvVariableStoreHeader->Size - sizeof(VARIABLE_STORE_HEADER)
- if (VariableOffset > (CfvVariableStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER))) {
- DEBUG ((DEBUG_ERROR, "TDX CFV: Variable header was invalid\n"));
- return FALSE;
- }
-
- VariableOffsetBeforeAlign = VariableOffset;
- // 4 byte align
- VariableOffset = (VariableOffset + 3) & (UINTN)(~3);
-
- if (!CheckPaddingData ((UINT8 *)(VariableBase + VariableOffsetBeforeAlign), VariableOffset - VariableOffsetBeforeAlign)) {
- DEBUG ((DEBUG_ERROR, "TDX CFV: Variable header was invalid\n"));
- return FALSE;
- }
- }
- }
-
- return TRUE;
-}
-
/**
Measure the Hoblist passed from the VMM.
diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
index fdfefd00d732..7502ec44669e 100644
--- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
+++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
@@ -179,14 +179,6 @@ PeilessStartup (
CpuDeadLoop ();
}
- //
- // Validate Tdx CFV
- //
- if (!TdxValidateCfv (CfvBase, FixedPcdGet32 (PcdCfvRawDataSize))) {
- ASSERT (FALSE);
- CpuDeadLoop ();
- }
-
//
// Measure Tdx CFV
//
diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupInternal.h b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupInternal.h
index 74b5f46552c2..09cac3e26c67 100644
--- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupInternal.h
+++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupInternal.h
@@ -52,23 +52,6 @@ EFIAPI
ConstructSecHobList (
);
-/**
- Check the integrity of CFV data.
-
- @param[in] TdxCfvBase - A pointer to CFV header
- @param[in] TdxCfvSize - CFV data size
-
- @retval TRUE - The CFV data is valid.
- @retval FALSE - The CFV data is invalid.
-
-**/
-BOOLEAN
-EFIAPI
-TdxValidateCfv (
- IN UINT8 *TdxCfvBase,
- IN UINT32 TdxCfvSize
- );
-
/**
Measure the Hoblist passed from the VMM.
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH V5 3/8] OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore
2022-09-06 4:35 [PATCH V5 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
2022-09-06 4:35 ` [PATCH V5 1/8] EmbeddedPkg: Add AllocateRuntimePages in PrePiMemoryAllocationLib Min Xu
2022-09-06 4:35 ` [PATCH V5 2/8] OvmfPkg/PeilessStartupLib: Delete TdxValidateCfv Min Xu
@ 2022-09-06 4:35 ` Min Xu
2022-09-06 4:35 ` [PATCH V5 4/8] OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore Min Xu
` (5 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Min Xu @ 2022-09-06 4:35 UTC (permalink / raw)
To: devel
Cc: Min M Xu, Erdem Aktas, James Bottomley, Jiewen Yao, Tom Lendacky,
Gerd Hoffmann
From: Min M Xu <min.m.xu@intel.com>
There are 3 functions added for EmuVariableNvStore:
- PlatformReserveEmuVariableNvStore
- PlatformInitEmuVariableNvStore
- PlatformValidateNvVarStore
PlatformReserveEmuVariableNvStore allocate storage for NV variables early
on so it will be at a consistent address.
PlatformInitEmuVariableNvStore copies the content in
PcdOvmfFlashNvStorageVariableBase to the storage allocated by
PlatformReserveEmuVariableNvStore. This is used in the case that OVMF is
launched with -bios parameter. Because in that situation UEFI variables
will be partially emulated, and non-volatile variables may lose their
contents after a reboot. This makes the secure boot feature not working.
PlatformValidateNvVarStore is renamed from TdxValidateCfv and it is used
to validate the integrity of FlashNvVarStore
(PcdOvmfFlashNvStorageVariableBase). It should be called before
PlatformInitEmuVariableNvStore is called to copy over the content.
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/Include/Library/PlatformInitLib.h | 51 ++++
OvmfPkg/Library/PlatformInitLib/Platform.c | 238 ++++++++++++++++++
.../PlatformInitLib/PlatformInitLib.inf | 3 +
3 files changed, 292 insertions(+)
diff --git a/OvmfPkg/Include/Library/PlatformInitLib.h b/OvmfPkg/Include/Library/PlatformInitLib.h
index 2987a367cc9c..c5234bf26d45 100644
--- a/OvmfPkg/Include/Library/PlatformInitLib.h
+++ b/OvmfPkg/Include/Library/PlatformInitLib.h
@@ -234,4 +234,55 @@ PlatformTdxPublishRamRegions (
VOID
);
+/**
+ Check the integrity of NvVarStore.
+
+ @param[in] NvVarStoreBase - A pointer to NvVarStore header
+ @param[in] NvVarStoreSize - NvVarStore size
+
+ @retval TRUE - The NvVarStore is valid.
+ @retval FALSE - The NvVarStore is invalid.
+
+**/
+BOOLEAN
+EFIAPI
+PlatformValidateNvVarStore (
+ IN UINT8 *NvVarStoreBase,
+ IN UINT32 NvVarStoreSize
+ );
+
+/**
+ Allocate storage for NV variables early on so it will be
+ at a consistent address. Since VM memory is preserved
+ across reboots, this allows the NV variable storage to survive
+ a VM reboot.
+
+ *
+ * @retval VOID* The pointer to the storage for NV Variables
+ */
+VOID *
+EFIAPI
+PlatformReserveEmuVariableNvStore (
+ VOID
+ );
+
+/**
+ When OVMF is lauched with -bios parameter, UEFI variables will be
+ partially emulated, and non-volatile variables may lose their contents
+ after a reboot. This makes the secure boot feature not working.
+
+ This function is used to initialize the EmuVariableNvStore
+ with the conent in PcdOvmfFlashNvStorageVariableBase.
+
+ @param[in] EmuVariableNvStore - A pointer to EmuVariableNvStore
+
+ @retval EFI_SUCCESS - Successfully init the EmuVariableNvStore
+ @retval Others - As the error code indicates
+ */
+EFI_STATUS
+EFIAPI
+PlatformInitEmuVariableNvStore (
+ IN VOID *EmuVariableNvStore
+ );
+
#endif // PLATFORM_INIT_LIB_H_
diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c
index c3d34e43af5a..2582689ffe35 100644
--- a/OvmfPkg/Library/PlatformInitLib/Platform.c
+++ b/OvmfPkg/Library/PlatformInitLib/Platform.c
@@ -25,10 +25,13 @@
#include <IndustryStandard/Pci22.h>
#include <IndustryStandard/Q35MchIch9.h>
#include <IndustryStandard/QemuCpuHotplug.h>
+#include <Library/MemoryAllocationLib.h>
#include <Library/QemuFwCfgLib.h>
#include <Library/QemuFwCfgS3Lib.h>
#include <Library/QemuFwCfgSimpleParserLib.h>
#include <Library/PciLib.h>
+#include <Guid/SystemNvDataGuid.h>
+#include <Guid/VariableFormat.h>
#include <OvmfPlatforms.h>
#include <Library/PlatformInitLib.h>
@@ -576,3 +579,238 @@ PlatformMaxCpuCountInitialization (
PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber = MaxCpuCount;
PlatformInfoHob->PcdCpuBootLogicalProcessorNumber = BootCpuCount;
}
+
+/**
+ Check padding data all bit should be 1.
+
+ @param[in] Buffer - A pointer to buffer header
+ @param[in] BufferSize - Buffer size
+
+ @retval TRUE - The padding data is valid.
+ @retval TRUE - The padding data is invalid.
+
+**/
+BOOLEAN
+CheckPaddingData (
+ IN UINT8 *Buffer,
+ IN UINT32 BufferSize
+ )
+{
+ UINT32 index;
+
+ for (index = 0; index < BufferSize; index++) {
+ if (Buffer[index] != 0xFF) {
+ return FALSE;
+ }
+ }
+
+ return TRUE;
+}
+
+/**
+ Check the integrity of NvVarStore.
+
+ @param[in] NvVarStoreBase - A pointer to NvVarStore header
+ @param[in] NvVarStoreSize - NvVarStore size
+
+ @retval TRUE - The NvVarStore is valid.
+ @retval FALSE - The NvVarStore is invalid.
+
+**/
+BOOLEAN
+EFIAPI
+PlatformValidateNvVarStore (
+ IN UINT8 *NvVarStoreBase,
+ IN UINT32 NvVarStoreSize
+ )
+{
+ UINT16 Checksum;
+ UINTN VariableBase;
+ UINT32 VariableOffset;
+ UINT32 VariableOffsetBeforeAlign;
+ EFI_FIRMWARE_VOLUME_HEADER *NvVarStoreFvHeader;
+ VARIABLE_STORE_HEADER *NvVarStoreHeader;
+ AUTHENTICATED_VARIABLE_HEADER *VariableHeader;
+
+ static EFI_GUID FvHdrGUID = EFI_SYSTEM_NV_DATA_FV_GUID;
+ static EFI_GUID VarStoreHdrGUID = EFI_AUTHENTICATED_VARIABLE_GUID;
+
+ VariableOffset = 0;
+
+ if (NvVarStoreBase == NULL) {
+ DEBUG ((DEBUG_ERROR, "NvVarStore pointer is NULL.\n"));
+ return FALSE;
+ }
+
+ //
+ // Verify the header zerovetor, filesystemguid,
+ // revision, signature, attributes, fvlength, checksum
+ // HeaderLength cannot be an odd number
+ //
+ NvVarStoreFvHeader = (EFI_FIRMWARE_VOLUME_HEADER *)NvVarStoreBase;
+
+ if ((!IsZeroBuffer (NvVarStoreFvHeader->ZeroVector, 16)) ||
+ (!CompareGuid (&FvHdrGUID, &NvVarStoreFvHeader->FileSystemGuid)) ||
+ (NvVarStoreFvHeader->Signature != EFI_FVH_SIGNATURE) ||
+ (NvVarStoreFvHeader->Attributes != 0x4feff) ||
+ (NvVarStoreFvHeader->Revision != EFI_FVH_REVISION) ||
+ (NvVarStoreFvHeader->FvLength != NvVarStoreSize)
+ )
+ {
+ DEBUG ((DEBUG_ERROR, "NvVarStore FV headers were invalid.\n"));
+ return FALSE;
+ }
+
+ //
+ // Verify the header checksum
+ //
+ Checksum = CalculateSum16 ((VOID *)NvVarStoreFvHeader, NvVarStoreFvHeader->HeaderLength);
+
+ if (Checksum != 0) {
+ DEBUG ((DEBUG_ERROR, "NvVarStore FV checksum was invalid.\n"));
+ return FALSE;
+ }
+
+ //
+ // Verify the header signature, size, format, state
+ //
+ NvVarStoreHeader = (VARIABLE_STORE_HEADER *)(NvVarStoreBase + NvVarStoreFvHeader->HeaderLength);
+ if ((!CompareGuid (&VarStoreHdrGUID, &NvVarStoreHeader->Signature)) ||
+ (NvVarStoreHeader->Format != VARIABLE_STORE_FORMATTED) ||
+ (NvVarStoreHeader->State != VARIABLE_STORE_HEALTHY) ||
+ (NvVarStoreHeader->Size > (NvVarStoreFvHeader->FvLength - NvVarStoreFvHeader->HeaderLength)) ||
+ (NvVarStoreHeader->Size < sizeof (VARIABLE_STORE_HEADER))
+ )
+ {
+ DEBUG ((DEBUG_ERROR, "NvVarStore header signature/size/format/state were invalid.\n"));
+ return FALSE;
+ }
+
+ //
+ // Verify the header startId, state
+ // Verify data to the end
+ //
+ VariableBase = (UINTN)NvVarStoreBase + NvVarStoreFvHeader->HeaderLength + sizeof (VARIABLE_STORE_HEADER);
+ while (VariableOffset < (NvVarStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER))) {
+ VariableHeader = (AUTHENTICATED_VARIABLE_HEADER *)(VariableBase + VariableOffset);
+ if (VariableHeader->StartId != VARIABLE_DATA) {
+ if (!CheckPaddingData ((UINT8 *)VariableHeader, NvVarStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER) - VariableOffset)) {
+ DEBUG ((DEBUG_ERROR, "NvVarStore variable header StartId was invalid.\n"));
+ return FALSE;
+ }
+
+ VariableOffset = NvVarStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER);
+ } else {
+ if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) ||
+ (VariableHeader->State == VAR_DELETED) ||
+ (VariableHeader->State == VAR_HEADER_VALID_ONLY) ||
+ (VariableHeader->State == VAR_ADDED)))
+ {
+ DEBUG ((DEBUG_ERROR, "NvVarStore Variable header State was invalid.\n"));
+ return FALSE;
+ }
+
+ VariableOffset += sizeof (AUTHENTICATED_VARIABLE_HEADER) + VariableHeader->NameSize + VariableHeader->DataSize;
+ // Verify VariableOffset should be less than or equal NvVarStoreHeader->Size - sizeof(VARIABLE_STORE_HEADER)
+ if (VariableOffset > (NvVarStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER))) {
+ DEBUG ((DEBUG_ERROR, "NvVarStore Variable header VariableOffset was invalid.\n"));
+ return FALSE;
+ }
+
+ VariableOffsetBeforeAlign = VariableOffset;
+ // 4 byte align
+ VariableOffset = (VariableOffset + 3) & (UINTN)(~3);
+
+ if (!CheckPaddingData ((UINT8 *)(VariableBase + VariableOffsetBeforeAlign), VariableOffset - VariableOffsetBeforeAlign)) {
+ DEBUG ((DEBUG_ERROR, "NvVarStore Variable header PaddingData was invalid.\n"));
+ return FALSE;
+ }
+ }
+ }
+
+ return TRUE;
+}
+
+/**
+ Allocate storage for NV variables early on so it will be
+ at a consistent address. Since VM memory is preserved
+ across reboots, this allows the NV variable storage to survive
+ a VM reboot.
+
+ *
+ * @retval VOID* The pointer to the storage for NV Variables
+ */
+VOID *
+EFIAPI
+PlatformReserveEmuVariableNvStore (
+ VOID
+ )
+{
+ VOID *VariableStore;
+ UINT32 VarStoreSize;
+
+ VarStoreSize = 2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize);
+ //
+ // Allocate storage for NV variables early on so it will be
+ // at a consistent address. Since VM memory is preserved
+ // across reboots, this allows the NV variable storage to survive
+ // a VM reboot.
+ //
+ VariableStore =
+ AllocateRuntimePages (
+ EFI_SIZE_TO_PAGES (VarStoreSize)
+ );
+ DEBUG ((
+ DEBUG_INFO,
+ "Reserved variable store memory: 0x%p; size: %dkb\n",
+ VariableStore,
+ VarStoreSize / 1024
+ ));
+
+ return VariableStore;
+}
+
+/**
+ When OVMF is lauched with -bios parameter, UEFI variables will be
+ partially emulated, and non-volatile variables may lose their contents
+ after a reboot. This makes the secure boot feature not working.
+
+ This function is used to initialize the EmuVariableNvStore
+ with the conent in PcdOvmfFlashNvStorageVariableBase.
+
+ @param[in] EmuVariableNvStore - A pointer to EmuVariableNvStore
+
+ @retval EFI_SUCCESS - Successfully init the EmuVariableNvStore
+ @retval Others - As the error code indicates
+ */
+EFI_STATUS
+EFIAPI
+PlatformInitEmuVariableNvStore (
+ IN VOID *EmuVariableNvStore
+ )
+{
+ UINT8 *Base;
+ UINT32 Size;
+ UINT32 EmuVariableNvStoreSize;
+
+ EmuVariableNvStoreSize = 2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize);
+ if ((EmuVariableNvStore == NULL) || (EmuVariableNvStoreSize == 0)) {
+ DEBUG ((DEBUG_ERROR, "Invalid EmuVariableNvStore parameter.\n"));
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Base = (UINT8 *)(UINTN)PcdGet32 (PcdOvmfFlashNvStorageVariableBase);
+ Size = (UINT32)PcdGet32 (PcdFlashNvStorageVariableSize);
+ ASSERT (Size < EmuVariableNvStoreSize);
+
+ if (!PlatformValidateNvVarStore (Base, PcdGet32 (PcdCfvRawDataSize))) {
+ ASSERT (FALSE);
+ return EFI_INVALID_PARAMETER;
+ }
+
+ DEBUG ((DEBUG_INFO, "Init EmuVariableNvStore with the content in FlashNvStorage\n"));
+
+ CopyMem (EmuVariableNvStore, Base, Size);
+
+ return EFI_SUCCESS;
+}
diff --git a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
index d2fa2d998df8..86a82ad3e084 100644
--- a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
+++ b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
@@ -47,6 +47,7 @@
HobLib
QemuFwCfgLib
QemuFwCfgSimpleParserLib
+ MemoryAllocationLib
MtrrLib
PcdLib
PciLib
@@ -96,6 +97,8 @@
gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
gUefiOvmfPkgTokenSpaceGuid.PcdTdxAcceptPageSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataSize
[FeaturePcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH V5 4/8] OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore
2022-09-06 4:35 [PATCH V5 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
` (2 preceding siblings ...)
2022-09-06 4:35 ` [PATCH V5 3/8] OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore Min Xu
@ 2022-09-06 4:35 ` Min Xu
2023-03-21 9:31 ` [edk2-devel] " joeyli
2022-09-06 4:35 ` [PATCH V5 5/8] OvmfPkg: Reserve and init EmuVariableNvStore in Pei-less Startup Min Xu
` (4 subsequent siblings)
8 siblings, 1 reply; 12+ messages in thread
From: Min Xu @ 2022-09-06 4:35 UTC (permalink / raw)
To: devel
Cc: Min M Xu, Erdem Aktas, James Bottomley, Jiewen Yao, Tom Lendacky,
Gerd Hoffmann
From: Min M Xu <min.m.xu@intel.com>
ReserveEmuVariableNvStore is updated with below 2 functions defined in
PlatformInitLib:
- PlatformReserveEmuVariableNvStore
- PlatformInitEmuVariableNvStore
PlatformInitEmuVariableNvStore works when secure boot feature is enabled.
This is because secure boot needs the EFI variables (PK/KEK/DB/DBX, etc)
and EmuVariableNvStore is cleared when OVMF is launched with -bios
parameter.
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/PlatformPei/Platform.c | 25 +++++++------------------
1 file changed, 7 insertions(+), 18 deletions(-)
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 009db67ee60a..b1f8140d6041 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -220,24 +220,13 @@ ReserveEmuVariableNvStore (
EFI_PHYSICAL_ADDRESS VariableStore;
RETURN_STATUS PcdStatus;
- //
- // Allocate storage for NV variables early on so it will be
- // at a consistent address. Since VM memory is preserved
- // across reboots, this allows the NV variable storage to survive
- // a VM reboot.
- //
- VariableStore =
- (EFI_PHYSICAL_ADDRESS)(UINTN)
- AllocateRuntimePages (
- EFI_SIZE_TO_PAGES (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize))
- );
- DEBUG ((
- DEBUG_INFO,
- "Reserved variable store memory: 0x%lX; size: %dkb\n",
- VariableStore,
- (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
- ));
- PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
+ VariableStore = (EFI_PHYSICAL_ADDRESS)(UINTN)PlatformReserveEmuVariableNvStore ();
+ PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
+
+ #ifdef SECURE_BOOT_FEATURE_ENABLED
+ PlatformInitEmuVariableNvStore ((VOID *)(UINTN)VariableStore);
+ #endif
+
ASSERT_RETURN_ERROR (PcdStatus);
}
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH V5 4/8] OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore
2022-09-06 4:35 ` [PATCH V5 4/8] OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore Min Xu
@ 2023-03-21 9:31 ` joeyli
2023-03-24 0:38 ` Min Xu
0 siblings, 1 reply; 12+ messages in thread
From: joeyli @ 2023-03-21 9:31 UTC (permalink / raw)
To: devel, min.m.xu
Cc: Erdem Aktas, James Bottomley, Jiewen Yao, Tom Lendacky,
Gerd Hoffmann
Hi Min M Xu,
I have filed a EDK2 bug relates to this patch:
Bug 4379 - Got NvVarStore FV headers were invalid when using OVMF with AMD SEV
https://bugzilla.tianocore.org/show_bug.cgi?id=4379
I got a "NvVarStore FV headers were invalid." assert when using OVMF with AMD
SEV. After reverted this patch, the assert is gone.
Thanks!
Joey Lee
On Tue, Sep 06, 2022 at 12:35:56PM +0800, Min Xu via groups.io wrote:
> From: Min M Xu <min.m.xu@intel.com>
>
> ReserveEmuVariableNvStore is updated with below 2 functions defined in
> PlatformInitLib:
> - PlatformReserveEmuVariableNvStore
> - PlatformInitEmuVariableNvStore
>
> PlatformInitEmuVariableNvStore works when secure boot feature is enabled.
> This is because secure boot needs the EFI variables (PK/KEK/DB/DBX, etc)
> and EmuVariableNvStore is cleared when OVMF is launched with -bios
> parameter.
>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
> OvmfPkg/PlatformPei/Platform.c | 25 +++++++------------------
> 1 file changed, 7 insertions(+), 18 deletions(-)
>
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 009db67ee60a..b1f8140d6041 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -220,24 +220,13 @@ ReserveEmuVariableNvStore (
> EFI_PHYSICAL_ADDRESS VariableStore;
> RETURN_STATUS PcdStatus;
>
> - //
> - // Allocate storage for NV variables early on so it will be
> - // at a consistent address. Since VM memory is preserved
> - // across reboots, this allows the NV variable storage to survive
> - // a VM reboot.
> - //
> - VariableStore =
> - (EFI_PHYSICAL_ADDRESS)(UINTN)
> - AllocateRuntimePages (
> - EFI_SIZE_TO_PAGES (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize))
> - );
> - DEBUG ((
> - DEBUG_INFO,
> - "Reserved variable store memory: 0x%lX; size: %dkb\n",
> - VariableStore,
> - (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
> - ));
> - PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
> + VariableStore = (EFI_PHYSICAL_ADDRESS)(UINTN)PlatformReserveEmuVariableNvStore ();
> + PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
> +
> + #ifdef SECURE_BOOT_FEATURE_ENABLED
> + PlatformInitEmuVariableNvStore ((VOID *)(UINTN)VariableStore);
> + #endif
> +
> ASSERT_RETURN_ERROR (PcdStatus);
> }
>
> --
> 2.29.2.windows.2
>
>
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH V5 4/8] OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore
2023-03-21 9:31 ` [edk2-devel] " joeyli
@ 2023-03-24 0:38 ` Min Xu
0 siblings, 0 replies; 12+ messages in thread
From: Min Xu @ 2023-03-24 0:38 UTC (permalink / raw)
To: joeyli, devel@edk2.groups.io
Cc: Aktas, Erdem, James Bottomley, Yao, Jiewen, Tom Lendacky,
Gerd Hoffmann, Xu, Min M
Hi, joeyli
Please see my comments in https://bugzilla.tianocore.org/show_bug.cgi?id=4379
I need your help to catch some logs in AMD SEV environment for this problem.
Thanks
Min
> -----Original Message-----
> From: joeyli <jlee@suse.com>
> Sent: Tuesday, March 21, 2023 5:32 PM
> To: devel@edk2.groups.io; Xu, Min M <min.m.xu@intel.com>
> Cc: Aktas, Erdem <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: Re: [edk2-devel] [PATCH V5 4/8] OvmfPkg/PlatformPei: Update
> ReserveEmuVariableNvStore
>
> Hi Min M Xu,
>
> I have filed a EDK2 bug relates to this patch:
>
> Bug 4379 - Got NvVarStore FV headers were invalid when using OVMF with
> AMD SEV
> https://bugzilla.tianocore.org/show_bug.cgi?id=4379
>
> I got a "NvVarStore FV headers were invalid." assert when using OVMF with
> AMD SEV. After reverted this patch, the assert is gone.
>
> Thanks!
> Joey Lee
>
> On Tue, Sep 06, 2022 at 12:35:56PM +0800, Min Xu via groups.io wrote:
> > From: Min M Xu <min.m.xu@intel.com>
> >
> > ReserveEmuVariableNvStore is updated with below 2 functions defined in
> > PlatformInitLib:
> > - PlatformReserveEmuVariableNvStore
> > - PlatformInitEmuVariableNvStore
> >
> > PlatformInitEmuVariableNvStore works when secure boot feature is enabled.
> > This is because secure boot needs the EFI variables (PK/KEK/DB/DBX,
> > etc) and EmuVariableNvStore is cleared when OVMF is launched with
> > -bios parameter.
> >
> > Cc: Erdem Aktas <erdemaktas@google.com>
> > Cc: James Bottomley <jejb@linux.ibm.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Min Xu <min.m.xu@intel.com>
> > ---
> > OvmfPkg/PlatformPei/Platform.c | 25 +++++++------------------
> > 1 file changed, 7 insertions(+), 18 deletions(-)
> >
> > diff --git a/OvmfPkg/PlatformPei/Platform.c
> > b/OvmfPkg/PlatformPei/Platform.c index 009db67ee60a..b1f8140d6041
> > 100644
> > --- a/OvmfPkg/PlatformPei/Platform.c
> > +++ b/OvmfPkg/PlatformPei/Platform.c
> > @@ -220,24 +220,13 @@ ReserveEmuVariableNvStore (
> > EFI_PHYSICAL_ADDRESS VariableStore;
> > RETURN_STATUS PcdStatus;
> >
> > - //
> > - // Allocate storage for NV variables early on so it will be
> > - // at a consistent address. Since VM memory is preserved
> > - // across reboots, this allows the NV variable storage to survive
> > - // a VM reboot.
> > - //
> > - VariableStore =
> > - (EFI_PHYSICAL_ADDRESS)(UINTN)
> > - AllocateRuntimePages (
> > - EFI_SIZE_TO_PAGES (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize))
> > - );
> > - DEBUG ((
> > - DEBUG_INFO,
> > - "Reserved variable store memory: 0x%lX; size: %dkb\n",
> > - VariableStore,
> > - (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
> > - ));
> > - PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved,
> > VariableStore);
> > + VariableStore =
> (EFI_PHYSICAL_ADDRESS)(UINTN)PlatformReserveEmuVariableNvStore ();
> > + PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
> > +
> > + #ifdef SECURE_BOOT_FEATURE_ENABLED
> > + PlatformInitEmuVariableNvStore ((VOID *)(UINTN)VariableStore);
> > + #endif
> > +
> > ASSERT_RETURN_ERROR (PcdStatus);
> > }
> >
> > --
> > 2.29.2.windows.2
> >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V5 5/8] OvmfPkg: Reserve and init EmuVariableNvStore in Pei-less Startup
2022-09-06 4:35 [PATCH V5 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
` (3 preceding siblings ...)
2022-09-06 4:35 ` [PATCH V5 4/8] OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore Min Xu
@ 2022-09-06 4:35 ` Min Xu
2022-09-06 4:35 ` [PATCH V5 6/8] OvmfPkg/NvVarsFileLib: Shortcut ConnectNvVarsToFileSystem in secure-boot Min Xu
` (3 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Min Xu @ 2022-09-06 4:35 UTC (permalink / raw)
To: devel
Cc: Min M Xu, Erdem Aktas, James Bottomley, Jiewen Yao, Tom Lendacky,
Gerd Hoffmann
From: Min M Xu <min.m.xu@intel.com>
EmuVariableNvStore is reserved and init with below 2 functions defined in
PlatformInitLib:
- PlatformReserveEmuVariableNvStore
- PlatformInitEmuVariableNvStore
PlatformInitEmuVariableNvStore works when secure boot feature is enabled.
This is because secure boot needs the EFI variables (PK/KEK/DB/DBX, etc)
and EmuVariableNvStore is cleared when OVMF is launched with -bios
parameter.
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
index 7502ec44669e..380e71597206 100644
--- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
+++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
@@ -42,6 +42,7 @@ InitializePlatform (
)
{
UINT32 LowerMemorySize;
+ VOID *VariableStore;
DEBUG ((DEBUG_INFO, "InitializePlatform in Pei-less boot\n"));
PlatformDebugDumpCmos ();
@@ -79,6 +80,12 @@ InitializePlatform (
LowerMemorySize
));
+ VariableStore = PlatformReserveEmuVariableNvStore ();
+ PlatformInfoHob->PcdEmuVariableNvStoreReserved = (UINT64)(UINTN)VariableStore;
+ #ifdef SECURE_BOOT_FEATURE_ENABLED
+ PlatformInitEmuVariableNvStore (VariableStore);
+ #endif
+
if (TdIsEnabled ()) {
PlatformTdxPublishRamRegions ();
} else {
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH V5 6/8] OvmfPkg/NvVarsFileLib: Shortcut ConnectNvVarsToFileSystem in secure-boot
2022-09-06 4:35 [PATCH V5 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
` (4 preceding siblings ...)
2022-09-06 4:35 ` [PATCH V5 5/8] OvmfPkg: Reserve and init EmuVariableNvStore in Pei-less Startup Min Xu
@ 2022-09-06 4:35 ` Min Xu
2022-09-06 4:35 ` [PATCH V5 7/8] OvmfPkg/TdxDxe: Set PcdEmuVariableNvStoreReserved Min Xu
` (2 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: Min Xu @ 2022-09-06 4:35 UTC (permalink / raw)
To: devel
Cc: Min M Xu, Erdem Aktas, James Bottomley, Jiewen Yao, Tom Lendacky,
Gerd Hoffmann
From: Min M Xu <min.m.xu@intel.com>
OvmfPkg/Library/NvVarsFileLib allows loading variables into emulated
varstore from a on-disk NvVars file. We can't allow that when secure
boot is active. So check secure-boot feature and shortcut the
ConnectNvVarsToFileSystem() function when sb is enabled.
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.c b/OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.c
index 21b71524ea48..72289da35819 100644
--- a/OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.c
+++ b/OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.c
@@ -28,6 +28,12 @@ ConnectNvVarsToFileSystem (
IN EFI_HANDLE FsHandle
)
{
+ #ifdef SECURE_BOOT_FEATURE_ENABLED
+
+ return EFI_UNSUPPORTED;
+
+ #else
+
EFI_STATUS Status;
//
@@ -46,6 +52,7 @@ ConnectNvVarsToFileSystem (
}
return Status;
+ #endif
}
/**
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH V5 7/8] OvmfPkg/TdxDxe: Set PcdEmuVariableNvStoreReserved
2022-09-06 4:35 [PATCH V5 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
` (5 preceding siblings ...)
2022-09-06 4:35 ` [PATCH V5 6/8] OvmfPkg/NvVarsFileLib: Shortcut ConnectNvVarsToFileSystem in secure-boot Min Xu
@ 2022-09-06 4:35 ` Min Xu
2022-09-06 4:36 ` [PATCH V5 8/8] OvmfPkg: Add build-flag SECURE_BOOT_FEATURE_ENABLED Min Xu
2022-09-06 7:22 ` [PATCH V5 0/8] Enable secure-boot when lauch OVMF with -bios parameter Yao, Jiewen
8 siblings, 0 replies; 12+ messages in thread
From: Min Xu @ 2022-09-06 4:35 UTC (permalink / raw)
To: devel
Cc: Min M Xu, Erdem Aktas, James Bottomley, Jiewen Yao, Tom Lendacky,
Gerd Hoffmann
From: Min M Xu <min.m.xu@intel.com>
Set PcdEmuVariableNvStoreReserved with the value in PlatformInfoHob. It
is the address of the EmuVariableNvStore reserved in Pei-less startup.
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/TdxDxe/TdxDxe.c | 2 ++
OvmfPkg/TdxDxe/TdxDxe.inf | 1 +
2 files changed, 3 insertions(+)
diff --git a/OvmfPkg/TdxDxe/TdxDxe.c b/OvmfPkg/TdxDxe/TdxDxe.c
index 2318db989792..837f1f8e3024 100644
--- a/OvmfPkg/TdxDxe/TdxDxe.c
+++ b/OvmfPkg/TdxDxe/TdxDxe.c
@@ -64,6 +64,8 @@ SetPcdSettings (
PlatformInfoHob->PcdCpuBootLogicalProcessorNumber
));
+ PcdSet64S (PcdEmuVariableNvStoreReserved, PlatformInfoHob->PcdEmuVariableNvStoreReserved);
+
if (TdIsEnabled ()) {
PcdStatus = PcdSet64S (PcdTdxSharedBitMask, TdSharedPageMask ());
ASSERT_RETURN_ERROR (PcdStatus);
diff --git a/OvmfPkg/TdxDxe/TdxDxe.inf b/OvmfPkg/TdxDxe/TdxDxe.inf
index a7e0abda1522..3ce8a5c32c98 100644
--- a/OvmfPkg/TdxDxe/TdxDxe.inf
+++ b/OvmfPkg/TdxDxe/TdxDxe.inf
@@ -68,3 +68,4 @@
gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr
gEfiMdeModulePkgTokenSpaceGuid.PcdTdxSharedBitMask
gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack
+ gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH V5 8/8] OvmfPkg: Add build-flag SECURE_BOOT_FEATURE_ENABLED
2022-09-06 4:35 [PATCH V5 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
` (6 preceding siblings ...)
2022-09-06 4:35 ` [PATCH V5 7/8] OvmfPkg/TdxDxe: Set PcdEmuVariableNvStoreReserved Min Xu
@ 2022-09-06 4:36 ` Min Xu
2022-09-06 7:22 ` [PATCH V5 0/8] Enable secure-boot when lauch OVMF with -bios parameter Yao, Jiewen
8 siblings, 0 replies; 12+ messages in thread
From: Min Xu @ 2022-09-06 4:36 UTC (permalink / raw)
To: devel
Cc: Min M Xu, Erdem Aktas, James Bottomley, Jiewen Yao, Tom Lendacky,
Gerd Hoffmann
From: Min M Xu <min.m.xu@intel.com>
SECURE_BOOT_FEATURE_ENABLED is the build-flag defined when secure boot
is enabled. Currently this flag is used in below lib:
- OvmfPkg/PlatformPei
- PeilessStartupLib
So it is defined in below 5 .dsc
- OvmfPkg/CloudHv/CloudHvX64.dsc
- OvmfPkg/IntelTdx/IntelTdxX64.dsc
- OvmfPkg/OvmfPkgIa32.dsc
- OvmfPkg/OvmfPkgIa32X64.dsc
- OvmfPkg/OvmfPkgX64.dsc
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/CloudHv/CloudHvX64.dsc | 9 +++++++++
OvmfPkg/IntelTdx/IntelTdxX64.dsc | 9 +++++++++
OvmfPkg/OvmfPkgIa32.dsc | 9 +++++++++
OvmfPkg/OvmfPkgIa32X64.dsc | 9 +++++++++
OvmfPkg/OvmfPkgX64.dsc | 9 +++++++++
5 files changed, 45 insertions(+)
diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
index f0d700f14477..0f0fc9a1de73 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.dsc
+++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
@@ -93,6 +93,15 @@
INTEL:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
GCC:*_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
+ #
+ # SECURE_BOOT_FEATURE_ENABLED
+ #
+!if $(SECURE_BOOT_ENABLE) == TRUE
+ MSFT:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
+ INTEL:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
+ GCC:*_*_*_CC_FLAGS = -D SECURE_BOOT_FEATURE_ENABLED
+!endif
+
!include NetworkPkg/NetworkBuildOptions.dsc.inc
[BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
index 71b1cf8e7090..e05fe36cbfa8 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
@@ -85,6 +85,15 @@
INTEL:*_*_*_CC_FLAGS = /D TDX_PEI_LESS_BOOT
GCC:*_*_*_CC_FLAGS = -D TDX_PEI_LESS_BOOT
+ #
+ # SECURE_BOOT_FEATURE_ENABLED
+ #
+!if $(SECURE_BOOT_ENABLE) == TRUE
+ MSFT:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
+ INTEL:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
+ GCC:*_*_*_CC_FLAGS = -D SECURE_BOOT_FEATURE_ENABLED
+!endif
+
[BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
XCODE:*_*_*_DLINK_FLAGS = -seg1addr 0x1000 -segalign 0x1000
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 797a543b95a9..367ddeb2da5f 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -94,6 +94,15 @@
INTEL:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
GCC:*_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
+ #
+ # SECURE_BOOT_FEATURE_ENABLED
+ #
+!if $(SECURE_BOOT_ENABLE) == TRUE
+ MSFT:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
+ INTEL:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
+ GCC:*_*_*_CC_FLAGS = -D SECURE_BOOT_FEATURE_ENABLED
+!endif
+
!include NetworkPkg/NetworkBuildOptions.dsc.inc
[BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 9b1228e85024..37c4c2fadca4 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -98,6 +98,15 @@
INTEL:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
GCC:*_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
+ #
+ # SECURE_BOOT_FEATURE_ENABLED
+ #
+!if $(SECURE_BOOT_ENABLE) == TRUE
+ MSFT:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
+ INTEL:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
+ GCC:*_*_*_CC_FLAGS = -D SECURE_BOOT_FEATURE_ENABLED
+!endif
+
!include NetworkPkg/NetworkBuildOptions.dsc.inc
[BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 5a6b68bcb106..276bcc303779 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -111,6 +111,15 @@
INTEL:*_*_*_CC_FLAGS = /D TDX_GUEST_SUPPORTED
GCC:*_*_*_CC_FLAGS = -D TDX_GUEST_SUPPORTED
+ #
+ # SECURE_BOOT_FEATURE_ENABLED
+ #
+!if $(SECURE_BOOT_ENABLE) == TRUE
+ MSFT:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
+ INTEL:*_*_*_CC_FLAGS = /D SECURE_BOOT_FEATURE_ENABLED
+ GCC:*_*_*_CC_FLAGS = -D SECURE_BOOT_FEATURE_ENABLED
+!endif
+
!include NetworkPkg/NetworkBuildOptions.dsc.inc
[BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH V5 0/8] Enable secure-boot when lauch OVMF with -bios parameter
2022-09-06 4:35 [PATCH V5 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
` (7 preceding siblings ...)
2022-09-06 4:36 ` [PATCH V5 8/8] OvmfPkg: Add build-flag SECURE_BOOT_FEATURE_ENABLED Min Xu
@ 2022-09-06 7:22 ` Yao, Jiewen
8 siblings, 0 replies; 12+ messages in thread
From: Yao, Jiewen @ 2022-09-06 7:22 UTC (permalink / raw)
To: Xu, Min M, devel@edk2.groups.io
Cc: Leif Lindholm, Ard Biesheuvel, Chang, Abner, Schaefer, Daniel,
Aktas, Erdem, James Bottomley, Tom Lendacky, Gerd Hoffmann
Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
Merged https://github.com/tianocore/edk2/pull/3292
> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Tuesday, September 6, 2022 12:36 PM
> To: devel@edk2.groups.io
> Cc: Xu, Min M <min.m.xu@intel.com>; Leif Lindholm
> <quic_llindhol@quicinc.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Chang, Abner <abner.chang@hpe.com>;
> Schaefer, Daniel <daniel.schaefer@hpe.com>; Aktas, Erdem
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH V5 0/8] Enable secure-boot when lauch OVMF with -bios
> parameter
>
> Secure-Boot related variables include the PK/KEK/DB/DBX and they are
> stored in NvVarStore (OVMF_VARS.fd). When lauching with -pflash,
> QEMU/OVMF will use emulated flash, and fully support UEFI variables.
> But when launching with -bios parameter, UEFI variables will be partially
> emulated, and non-volatile variables may lose their contents after a
> reboot. See OvmfPkg/README.
>
> Tdx guest is an example that -pflash is not supported. So this patch-set
> is designed to initialize the NvVarStore with the content of in
> OVMF_VARS.fd.
>
> patch 1:
> Add a new function (AllocateRuntimePages) in PrePiMemoryAllocationLib.
> This function will be used in PeilessStartupLib which will run
> in SEC phase.
>
> patch 2:
> Delete the TdxValidateCfv in PeilessStartupLib. Because it is going to
> be renamed to PlatformValidateNvVarStore and be moved to
> PlatformInitLib.
>
> patch 3 - 7:
> Then we add functions for EmuVariableNvStore in PlatformInitLib. This
> lib will then be called in OvmfPkg/PlatformPei and PeilessStartupLib.
> We also shortcut ConnectNvVarsToFileSystem in secure-boot.
>
> patch 8:
> At last a build-flag (SECURE_BOOT_FEATURE_ENABLED) is introduced in
> the dsc in OvmfPkg. Because the copy over of OVMR_VARS.fd to
> EmuVariableNvStore is only required when secure-boot is enabled.
>
> Code: https://github.com/mxu9/edk2/tree/secure-boot.v5
>
> v5 changes:
> - Set InternalAllocatePages to STATIC function according to the review
> comment.
> - Rebase the code to commit c05a218a9758.
>
> v4 chagnes:
> - "EmbeddedPkg: Add AllocateRuntimePages in PrePiMemoryAllocationLib"
> is
> missed in v3. It is added in this version.
> - No other changes.
>
> v3 changes:
> - Renamed TdxValidateCfv to PlatformValidateNvVarStore and
> implemented
> in PlatformInitlLib/Platform.c.
> - Shortcut ConnectNvVarsToFileSystem in secure-boot.
> - Other minor changes, such as adding log in
> PlatformInitEmuVariableNvStore.
>
> v2 changes:
> - The v1 title is "Enable Secure-Boot in Tdx guest". Because the
> patch-setwe was first designed to fix the gap when secure-boot feature
> was enabled in Tdx guest. After discussing with the community (see
> the disuccsions under https://edk2.groups.io/g/devel/message/90589)
> this patch-set can fix the secure-boot issue when OVMF is lauched
> with -bios parameter. So the title is updated.
> - Add a new function (AllocateRuntimePages) in PrePiMemoryAllocationLib.
> - Add build-flag SECURE_BOOT_FEATURE_ENABLED to control the copy
> over
> of OVMF_VARS.fd to EmuVariableNvStore.
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Abner Chang <abner.chang@hpe.com>
> Cc: Daniel Schaefer <daniel.schaefer@hpe.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com> [jejb]
> Cc: Jiewen Yao <jiewen.yao@intel.com> [jyao1]
> Cc: Tom Lendacky <thomas.lendacky@amd.com> [tlendacky]
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
>
> Min M Xu (8):
> EmbeddedPkg: Add AllocateRuntimePages in PrePiMemoryAllocationLib
> OvmfPkg/PeilessStartupLib: Delete TdxValidateCfv
> OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore
> OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore
> OvmfPkg: Reserve and init EmuVariableNvStore in Pei-less Startup
> OvmfPkg/NvVarsFileLib: Shortcut ConnectNvVarsToFileSystem in
> secure-boot
> OvmfPkg/TdxDxe: Set PcdEmuVariableNvStoreReserved
> OvmfPkg: Add build-flag SECURE_BOOT_FEATURE_ENABLED
>
> EmbeddedPkg/Include/Library/PrePiLib.h | 19 ++
> .../MemoryAllocationLib.c | 65 +++--
> OvmfPkg/CloudHv/CloudHvX64.dsc | 9 +
> OvmfPkg/Include/Library/PlatformInitLib.h | 51 ++++
> OvmfPkg/IntelTdx/IntelTdxX64.dsc | 9 +
> OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.c | 7 +
> OvmfPkg/Library/PeilessStartupLib/IntelTdx.c | 153 -----------
> .../PeilessStartupLib/PeilessStartup.c | 15 +-
> .../PeilessStartupInternal.h | 17 --
> OvmfPkg/Library/PlatformInitLib/Platform.c | 238
> ++++++++++++++++++
> .../PlatformInitLib/PlatformInitLib.inf | 3 +
> OvmfPkg/OvmfPkgIa32.dsc | 9 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 9 +
> OvmfPkg/OvmfPkgX64.dsc | 9 +
> OvmfPkg/PlatformPei/Platform.c | 25 +-
> OvmfPkg/TdxDxe/TdxDxe.c | 2 +
> OvmfPkg/TdxDxe/TdxDxe.inf | 1 +
> 17 files changed, 429 insertions(+), 212 deletions(-)
>
> --
> 2.29.2.windows.2
^ permalink raw reply [flat|nested] 12+ messages in thread