RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3853

TdHobList and Configuration FV are external data provided by Host VMM.
These are not trusted in Td guest. So they should be validated , measured
and extended to Td RTMR registers. In the meantime 2 EFI_CC_EVENT_HOB are
created. These 2 GUIDed HOBs carry the hash value of TdHobList and
Configuration FV. In DXE phase EFI_CC_EVENT can be created based on these
2 GUIDed HOBs.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/IntelTdx/IntelTdxX64.dsc              |   4 +
 OvmfPkg/Library/PeilessStartupLib/IntelTdx.c  | 163 ++++++++++++++++++
 .../PeilessStartupLib/PeilessStartup.c        |  31 ++++
 .../PeilessStartupInternal.h                  |  17 ++
 .../PeilessStartupLib/PeilessStartupLib.inf   |   8 +-
 5 files changed, 221 insertions(+), 2 deletions(-)
 create mode 100644 OvmfPkg/Library/PeilessStartupLib/IntelTdx.c

diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
index 245155d41b30..e6cd10a120a8 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
@@ -520,6 +520,10 @@
   OvmfPkg/IntelTdx/Sec/SecMain.inf {
     <LibraryClasses>
       NULL|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
+      SecMeasurementLib|OvmfPkg/Library/SecMeasurementLib/SecMeasurementLibTdx.inf
+      BaseCryptLib|CryptoPkg/Library/BaseCryptLib/SecCryptLib.inf
+      HashLib|SecurityPkg/Library/HashLibTdx/HashLibTdx.inf
+      NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
   }
 
   #
