* [PATCH V2 1/8] OvmfPkg: Move TdxValidateCfv from PeilessStartupLib to PlatformInitLib
2022-06-26 3:05 [PATCH V2 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
@ 2022-06-26 3:05 ` Min Xu
2022-06-27 7:02 ` Gerd Hoffmann
2022-06-26 3:05 ` [PATCH V2 2/8] OvmfPkg: Validate Cfv integrity in Tdx guest Min Xu
` (6 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Min Xu @ 2022-06-26 3:05 UTC (permalink / raw)
To: devel
Cc: Min M Xu, Erdem Aktas, James Bottomley, Jiewen Yao, Gerd Hoffmann,
Tom Lendacky
From: Min M Xu <min.m.xu@intel.com>
TdxValidateCfv validates the integrity of Configuration FV (CFV). It was
implemented in PeilessStartupLib which is included in IntelTdxX64.
In OvmfPkgX64 we should validate CFV as well. So it is moved from
PeilessStartupLib to PlatformInitLib so that it can be called in both
OvmfPkgX64 and IntelTdxX64.
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/Include/Library/PlatformInitLib.h | 17 ++
OvmfPkg/Library/PeilessStartupLib/IntelTdx.c | 153 ------------------
.../PeilessStartupInternal.h | 17 --
OvmfPkg/Library/PlatformInitLib/IntelTdx.c | 153 ++++++++++++++++++
4 files changed, 170 insertions(+), 170 deletions(-)
diff --git a/OvmfPkg/Include/Library/PlatformInitLib.h b/OvmfPkg/Include/Library/PlatformInitLib.h
index 2987a367cc9c..a3acfb1fb196 100644
--- a/OvmfPkg/Include/Library/PlatformInitLib.h
+++ b/OvmfPkg/Include/Library/PlatformInitLib.h
@@ -234,4 +234,21 @@ PlatformTdxPublishRamRegions (
VOID
);
+/**
+ 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
+ );
+
#endif // PLATFORM_INIT_LIB_H_
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/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.
diff --git a/OvmfPkg/Library/PlatformInitLib/IntelTdx.c b/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
index c6d7c8bb6e0e..626a670c9c64 100644
--- a/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
+++ b/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
@@ -20,6 +20,8 @@
#include <Library/PeiServicesLib.h>
#include <Library/TdxLib.h>
#include <Library/SynchronizationLib.h>
+#include <Guid/SystemNvDataGuid.h>
+#include <Guid/VariableFormat.h>
#include <WorkArea.h>
#include <ConfidentialComputingGuestAttr.h>
@@ -561,3 +563,154 @@ PlatformTdxPublishRamRegions (
);
}
}
+
+/**
+ 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;
+}
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH V2 1/8] OvmfPkg: Move TdxValidateCfv from PeilessStartupLib to PlatformInitLib
2022-06-26 3:05 ` [PATCH V2 1/8] OvmfPkg: Move TdxValidateCfv from PeilessStartupLib to PlatformInitLib Min Xu
@ 2022-06-27 7:02 ` Gerd Hoffmann
2022-06-27 8:04 ` Min Xu
0 siblings, 1 reply; 20+ messages in thread
From: Gerd Hoffmann @ 2022-06-27 7:02 UTC (permalink / raw)
To: Min Xu; +Cc: devel, Erdem Aktas, James Bottomley, Jiewen Yao, Tom Lendacky
On Sun, Jun 26, 2022 at 11:05:50AM +0800, Min Xu wrote:
> From: Min M Xu <min.m.xu@intel.com>
>
> TdxValidateCfv validates the integrity of Configuration FV (CFV). It was
> implemented in PeilessStartupLib which is included in IntelTdxX64.
> --- a/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
> +++ b/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
> +/**
> + 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
> + )
Hmm, is there anything tdx-specific in this function?
Looks like generic verification of varstore structure to me.
take care,
Gerd
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH V2 1/8] OvmfPkg: Move TdxValidateCfv from PeilessStartupLib to PlatformInitLib
2022-06-27 7:02 ` Gerd Hoffmann
@ 2022-06-27 8:04 ` Min Xu
2022-06-27 8:41 ` [edk2-devel] " Gerd Hoffmann
0 siblings, 1 reply; 20+ messages in thread
From: Min Xu @ 2022-06-27 8:04 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: devel@edk2.groups.io, Aktas, Erdem, James Bottomley, Yao, Jiewen,
Tom Lendacky
On June 27, 2022 3:02 PM, Gerd Hoffmann wrote:
> On Sun, Jun 26, 2022 at 11:05:50AM +0800, Min Xu wrote:
> > From: Min M Xu <min.m.xu@intel.com>
> >
> > TdxValidateCfv validates the integrity of Configuration FV (CFV). It
> > was implemented in PeilessStartupLib which is included in IntelTdxX64.
>
> > --- a/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
> > +++ b/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
>
> > +/**
> > + 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
> > + )
>
> Hmm, is there anything tdx-specific in this function?
> Looks like generic verification of varstore structure to me.
>
There is no tdx-specific in this function. I will rename it to something more generic.
Thanks
Min
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/8] OvmfPkg: Move TdxValidateCfv from PeilessStartupLib to PlatformInitLib
2022-06-27 8:04 ` Min Xu
@ 2022-06-27 8:41 ` Gerd Hoffmann
2022-06-29 5:16 ` Min Xu
0 siblings, 1 reply; 20+ messages in thread
From: Gerd Hoffmann @ 2022-06-27 8:41 UTC (permalink / raw)
To: devel, min.m.xu; +Cc: Aktas, Erdem, James Bottomley, Yao, Jiewen, Tom Lendacky
On Mon, Jun 27, 2022 at 08:04:06AM +0000, Min Xu wrote:
> On June 27, 2022 3:02 PM, Gerd Hoffmann wrote:
> > On Sun, Jun 26, 2022 at 11:05:50AM +0800, Min Xu wrote:
> > > From: Min M Xu <min.m.xu@intel.com>
> > >
> > > TdxValidateCfv validates the integrity of Configuration FV (CFV). It
> > > was implemented in PeilessStartupLib which is included in IntelTdxX64.
> >
> > > --- a/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
> > > +++ b/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
> >
> > > +/**
> > > + 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
> > > + )
> >
> > Hmm, is there anything tdx-specific in this function?
> > Looks like generic verification of varstore structure to me.
> >
> There is no tdx-specific in this function. I will rename it to something more generic.
Also move out of IntelTdx.c please.
Does it make sense to call it right before calling PlatformInitEmuVariableNvStore()?
take care,
Gerd
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/8] OvmfPkg: Move TdxValidateCfv from PeilessStartupLib to PlatformInitLib
2022-06-27 8:41 ` [edk2-devel] " Gerd Hoffmann
@ 2022-06-29 5:16 ` Min Xu
0 siblings, 0 replies; 20+ messages in thread
From: Min Xu @ 2022-06-29 5:16 UTC (permalink / raw)
To: Gerd Hoffmann, devel@edk2.groups.io
Cc: Aktas, Erdem, James Bottomley, Yao, Jiewen, Tom Lendacky
On June 27, 2022 4:41 PM, Gerd Hoffmann wrote:
> On Mon, Jun 27, 2022 at 08:04:06AM +0000, Min Xu wrote:
> > On June 27, 2022 3:02 PM, Gerd Hoffmann wrote:
> > > On Sun, Jun 26, 2022 at 11:05:50AM +0800, Min Xu wrote:
> > > > From: Min M Xu <min.m.xu@intel.com>
> > > >
> > > > TdxValidateCfv validates the integrity of Configuration FV (CFV).
> > > > It was implemented in PeilessStartupLib which is included in
> IntelTdxX64.
> > >
> > > > --- a/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
> > > > +++ b/OvmfPkg/Library/PlatformInitLib/IntelTdx.c
> > >
> > > > +/**
> > > > + 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
> > > > + )
> > >
> > > Hmm, is there anything tdx-specific in this function?
> > > Looks like generic verification of varstore structure to me.
> > >
> > There is no tdx-specific in this function. I will rename it to something more
> generic.
>
> Also move out of IntelTdx.c please.
Yes, it will be in Platform.c, together with PlatformInitEmuVariableNvStore and PlatformReserveEmuVariableNvStore.
>
> Does it make sense to call it right before calling
> PlatformInitEmuVariableNvStore()?
I think it can be called in PlatformInitEmuVariableNvStore, as it is a pre-check before copy over the content to EmuVariableNvStore.
Thanks
Min
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH V2 2/8] OvmfPkg: Validate Cfv integrity in Tdx guest
2022-06-26 3:05 [PATCH V2 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
2022-06-26 3:05 ` [PATCH V2 1/8] OvmfPkg: Move TdxValidateCfv from PeilessStartupLib to PlatformInitLib Min Xu
@ 2022-06-26 3:05 ` Min Xu
2022-06-26 3:05 ` [PATCH V2 3/8] EmbeddedPkg: Add AllocateRuntimePages in PrePiMemoryAllocationLib Min Xu
` (5 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Min Xu @ 2022-06-26 3:05 UTC (permalink / raw)
To: devel
Cc: Min M Xu, Erdem Aktas, James Bottomley, Jiewen Yao, Gerd Hoffmann,
Tom Lendacky
From: Min M Xu <min.m.xu@intel.com>
Validate Configurtion FV (CFV) in Tdx guest.
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/Sec/SecMain.c | 8 ++++++++
OvmfPkg/Sec/SecMain.inf | 2 ++
2 files changed, 10 insertions(+)
diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
index 1167d22a68cc..f6c00b8dab96 100644
--- a/OvmfPkg/Sec/SecMain.c
+++ b/OvmfPkg/Sec/SecMain.c
@@ -768,6 +768,14 @@ SecCoreStartupWithStack (
if (ProcessTdxHobList () != EFI_SUCCESS) {
CpuDeadLoop ();
}
+
+ //
+ // Config FV (Cfv) contains the configuration information and its integrity
+ // should be validated.
+ //
+ if (!TdxValidateCfv ((UINT8 *)(UINTN)FixedPcdGet32 (PcdCfvBase), FixedPcdGet32 (PcdCfvRawDataSize))) {
+ CpuDeadLoop ();
+ }
}
#endif
diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
index 561a840f29c5..ae0094a15eda 100644
--- a/OvmfPkg/Sec/SecMain.inf
+++ b/OvmfPkg/Sec/SecMain.inf
@@ -84,6 +84,8 @@
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase
gUefiOvmfPkgTokenSpaceGuid.PcdTdxAcceptPageSize
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdCfvBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataSize
[FeaturePcd]
gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH V2 3/8] EmbeddedPkg: Add AllocateRuntimePages in PrePiMemoryAllocationLib
2022-06-26 3:05 [PATCH V2 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
2022-06-26 3:05 ` [PATCH V2 1/8] OvmfPkg: Move TdxValidateCfv from PeilessStartupLib to PlatformInitLib Min Xu
2022-06-26 3:05 ` [PATCH V2 2/8] OvmfPkg: Validate Cfv integrity in Tdx guest Min Xu
@ 2022-06-26 3:05 ` Min Xu
2022-07-20 8:49 ` Min Xu
2022-08-03 6:22 ` [edk2-devel] " Ard Biesheuvel
2022-06-26 3:05 ` [PATCH V2 4/8] OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore Min Xu
` (4 subsequent siblings)
7 siblings, 2 replies; 20+ messages in thread
From: Min Xu @ 2022-06-26 3:05 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>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
EmbeddedPkg/Include/Library/PrePiLib.h | 19 ++++++
.../MemoryAllocationLib.c | 64 ++++++++++++++-----
2 files changed, 67 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..9d7b34ad28fa 100644
--- a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
+++ b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
@@ -14,23 +14,11 @@
#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.
-
-**/
VOID *
EFIAPI
-AllocatePages (
- IN UINTN Pages
+InternalAllocatePages (
+ IN UINTN Pages,
+ IN EFI_MEMORY_TYPE MemoryType
)
{
EFI_PEI_HOB_POINTERS Hob;
@@ -65,12 +53,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] 20+ messages in thread* Re: [PATCH V2 3/8] EmbeddedPkg: Add AllocateRuntimePages in PrePiMemoryAllocationLib
2022-06-26 3:05 ` [PATCH V2 3/8] EmbeddedPkg: Add AllocateRuntimePages in PrePiMemoryAllocationLib Min Xu
@ 2022-07-20 8:49 ` Min Xu
2022-08-03 2:26 ` Min Xu
2022-08-03 6:22 ` [edk2-devel] " Ard Biesheuvel
1 sibling, 1 reply; 20+ messages in thread
From: Min Xu @ 2022-07-20 8:49 UTC (permalink / raw)
To: devel@edk2.groups.io, Leif Lindholm, Ard Biesheuvel, Chang, Abner,
Schaefer, Daniel
Cc: Gerd Hoffmann, Xu, Min M
Hi, Leif/Ard/Abner/Daniel
Since you're maintainers/reviewers of EmbeddedPkg. Do you have any comments to this patch?
Thanks
Min
> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Sunday, June 26, 2022 11:06 AM
> 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>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH V2 3/8] EmbeddedPkg: Add AllocateRuntimePages in
> PrePiMemoryAllocationLib
>
> 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>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
> EmbeddedPkg/Include/Library/PrePiLib.h | 19 ++++++
> .../MemoryAllocationLib.c | 64 ++++++++++++++-----
> 2 files changed, 67 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..9d7b34ad28fa 100644
> ---
> a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> +++
> b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> @@ -14,23 +14,11 @@
> #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.
> -
> -**/
> VOID *
> EFIAPI
> -AllocatePages (
> - IN UINTN Pages
> +InternalAllocatePages (
> + IN UINTN Pages,
> + IN EFI_MEMORY_TYPE MemoryType
> )
> {
> EFI_PEI_HOB_POINTERS Hob;
> @@ -65,12 +53,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 [flat|nested] 20+ messages in thread* Re: [PATCH V2 3/8] EmbeddedPkg: Add AllocateRuntimePages in PrePiMemoryAllocationLib
2022-07-20 8:49 ` Min Xu
@ 2022-08-03 2:26 ` Min Xu
0 siblings, 0 replies; 20+ messages in thread
From: Min Xu @ 2022-08-03 2:26 UTC (permalink / raw)
To: devel@edk2.groups.io, Leif Lindholm, Ard Biesheuvel, Chang, Abner,
Schaefer, Daniel
Cc: Gerd Hoffmann, Xu, Min M
Hi, Leif/Ard/Abner/Daniel
Since you're maintainers/reviewers of EmbeddedPkg. Do you have any comments to this patch?
The same reminder mail has been sent out two weeks ago but no response is received. Or anyone else can comment on this patch?
Thanks much!
Min
> -----Original Message-----
> From: Xu, Min M
> Sent: Wednesday, July 20, 2022 4:50 PM
> To: devel@edk2.groups.io; Leif Lindholm <leif@nuviainc.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Chang, Abner <abner.chang@hpe.com>;
> Schaefer, Daniel <daniel.schaefer@hpe.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>; Xu, Min M <min.m.xu@intel.com>
> Subject: RE: [PATCH V2 3/8] EmbeddedPkg: Add AllocateRuntimePages in
> PrePiMemoryAllocationLib
>
> Hi, Leif/Ard/Abner/Daniel
> Since you're maintainers/reviewers of EmbeddedPkg. Do you have any
> comments to this patch?
>
> Thanks
> Min
>
> > -----Original Message-----
> > From: Xu, Min M <min.m.xu@intel.com>
> > Sent: Sunday, June 26, 2022 11:06 AM
> > 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>; Gerd Hoffmann
> > <kraxel@redhat.com>
> > Subject: [PATCH V2 3/8] EmbeddedPkg: Add AllocateRuntimePages in
> > PrePiMemoryAllocationLib
> >
> > 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>
> > Signed-off-by: Min Xu <min.m.xu@intel.com>
> > ---
> > EmbeddedPkg/Include/Library/PrePiLib.h | 19 ++++++
> > .../MemoryAllocationLib.c | 64 ++++++++++++++-----
> > 2 files changed, 67 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..9d7b34ad28fa 100644
> > ---
> > a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> > +++
> > b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> > @@ -14,23 +14,11 @@
> > #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.
> > -
> > -**/
> > VOID *
> > EFIAPI
> > -AllocatePages (
> > - IN UINTN Pages
> > +InternalAllocatePages (
> > + IN UINTN Pages,
> > + IN EFI_MEMORY_TYPE MemoryType
> > )
> > {
> > EFI_PEI_HOB_POINTERS Hob;
> > @@ -65,12 +53,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 [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 3/8] EmbeddedPkg: Add AllocateRuntimePages in PrePiMemoryAllocationLib
2022-06-26 3:05 ` [PATCH V2 3/8] EmbeddedPkg: Add AllocateRuntimePages in PrePiMemoryAllocationLib Min Xu
2022-07-20 8:49 ` Min Xu
@ 2022-08-03 6:22 ` Ard Biesheuvel
1 sibling, 0 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2022-08-03 6:22 UTC (permalink / raw)
To: edk2-devel-groups-io, Min Xu
Cc: Leif Lindholm, Abner Chang, Daniel Schaefer, Gerd Hoffmann
On Sun, 26 Jun 2022 at 05:06, Min Xu <min.m.xu@intel.com> wrote:
>
> 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>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> EmbeddedPkg/Include/Library/PrePiLib.h | 19 ++++++
> .../MemoryAllocationLib.c | 64 ++++++++++++++-----
> 2 files changed, 67 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..9d7b34ad28fa 100644
> --- a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> +++ b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c
> @@ -14,23 +14,11 @@
> #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.
> -
> -**/
> VOID *
> EFIAPI
> -AllocatePages (
> - IN UINTN Pages
> +InternalAllocatePages (
> + IN UINTN Pages,
> + IN EFI_MEMORY_TYPE MemoryType
> )
> {
> EFI_PEI_HOB_POINTERS Hob;
> @@ -65,12 +53,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 [flat|nested] 20+ messages in thread
* [PATCH V2 4/8] OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore
2022-06-26 3:05 [PATCH V2 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
` (2 preceding siblings ...)
2022-06-26 3:05 ` [PATCH V2 3/8] EmbeddedPkg: Add AllocateRuntimePages in PrePiMemoryAllocationLib Min Xu
@ 2022-06-26 3:05 ` Min Xu
2022-06-27 9:10 ` Gerd Hoffmann
2022-06-26 3:05 ` [PATCH V2 5/8] OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore Min Xu
` (3 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Min Xu @ 2022-06-26 3:05 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 2 functions added for EmuVariableNvStore:
- PlatformReserveEmuVariableNvStore
- PlatformInitEmuVariableNvStore
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.
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>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/Include/Library/PlatformInitLib.h | 34 ++++++++
OvmfPkg/Library/PlatformInitLib/Platform.c | 77 +++++++++++++++++++
.../PlatformInitLib/PlatformInitLib.inf | 2 +
3 files changed, 113 insertions(+)
diff --git a/OvmfPkg/Include/Library/PlatformInitLib.h b/OvmfPkg/Include/Library/PlatformInitLib.h
index a3acfb1fb196..3a84a56be3c1 100644
--- a/OvmfPkg/Include/Library/PlatformInitLib.h
+++ b/OvmfPkg/Include/Library/PlatformInitLib.h
@@ -251,4 +251,38 @@ TdxValidateCfv (
IN UINT32 TdxCfvSize
);
+/**
+ 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..194768379f2b 100644
--- a/OvmfPkg/Library/PlatformInitLib/Platform.c
+++ b/OvmfPkg/Library/PlatformInitLib/Platform.c
@@ -25,6 +25,7 @@
#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>
@@ -576,3 +577,79 @@ PlatformMaxCpuCountInitialization (
PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber = MaxCpuCount;
PlatformInfoHob->PcdCpuBootLogicalProcessorNumber = BootCpuCount;
}
+
+/**
+ 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)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Base = (UINT8 *)(UINTN)PcdGet32 (PcdOvmfFlashNvStorageVariableBase);
+ Size = (UINT32)PcdGet32 (PcdFlashNvStorageVariableSize);
+ ASSERT (Size < EmuVariableNvStoreSize);
+
+ CopyMem (EmuVariableNvStore, Base, Size);
+
+ return EFI_SUCCESS;
+}
diff --git a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf
index d2fa2d998df8..fec1f8f24314 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,7 @@
gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
gUefiOvmfPkgTokenSpaceGuid.PcdTdxAcceptPageSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
[FeaturePcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
--
2.29.2.windows.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH V2 4/8] OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore
2022-06-26 3:05 ` [PATCH V2 4/8] OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore Min Xu
@ 2022-06-27 9:10 ` Gerd Hoffmann
2022-06-29 5:16 ` [edk2-devel] " Min Xu
0 siblings, 1 reply; 20+ messages in thread
From: Gerd Hoffmann @ 2022-06-27 9:10 UTC (permalink / raw)
To: Min Xu; +Cc: devel, Erdem Aktas, James Bottomley, Jiewen Yao, Tom Lendacky
> +VOID *
> +EFIAPI
> +PlatformReserveEmuVariableNvStore (
> + VOID
> + )
> + DEBUG ((
> + DEBUG_INFO,
> + "Reserved variable store memory: 0x%p; size: %dkb\n",
> + VariableStore,
> + VarStoreSize / 1024
> + ));
> +EFI_STATUS
> +EFIAPI
> +PlatformInitEmuVariableNvStore (
> + IN VOID *EmuVariableNvStore
> + )
> +{
I think it would be good to add a log message to this function too,
to make it easier to check variable initialization actually works
the way it is supposed to work.
take care,
Gerd
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [edk2-devel] [PATCH V2 4/8] OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore
2022-06-27 9:10 ` Gerd Hoffmann
@ 2022-06-29 5:16 ` Min Xu
0 siblings, 0 replies; 20+ messages in thread
From: Min Xu @ 2022-06-29 5:16 UTC (permalink / raw)
To: devel@edk2.groups.io, kraxel@redhat.com
Cc: Aktas, Erdem, James Bottomley, Yao, Jiewen, Tom Lendacky
On June 27, 2022 5:10 PM, Gerd Hoffmann wrote:
> > +VOID *
> > +EFIAPI
> > +PlatformReserveEmuVariableNvStore (
> > + VOID
> > + )
>
> > + DEBUG ((
> > + DEBUG_INFO,
> > + "Reserved variable store memory: 0x%p; size: %dkb\n",
> > + VariableStore,
> > + VarStoreSize / 1024
> > + ));
>
> > +EFI_STATUS
> > +EFIAPI
> > +PlatformInitEmuVariableNvStore (
> > + IN VOID *EmuVariableNvStore
> > + )
> > +{
>
> I think it would be good to add a log message to this function too, to make it
> easier to check variable initialization actually works the way it is supposed
> to work.
>
Thanks for reminder. Log will be added.
Thanks
Min
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH V2 5/8] OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore
2022-06-26 3:05 [PATCH V2 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
` (3 preceding siblings ...)
2022-06-26 3:05 ` [PATCH V2 4/8] OvmfPkg/PlatformInitLib: Add functions for EmuVariableNvStore Min Xu
@ 2022-06-26 3:05 ` Min Xu
2022-06-27 9:14 ` Gerd Hoffmann
2022-06-26 3:05 ` [PATCH V2 6/8] OvmfPkg: Reserve and init EmuVariableNvStore in Pei-less Startup Min Xu
` (2 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Min Xu @ 2022-06-26 3:05 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> [jejb]
Cc: Jiewen Yao <jiewen.yao@intel.com> [jyao1]
Cc: Tom Lendacky <thomas.lendacky@amd.com> [tlendacky]
Cc: 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] 20+ messages in thread* Re: [PATCH V2 5/8] OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore
2022-06-26 3:05 ` [PATCH V2 5/8] OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore Min Xu
@ 2022-06-27 9:14 ` Gerd Hoffmann
2022-06-29 5:17 ` [edk2-devel] " Min Xu
0 siblings, 1 reply; 20+ messages in thread
From: Gerd Hoffmann @ 2022-06-27 9:14 UTC (permalink / raw)
To: Min Xu; +Cc: devel, Erdem Aktas, James Bottomley, Jiewen Yao, Tom Lendacky
Hi,
> + #ifdef SECURE_BOOT_FEATURE_ENABLED
> + PlatformInitEmuVariableNvStore ((VOID *)(UINTN)VariableStore);
> + #endif
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. I think we need a simliar check there. Shortcutting
the ConnectNvVarsToFileSystem() function with a
#ifdef SECURE_BOOT_FEATURE_ENABLED
return EFI_NOT_SUPPORTED;
#endif
should do the trick I think.
thanks,
Gerd
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH V2 5/8] OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore
2022-06-27 9:14 ` Gerd Hoffmann
@ 2022-06-29 5:17 ` Min Xu
0 siblings, 0 replies; 20+ messages in thread
From: Min Xu @ 2022-06-29 5:17 UTC (permalink / raw)
To: devel@edk2.groups.io, kraxel@redhat.com
Cc: Aktas, Erdem, James Bottomley, Yao, Jiewen, Tom Lendacky
On June 27, 2022 5:14 PM, Gerd Hoffmann wrote:
> > + #ifdef SECURE_BOOT_FEATURE_ENABLED
> > + PlatformInitEmuVariableNvStore ((VOID *)(UINTN)VariableStore);
> > + #endif
>
> 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. I think we need a simliar check there. Shortcutting the
> ConnectNvVarsToFileSystem() function with a
>
> #ifdef SECURE_BOOT_FEATURE_ENABLED
> return EFI_NOT_SUPPORTED;
> #endif
>
> should do the trick I think.
>
A good suggestion. It will be updated in the next version.
Thanks
Min
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH V2 6/8] OvmfPkg: Reserve and init EmuVariableNvStore in Pei-less Startup
2022-06-26 3:05 [PATCH V2 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
` (4 preceding siblings ...)
2022-06-26 3:05 ` [PATCH V2 5/8] OvmfPkg/PlatformPei: Update ReserveEmuVariableNvStore Min Xu
@ 2022-06-26 3:05 ` Min Xu
2022-06-26 3:05 ` [PATCH V2 7/8] OvmfPkg/TdxDxe: Set PcdEmuVariableNvStoreReserved Min Xu
2022-06-26 3:05 ` [PATCH V2 8/8] OvmfPkg: Add build-flag SECURE_BOOT_FEATURE_ENABLED Min Xu
7 siblings, 0 replies; 20+ messages in thread
From: Min Xu @ 2022-06-26 3:05 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> [jejb]
Cc: Jiewen Yao <jiewen.yao@intel.com> [jyao1]
Cc: Tom Lendacky <thomas.lendacky@amd.com> [tlendacky]
Cc: 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 fdfefd00d732..663d5dacd3da 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] 20+ messages in thread* [PATCH V2 7/8] OvmfPkg/TdxDxe: Set PcdEmuVariableNvStoreReserved
2022-06-26 3:05 [PATCH V2 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
` (5 preceding siblings ...)
2022-06-26 3:05 ` [PATCH V2 6/8] OvmfPkg: Reserve and init EmuVariableNvStore in Pei-less Startup Min Xu
@ 2022-06-26 3:05 ` Min Xu
2022-06-26 3:05 ` [PATCH V2 8/8] OvmfPkg: Add build-flag SECURE_BOOT_FEATURE_ENABLED Min Xu
7 siblings, 0 replies; 20+ messages in thread
From: Min Xu @ 2022-06-26 3:05 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> [jejb]
Cc: Jiewen Yao <jiewen.yao@intel.com> [jyao1]
Cc: Tom Lendacky <thomas.lendacky@amd.com> [tlendacky]
Cc: 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] 20+ messages in thread* [PATCH V2 8/8] OvmfPkg: Add build-flag SECURE_BOOT_FEATURE_ENABLED
2022-06-26 3:05 [PATCH V2 0/8] Enable secure-boot when lauch OVMF with -bios parameter Min Xu
` (6 preceding siblings ...)
2022-06-26 3:05 ` [PATCH V2 7/8] OvmfPkg/TdxDxe: Set PcdEmuVariableNvStoreReserved Min Xu
@ 2022-06-26 3:05 ` Min Xu
7 siblings, 0 replies; 20+ messages in thread
From: Min Xu @ 2022-06-26 3:05 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> [jejb]
Cc: Jiewen Yao <jiewen.yao@intel.com> [jyao1]
Cc: Tom Lendacky <thomas.lendacky@amd.com> [tlendacky]
Cc: 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 ca601aa09d3a..2712731caf55 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 c662ae8720ff..f4f495a9d199 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 934edbbd2a7b..3126e695b7dd 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -89,6 +89,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 4f432c294958..0c86e0b4882d 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.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/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index b22da97d4f77..a36bcef4fd3c 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -106,6 +106,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] 20+ messages in thread