diff --git a/OvmfPkg/Library/PeilessStartupLib/IntelTdx.c b/OvmfPkg/Library/PeilessStartupLib/IntelTdx.c
new file mode 100644
index 000000000000..d240d3b7719f
--- /dev/null
+++ b/OvmfPkg/Library/PeilessStartupLib/IntelTdx.c
@@ -0,0 +1,163 @@
+/** @file
+  Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <PiPei.h>
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Guid/VariableFormat.h>
+#include <Guid/SystemNvDataGuid.h>
+#include "PeilessStartupInternal.h"
+
+/**
+  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;
+}
diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
index 126eb74048f4..54236b956c52 100644
--- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
+++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
@@ -20,6 +20,7 @@
 #include <ConfidentialComputingGuestAttr.h>
 #include <Guid/MemoryTypeInformation.h>
 #include <OvmfPlatforms.h>
+#include <Library/SecMeasurementLib.h>
 #include "PeilessStartupInternal.h"
 
 #define GET_GPAW_INIT_STATE(INFO)  ((UINT8) ((INFO) & 0x3f))
@@ -133,11 +134,13 @@ PeilessStartup (
   UINT32                      DxeCodeSize;
   TD_RETURN_DATA              TdReturnData;
   VOID                        *VmmHobList;
+  UINT8                       *CfvBase;
 
   Status      = EFI_SUCCESS;
   BootFv      = NULL;
   VmmHobList  = NULL;
   SecCoreData = (EFI_SEC_PEI_HAND_OFF *)Context;
+  CfvBase     = (UINT8 *)(UINTN)FixedPcdGet32 (PcdCfvBase);
 
   ZeroMem (&PlatformInfoHob, sizeof (PlatformInfoHob));
 
@@ -167,6 +170,34 @@ PeilessStartup (
 
   DEBUG ((DEBUG_INFO, "HobList: %p\n", GetHobList ()));
 
+  if (TdIsEnabled ()) {
+    //
+    // Measure HobList
+    //
+    Status = MeasureHobList (VmmHobList);
+    if (EFI_ERROR (Status)) {
+      ASSERT (FALSE);
+      CpuDeadLoop ();
+    }
+
+    //
+    // Validate Tdx CFV
+    //
+    if (!TdxValidateCfv (CfvBase, FixedPcdGet32 (PcdCfvRawDataSize))) {
+      ASSERT (FALSE);
+      CpuDeadLoop ();
+    }
+
+    //
+    // Measure Tdx CFV
+    //
+    Status = MeasureFvImage ((EFI_PHYSICAL_ADDRESS)(UINTN)CfvBase, FixedPcdGet32 (PcdCfvRawDataSize), 1);
+    if (EFI_ERROR (Status)) {
+      ASSERT (FALSE);
+      CpuDeadLoop ();
+    }
+  }
+
   //
   // Initialize the Platform
   //
diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupInternal.h b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupInternal.h
index 23e9e0be53f1..dd79b8a06b44 100644
--- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupInternal.h
+++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupInternal.h
@@ -52,4 +52,21 @@ 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
+  );
+
 #endif
diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
index 8791984586a4..c5d291f02bcd 100644
--- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
+++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
@@ -29,8 +29,7 @@
   PeilessStartup.c
   Hob.c
   DxeLoad.c
-
-[Sources.X64]
+  IntelTdx.c
   X64/VirtualMemory.c
 
 [Packages]
@@ -39,6 +38,8 @@
   UefiCpuPkg/UefiCpuPkg.dec
   OvmfPkg/OvmfPkg.dec
   EmbeddedPkg/EmbeddedPkg.dec
+  CryptoPkg/CryptoPkg.dec
+  SecurityPkg/SecurityPkg.dec
 
 [LibraryClasses]
   BaseLib
@@ -56,6 +57,8 @@
   PrePiLib
   QemuFwCfgLib
   PlatformInitLib
+  HashLib
+  SecMeasurementLib
 
 [Guids]
   gEfiHobMemoryAllocModuleGuid
@@ -63,6 +66,7 @@
   gUefiOvmfPkgPlatformInfoGuid
   gEfiMemoryTypeInformationGuid
   gPcdDataBaseHobGuid
+  gCcEventEntryHobGuid
 
 [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdCfvBase
-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88988): https://edk2.groups.io/g/devel/message/88988
Mute This Topic: https://groups.io/mt/90531017/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Mon, Apr 18, 2022 at 07:59:56AM +0800, Min Xu wrote:
> RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3853
> 
> TdHobList and Configuration FV are external data provided by Host VMM.
> These are not trusted in Td guest. So they should be validated , measured
> and extended to Td RTMR registers. In the meantime 2 EFI_CC_EVENT_HOB are
> created. These 2 GUIDed HOBs carry the hash value of TdHobList and
> Configuration FV. In DXE phase EFI_CC_EVENT can be created based on these
> 2 GUIDed HOBs.

Why this is done in the SEC phase?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89057): https://edk2.groups.io/g/devel/message/89057
Mute This Topic: https://groups.io/mt/90531017/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On April 19, 2022 2:59 PM, Gerd Hoffmann wrote:
> On Mon, Apr 18, 2022 at 07:59:56AM +0800, Min Xu wrote:
> > RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3853
> >
> > TdHobList and Configuration FV are external data provided by Host VMM.
> > These are not trusted in Td guest. So they should be validated ,
> > measured and extended to Td RTMR registers. In the meantime 2
> > EFI_CC_EVENT_HOB are created. These 2 GUIDed HOBs carry the hash
> value
> > of TdHobList and Configuration FV. In DXE phase EFI_CC_EVENT can be
> > created based on these
> > 2 GUIDed HOBs.
> 
> Why this is done in the SEC phase?
TdHobList is consumed in SEC phase. So before it is consumed, it should be validated, measured.

CFV contains the information provisioned by host VMM, for example, the secure boot parameters. These external data should be validated and measured as well.
RTMR based measurement is implemented in TDVF Config-B (https://edk2.groups.io/g/devel/message/76367). Config-B skip the PEI phase.
So it just looks like the Tcg2Pei which measures FVs before handing off control to DXE.

Thanks
Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89069): https://edk2.groups.io/g/devel/message/89069
Mute This Topic: https://groups.io/mt/90531017/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, Apr 19, 2022 at 11:12:39AM +0000, Min Xu wrote:
> On April 19, 2022 2:59 PM, Gerd Hoffmann wrote:
> > On Mon, Apr 18, 2022 at 07:59:56AM +0800, Min Xu wrote:
> > > RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3853
> > >
> > > TdHobList and Configuration FV are external data provided by Host VMM.
> > > These are not trusted in Td guest. So they should be validated ,
> > > measured and extended to Td RTMR registers. In the meantime 2
> > > EFI_CC_EVENT_HOB are created. These 2 GUIDed HOBs carry the hash
> > value
> > > of TdHobList and Configuration FV. In DXE phase EFI_CC_EVENT can be
> > > created based on these
> > > 2 GUIDed HOBs.
> > 
> > Why this is done in the SEC phase?
> TdHobList is consumed in SEC phase. So before it is consumed, it should be validated, measured.

Yes for validation (aka sanity-checking the fields, etc).
But for measurement I don't see why the ordering matters.
Whenever you do that before or after consuming the TdHob
should not make a difference.

> CFV contains the information provisioned by host VMM, for example, the
> secure boot parameters. These external data should be validated and
> measured as well.

Same argument here.

You pull a bunch of stuff into SEC (sha384, ...), and I'm wondering
whenever it would be better to move measurement to DXE instead where
you just don't need that kind of changes.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89072): https://edk2.groups.io/g/devel/message/89072
Mute This Topic: https://groups.io/mt/90531017/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Inlined

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Tuesday, April 19, 2022 8:49 PM
> To: devel@edk2.groups.io; Xu, Min M <min.m.xu@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Yao, Jiewen
> <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Brijesh
> Singh <brijesh.singh@amd.com>; Aktas, Erdem <erdemaktas@google.com>;
> James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V3 5/9] OvmfPkg/IntelTdx: Measure Td
> HobList and Configuration FV
> 
> On Tue, Apr 19, 2022 at 11:12:39AM +0000, Min Xu wrote:
> > On April 19, 2022 2:59 PM, Gerd Hoffmann wrote:
> > > On Mon, Apr 18, 2022 at 07:59:56AM +0800, Min Xu wrote:
> > > > RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3853
> > > >
> > > > TdHobList and Configuration FV are external data provided by Host VMM.
> > > > These are not trusted in Td guest. So they should be validated ,
> > > > measured and extended to Td RTMR registers. In the meantime 2
> > > > EFI_CC_EVENT_HOB are created. These 2 GUIDed HOBs carry the hash
> > > value
> > > > of TdHobList and Configuration FV. In DXE phase EFI_CC_EVENT can be
> > > > created based on these
> > > > 2 GUIDed HOBs.
> > >
> > > Why this is done in the SEC phase?
> > TdHobList is consumed in SEC phase. So before it is consumed, it should be
> validated, measured.
> 
> Yes for validation (aka sanity-checking the fields, etc).
> But for measurement I don't see why the ordering matters.
> Whenever you do that before or after consuming the TdHob
> should not make a difference.

[Jiewen] I disagree. The order matters from security perspective.
If you use it, there is risk that the buggy code will compromise the system before you have chance to measure it.
There was already known attacks: The measurement was in wrong place, which caused the attack can forge the measurement.

The best practice is always: measure then use.






> 
> > CFV contains the information provisioned by host VMM, for example, the
> > secure boot parameters. These external data should be validated and
> > measured as well.
> 
> Same argument here.
> 
> You pull a bunch of stuff into SEC (sha384, ...), and I'm wondering
> whenever it would be better to move measurement to DXE instead where
> you just don't need that kind of changes.
> 
> take care,
>   Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89077): https://edk2.groups.io/g/devel/message/89077
Mute This Topic: https://groups.io/mt/90531017/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> > Yes for validation (aka sanity-checking the fields, etc).
> > But for measurement I don't see why the ordering matters.
> > Whenever you do that before or after consuming the TdHob
> > should not make a difference.
> 
> [Jiewen] I disagree. The order matters from security perspective.
> If you use it, there is risk that the buggy code will compromise the system before you have chance to measure it.

Measurement will only record hashes for verification later on.
It will not prevent running possibly buggy/compromised code.

So, no matter what the order is, you'll figure the system got
compromised after the fact, when checking the hashes later, and in turn
take actions like refusing to hand out secrets to the compromised
system.

> There was already known attacks: The measurement was in wrong place,
> which caused the attack can forge the measurement.

Do you have a link or CVE number for me?

thanks,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89125): https://edk2.groups.io/g/devel/message/89125
Mute This Topic: https://groups.io/mt/90531017/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, 2022-04-20 at 10:16 +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > Yes for validation (aka sanity-checking the fields, etc).
> > > But for measurement I don't see why the ordering matters.
> > > Whenever you do that before or after consuming the TdHob
> > > should not make a difference.
> > 
> > [Jiewen] I disagree. The order matters from security perspective.
> > If you use it, there is risk that the buggy code will compromise
> > the system before you have chance to measure it.
> 
> Measurement will only record hashes for verification later on.
> It will not prevent running possibly buggy/compromised code.

This is true, but this is also the design of measured boot: it's for
proof of correctness (or not) after the fact.  Secure boot is more the
technology that can prevent boot.

> So, no matter what the order is, you'll figure the system got
> compromised after the fact, when checking the hashes later, and in
> turn take actions like refusing to hand out secrets to the
> compromised system.

Not if the code falsifies the measurement both in the log and to the
TPM.  That's why the requirement of measured boot is you start with a
small rom based root of trust, which can't be updated because it's in
rom.  It measures the next stage (usually PEI) before executing it so
that the measurement in the TPM would change if the next stage (which
is often in flash) got compromised, so any tampering is certain to be
detected and if the compromised code tries to falsify the log, the log
now wouldn't match the TPM, so it can't evade detection.

The requirement from the TCG is that the trusted code measures the
untrusted code through the TPM before executing it to get this
proveable detection of tampering.  The TCG allows you to be elastic
about when you record the measurements in the log as long as you
measure through the TPM at the correct points.

The above applies equally to TPM substitutes like the TDX msrs.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89139): https://edk2.groups.io/g/devel/message/89139
Mute This Topic: https://groups.io/mt/90531017/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,
 
> > So, no matter what the order is, you'll figure the system got
> > compromised after the fact, when checking the hashes later, and in
> > turn take actions like refusing to hand out secrets to the
> > compromised system.
> 
> Not if the code falsifies the measurement both in the log and to the
> TPM.  That's why the requirement of measured boot is you start with a
> small rom based root of trust, which can't be updated because it's in
> rom.  It measures the next stage (usually PEI) before executing it so
> that the measurement in the TPM would change if the next stage (which
> is often in flash) got compromised, so any tampering is certain to be
> detected and if the compromised code tries to falsify the log, the log
> now wouldn't match the TPM, so it can't evade detection.

How do we establish the root of trust in case of TDX?  We don't have a
real rom in virtual machines ...

Does the tdx firmware measure the firmware code before running it?

Why handle CFV and BFV differently?  Wouldn't it be easier to have the
tdx firmware simply measure the complete OVMF.fd image, given that tdx
doesn't support flash and thus we don't have the code/vars split in the
first place?

The TD HobList is prepared by the hypervisor and present at launch time,
so possibly the tdx firmware could measure it too before handing over
control to the guest?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89144): https://edk2.groups.io/g/devel/message/89144
Mute This Topic: https://groups.io/mt/90531017/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

The Root-of-Trust for Measurement (RTM) for TDX is TDX-Module. The TDX-Module will enforce the MRTD calculation for the TDVF code.
Then TDVF can then act as Chain-of-Trust for Measurement (CTM) to setup RTMR and continue the rest.

It is described in [TDX-Module] Chapter 11, [TDVF] Chapter 8.

[TDX-Module] https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1.0-public-spec-v0.931.pdf
[TDVF] https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-virtual-firmware-design-guide-rev-1.01.pdf


> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Thursday, April 21, 2022 12:29 AM
> To: James Bottomley <jejb@linux.ibm.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Xu, Min M
> <min.m.xu@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Aktas, Erdem <erdemaktas@google.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V3 5/9] OvmfPkg/IntelTdx: Measure Td
> HobList and Configuration FV
> 
>   Hi,
> 
> > > So, no matter what the order is, you'll figure the system got
> > > compromised after the fact, when checking the hashes later, and in
> > > turn take actions like refusing to hand out secrets to the
> > > compromised system.
> >
> > Not if the code falsifies the measurement both in the log and to the
> > TPM.  That's why the requirement of measured boot is you start with a
> > small rom based root of trust, which can't be updated because it's in
> > rom.  It measures the next stage (usually PEI) before executing it so
> > that the measurement in the TPM would change if the next stage (which
> > is often in flash) got compromised, so any tampering is certain to be
> > detected and if the compromised code tries to falsify the log, the log
> > now wouldn't match the TPM, so it can't evade detection.
> 
> How do we establish the root of trust in case of TDX?  We don't have a
> real rom in virtual machines ...
> 
> Does the tdx firmware measure the firmware code before running it?
> 
> Why handle CFV and BFV differently?  Wouldn't it be easier to have the
> tdx firmware simply measure the complete OVMF.fd image, given that tdx
> doesn't support flash and thus we don't have the code/vars split in the
> first place?
> 
> The TD HobList is prepared by the hypervisor and present at launch time,
> so possibly the tdx firmware could measure it too before handing over
> control to the guest?
> 
> take care,
>   Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89159): https://edk2.groups.io/g/devel/message/89159
Mute This Topic: https://groups.io/mt/90531017/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, Apr 20, 2022 at 10:29:11PM +0000, Yao, Jiewen wrote:
> The Root-of-Trust for Measurement (RTM) for TDX is TDX-Module. The TDX-Module will enforce the MRTD calculation for the TDVF code.
> Then TDVF can then act as Chain-of-Trust for Measurement (CTM) to setup RTMR and continue the rest.
> 
> It is described in [TDX-Module] Chapter 11, [TDVF] Chapter 8.
> 
> [TDX-Module] https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1.0-public-spec-v0.931.pdf
> [TDVF] https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-virtual-firmware-design-guide-rev-1.01.pdf

Ok.  So it all works via TDH.MEM.PAGE.ADD (initial set of accepted
pages) and TDH.MR.EXTEND (measure into MRTD) functions.

Looking at our binary ...

# virt-fw-dump -i Build/IntelTdx/DEBUG_GCC5/FV/OVMF.fd --ovmf-meta
image=Build/IntelTdx/DEBUG_GCC5/FV/OVMF.fd
  resetvector size=0x9b0
    [ ... sev metadata snipped ... ]
    guid:TdxMetadataOffset size=0x16 data=50080000
      mbase=0xffc84000 msize=0x37c000 type=BFV (code) fbase=0x84000 fsize=0x37c000 flags=0x1
      mbase=0xffc00000 msize=0x84000 type=CFV (vars) fbase=0x0 fsize=0x84000
      mbase=0x810000 msize=0x10000 type=MEM
      mbase=0x80b000 msize=0x2000 type=MEM
      mbase=0x809000 msize=0x2000 type=TD Hob
      mbase=0x800000 msize=0x6000 type=MEM

... BFV is measured (bit 0 of flags) whereas CFV and TD Hob are only
added but not measured.

Adding CFV and TH Hob to the initial launch measurement should be
possible by just updating flags, correct?

I think this should be done for the CFV.  The firmware will be loaded
via "qemu -bios OVMF.fd".  No separate images for CODE and VARS. So
splitting the measurement looks rather pointless to me.

TD Hob could be part of the initial launch measurement too, which would
avoid the need to measure anything in SEC.  On the other hand the that
would make the launch measurement depend not only on the firmware image
but also the guest configuration (memory size), which would likely make
things more complexity elsewhere, so probably not a good idea.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89179): https://edk2.groups.io/g/devel/message/89179
Mute This Topic: https://groups.io/mt/90531017/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Adding CFV and TD_HOB to MRTD is technically possible, but not desired.

In a typical trust boot use case, the verifier should have a way to distinguish the *code* from *configuration*.
If you look at the TCG specification, the TPM has 24 PCRs. 8 of them are allocated for BIOS. Each PCRs record one type of measurements.
Technically, you can merge all PCR into one. But no one will do that in reality.

I would say: merging everything into one MRTD is a terrible idea.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Thursday, April 21, 2022 5:15 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: James Bottomley <jejb@linux.ibm.com>; devel@edk2.groups.io; Xu, Min M
> <min.m.xu@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Aktas, Erdem <erdemaktas@google.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V3 5/9] OvmfPkg/IntelTdx: Measure Td
> HobList and Configuration FV
> 
> On Wed, Apr 20, 2022 at 10:29:11PM +0000, Yao, Jiewen wrote:
> > The Root-of-Trust for Measurement (RTM) for TDX is TDX-Module. The TDX-
> Module will enforce the MRTD calculation for the TDVF code.
> > Then TDVF can then act as Chain-of-Trust for Measurement (CTM) to setup
> RTMR and continue the rest.
> >
> > It is described in [TDX-Module] Chapter 11, [TDVF] Chapter 8.
> >
> > [TDX-Module]
> https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-
> module-1.0-public-spec-v0.931.pdf
> > [TDVF]
> https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-
> virtual-firmware-design-guide-rev-1.01.pdf
> 
> Ok.  So it all works via TDH.MEM.PAGE.ADD (initial set of accepted
> pages) and TDH.MR.EXTEND (measure into MRTD) functions.
> 
> Looking at our binary ...
> 
> # virt-fw-dump -i Build/IntelTdx/DEBUG_GCC5/FV/OVMF.fd --ovmf-meta
> image=Build/IntelTdx/DEBUG_GCC5/FV/OVMF.fd
>   resetvector size=0x9b0
>     [ ... sev metadata snipped ... ]
>     guid:TdxMetadataOffset size=0x16 data=50080000
>       mbase=0xffc84000 msize=0x37c000 type=BFV (code) fbase=0x84000
> fsize=0x37c000 flags=0x1
>       mbase=0xffc00000 msize=0x84000 type=CFV (vars) fbase=0x0 fsize=0x84000
>       mbase=0x810000 msize=0x10000 type=MEM
>       mbase=0x80b000 msize=0x2000 type=MEM
>       mbase=0x809000 msize=0x2000 type=TD Hob
>       mbase=0x800000 msize=0x6000 type=MEM
> 
> ... BFV is measured (bit 0 of flags) whereas CFV and TD Hob are only
> added but not measured.
> 
> Adding CFV and TH Hob to the initial launch measurement should be
> possible by just updating flags, correct?
> 
> I think this should be done for the CFV.  The firmware will be loaded
> via "qemu -bios OVMF.fd".  No separate images for CODE and VARS. So
> splitting the measurement looks rather pointless to me.
> 
> TD Hob could be part of the initial launch measurement too, which would
> avoid the need to measure anything in SEC.  On the other hand the that
> would make the launch measurement depend not only on the firmware image
> but also the guest configuration (memory size), which would likely make
> things more complexity elsewhere, so probably not a good idea.
> 
> take care,
>   Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89181): https://edk2.groups.io/g/devel/message/89181
Mute This Topic: https://groups.io/mt/90531017/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Gerd
I cannot agree your statement on ordering.

Smart attacker can forge the good measurement based upon the severity of vulnerability.

One famous example in 2011:
https://invisiblethingslab.com/resources/2011/Attacking_Intel_TXT_via_SINIT_hijacking.pdf
Because the attack happens before PCR18 measurement, the PCR18 is forged successfully.




> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Wednesday, April 20, 2022 4:17 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: devel@edk2.groups.io; Xu, Min M <min.m.xu@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V3 5/9] OvmfPkg/IntelTdx: Measure Td
> HobList and Configuration FV
> 
>   Hi,
> 
> > > Yes for validation (aka sanity-checking the fields, etc).
> > > But for measurement I don't see why the ordering matters.
> > > Whenever you do that before or after consuming the TdHob
> > > should not make a difference.
> >
> > [Jiewen] I disagree. The order matters from security perspective.
> > If you use it, there is risk that the buggy code will compromise the system
> before you have chance to measure it.
> 
> Measurement will only record hashes for verification later on.
> It will not prevent running possibly buggy/compromised code.
> 
> So, no matter what the order is, you'll figure the system got
> compromised after the fact, when checking the hashes later, and in turn
> take actions like refusing to hand out secrets to the compromised
> system.
> 
> > There was already known attacks: The measurement was in wrong place,
> > which caused the attack can forge the measurement.
> 
> Do you have a link or CVE number for me?
> 
> thanks,
>   Gerd
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89134): https://edk2.groups.io/g/devel/message/89134
Mute This Topic: https://groups.io/mt/90531017/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, Apr 20, 2022 at 09:46:13AM +0000, Yao, Jiewen wrote:
> Gerd
> I cannot agree your statement on ordering.
> 
> Smart attacker can forge the good measurement based upon the severity of vulnerability.
> 
> One famous example in 2011:
> https://invisiblethingslab.com/resources/2011/Attacking_Intel_TXT_via_SINIT_hijacking.pdf
> Because the attack happens before PCR18 measurement, the PCR18 is forged successfully.

Ok, understood.  The paper explains it nicely.

thanks,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89142): https://edk2.groups.io/g/devel/message/89142
Mute This Topic: https://groups.io/mt/90531017/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-