[PATCH V1 0/3] Fix incorrect implementation in CcMeasurement

[PATCH V1 0/3] Fix incorrect implementation in CcMeasurement

[Min Xu]

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

This patch-set fix the incorrect implementation in CcMeasurement.

Patch#1:
  Incorrect protocol and structure version

Patch#2/3:
  Incorrect mapping between TPM PCR index and TDX Measurement Register
  index.

Code: https://github.com/mxu9/edk2/tree/CcMapping.v1

Cc: Erdem Aktas <erdemaktas@google.com> [ruleof2]
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: Arti Gupta <ARGU@microsoft.com>
Reported-by: Arti Gupta <ARGU@microsoft.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>

Min M Xu (3):
  OvmfPkg/TdTcg2Dxe: Fix incorrect protocol and structure version
  OvmfPkg/TdTcg2Dxe: Fix the mapping error between PCR index and MR
    index
  OvmfPkg/SecTpmMeasurementLib: Fix the mapping error of PCR and RTMR
    index

 OvmfPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c        | 93 ++++++++++++-------
 .../SecTpmMeasurementLibTdx.c                 |  7 +-
 2 files changed, 65 insertions(+), 35 deletions(-)

-- 
2.29.2.windows.2

[PATCH V1 1/3] OvmfPkg/TdTcg2Dxe: Fix incorrect protocol and structure version

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

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

According to the Uefi spec 2.10 Section 38.2.2.
EFI_CC_MEASUREMENT_PROTOCOL.GetCapability, the minor version of
StructureVersion and ProtocolVersion should be 0.

Cc: Erdem Aktas <erdemaktas@google.com> [ruleof2]
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: Arti Gupta <ARGU@microsoft.com>
Reported-by: Arti Gupta <ARGU@microsoft.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c b/OvmfPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c
index e9315ecda17b..a6b4f8e0aa6b 100644
--- a/OvmfPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c
+++ b/OvmfPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c
@@ -2411,9 +2411,9 @@ DriverEntry (
 
   mTdxDxeData.BsCap.Size                   = sizeof (EFI_CC_BOOT_SERVICE_CAPABILITY);
   mTdxDxeData.BsCap.ProtocolVersion.Major  = 1;
-  mTdxDxeData.BsCap.ProtocolVersion.Minor  = 1;
+  mTdxDxeData.BsCap.ProtocolVersion.Minor  = 0;
   mTdxDxeData.BsCap.StructureVersion.Major = 1;
-  mTdxDxeData.BsCap.StructureVersion.Minor = 1;
+  mTdxDxeData.BsCap.StructureVersion.Minor = 0;
 
   //
   // Get supported PCR and current Active PCRs
-- 
2.29.2.windows.2

[PATCH V1 2/3] OvmfPkg/TdTcg2Dxe: Fix the mapping error between PCR index and MR index

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

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

According to UEFI Spec 2.10 it is supposed to return the mapping from PCR
index to CC MR index:
//
// In the current version, we use the below mapping for TDX:
//
// TPM PCR Index | CC Measurement Register Index | TDX-measurement register
// -----------------------------------------------------------------------
// 0             |   0                           |   MRTD
// 1, 7          |   1                           |   RTMR[0]
// 2~6           |   2                           |   RTMR[1]
// 8~15          |   3                           |   RTMR[2]

In the current implementation TdMapPcrToMrIndex returns the index of RTMR,
not the MR index.

After fix the spec unconsistent, other related codes are updated
accordingly.
1) The index of event log uses the input MrIndex.
2) MrIndex is decreated by 1 before it is sent for RTMR extending.

Cc: Erdem Aktas <erdemaktas@google.com> [ruleof2]
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: Arti Gupta <ARGU@microsoft.com>
Reported-by: Arti Gupta <ARGU@microsoft.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c | 89 +++++++++++++++++---------
 1 file changed, 60 insertions(+), 29 deletions(-)

diff --git a/OvmfPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c b/OvmfPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c
index a6b4f8e0aa6b..d19923b0c682 100644
--- a/OvmfPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c
+++ b/OvmfPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c
@@ -49,7 +49,11 @@
 #define PERF_ID_CC_TCG2_DXE  0x3130
 
 #define   CC_EVENT_LOG_AREA_COUNT_MAX  1
-#define   INVALID_RTMR_INDEX           4
+#define   CC_MR_INDEX_0_MRTD           0
+#define   CC_MR_INDEX_1_RTMR0          1
+#define   CC_MR_INDEX_2_RTMR1          2
+#define   CC_MR_INDEX_3_RTMR2          3
+#define   CC_MR_INDEX_INVALID          4
 
 typedef struct {
   CHAR16      *VariableName;
@@ -240,7 +244,7 @@ EFI_HANDLE  mImageHandle;
 
   Notes: PE/COFF image is checked by BasePeCoffLib PeCoffLoaderGetImageInfo().
 
-  @param[in]  MrIndex      RTMR index
+  @param[in]  RtmrIndex        RTMR index
   @param[in]  ImageAddress   Start address of image buffer.
   @param[in]  ImageSize      Image size
   @param[out] DigestList     Digest list of this image.
@@ -251,7 +255,7 @@ EFI_HANDLE  mImageHandle;
 **/
 EFI_STATUS
 MeasurePeImageAndExtend (
-  IN  UINT32                MrIndex,
+  IN  UINT32                RtmrIndex,
   IN  EFI_PHYSICAL_ADDRESS  ImageAddress,
   IN  UINTN                 ImageSize,
   OUT TPML_DIGEST_VALUES    *DigestList
@@ -925,10 +929,22 @@ TcgCommLogEvent (
 }
 
 /**
-    RTMR[0]  => PCR[1,7]
-    RTMR[1]  => PCR[2,3,4,5]
-    RTMR[2]  => PCR[8~15]
-    RTMR[3]  => NA
+  According to UEFI Spec 2.10 Section 38.4.1:
+    The following table shows the TPM PCR index mapping and CC event log measurement
+  register index interpretation for Intel TDX, where MRTD means Trust Domain Measurement
+   Register and RTMR means Runtime Measurement Register
+
+    // TPM PCR Index | CC Measurement Register Index | TDX-measurement register
+    //  ------------------------------------------------------------------------
+    // 0             |   0                           |   MRTD
+    // 1, 7          |   1                           |   RTMR[0]
+    // 2~6           |   2                           |   RTMR[1]
+    // 8~15          |   3                           |   RTMR[2]
+
+  @param[in] PCRIndex Index of the TPM PCR
+
+  @retval    UINT32               Index of the CC Event Log Measurement Register Index
+  @retval    CC_MR_INDEX_INVALID  Invalid MR Index
 **/
 UINT32
 EFIAPI
@@ -938,18 +954,20 @@ MapPcrToMrIndex (
 {
   UINT32  MrIndex;
 
-  if ((PCRIndex > 16) || (PCRIndex == 6) || (PCRIndex == 0)) {
+  if (PCRIndex > 15) {
     ASSERT (FALSE);
-    return INVALID_RTMR_INDEX;
+    return CC_MR_INDEX_INVALID;
   }
 
   MrIndex = 0;
-  if ((PCRIndex == 1) || (PCRIndex == 7)) {
-    MrIndex = 0;
-  } else if ((PCRIndex > 1) && (PCRIndex < 6)) {
-    MrIndex = 1;
-  } else if ((PCRIndex > 7) && (PCRIndex < 16)) {
-    MrIndex = 2;
+  if (PCRIndex == 0) {
+    MrIndex = CC_MR_INDEX_0_MRTD;
+  } else if ((PCRIndex == 1) || (PCRIndex == 7)) {
+    MrIndex = CC_MR_INDEX_1_RTMR0;
+  } else if ((PCRIndex >= 2) && (PCRIndex <= 6)) {
+    MrIndex = CC_MR_INDEX_2_RTMR1;
+  } else if ((PCRIndex >= 8) && (PCRIndex <= 15)) {
+    MrIndex = CC_MR_INDEX_3_RTMR2;
   }
 
   return MrIndex;
@@ -967,13 +985,9 @@ TdMapPcrToMrIndex (
     return EFI_INVALID_PARAMETER;
   }
 
-  if ((PCRIndex > 16) || (PCRIndex == 0) || (PCRIndex == 6)) {
-    return EFI_INVALID_PARAMETER;
-  }
-
   *MrIndex = MapPcrToMrIndex (PCRIndex);
 
-  return *MrIndex == INVALID_RTMR_INDEX ? EFI_INVALID_PARAMETER : EFI_SUCCESS;
+  return *MrIndex == CC_MR_INDEX_INVALID ? EFI_INVALID_PARAMETER : EFI_SUCCESS;
 }
 
 /**
@@ -1197,12 +1211,7 @@ TdxDxeLogHashEvent (
   LogFormat = EFI_CC_EVENT_LOG_FORMAT_TCG_2;
 
   ZeroMem (&CcEvent, sizeof (CcEvent));
-  //
-  // The index of event log is designed as below:
-  //   0  : MRTD
-  //   1-4: RTMR[0-3]
-  //
-  CcEvent.MrIndex   = NewEventHdr->MrIndex + 1;
+  CcEvent.MrIndex   = NewEventHdr->MrIndex;
   CcEvent.EventType = NewEventHdr->EventType;
   DigestBuffer      = (UINT8 *)&CcEvent.Digests;
   EventSizePtr      = CopyDigestListToBuffer (DigestBuffer, DigestList, HASH_ALG_SHA384);
@@ -1270,8 +1279,16 @@ TdxDxeHashLogExtendEvent (
     return Status;
   }
 
+  //
+  // According to UEFI Spec 2.10 Section 38.4.1 the mapping between MrIndex and Intel
+  // TDX Measurement Register is:
+  //    MrIndex 0   <--> MRTD
+  //    MrIndex 1-3 <--> RTMR[0-2]
+  // Only the RMTR registers can be extended in TDVF by HashAndExtend. So MrIndex will
+  // decreased by 1 before it is sent to HashAndExtend.
+  //
   Status = HashAndExtend (
-             NewEventHdr->MrIndex,
+             NewEventHdr->MrIndex - 1,
              HashData,
              (UINTN)HashDataLen,
              &DigestList
@@ -1335,7 +1352,13 @@ TdHashLogExtendEvent (
     return EFI_INVALID_PARAMETER;
   }
 
-  if (CcEvent->Header.MrIndex > 4) {
+  if (CcEvent->Header.MrIndex == CC_MR_INDEX_0_MRTD) {
+    DEBUG ((DEBUG_ERROR, "%a: MRTD cannot be extended in TDVF.\n", __FUNCTION__));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (CcEvent->Header.MrIndex >= CC_MR_INDEX_INVALID) {
+    DEBUG ((DEBUG_ERROR, "%a: MrIndex is invalid. (%d)\n", __FUNCTION__, CcEvent->Header.MrIndex));
     return EFI_INVALID_PARAMETER;
   }
 
@@ -1343,8 +1366,16 @@ TdHashLogExtendEvent (
   NewEventHdr.EventType = CcEvent->Header.EventType;
   NewEventHdr.EventSize = CcEvent->Size - sizeof (UINT32) - CcEvent->Header.HeaderSize;
   if ((Flags & EFI_CC_FLAG_PE_COFF_IMAGE) != 0) {
+    //
+    // According to UEFI Spec 2.10 Section 38.4.1 the mapping between MrIndex and Intel
+    // TDX Measurement Register is:
+    //    MrIndex 0   <--> MRTD
+    //    MrIndex 1-3 <--> RTMR[0-2]
+    // Only the RMTR registers can be extended in TDVF by HashAndExtend. So MrIndex will
+    // decreased by 1 before it is sent to MeasurePeImageAndExtend.
+    //
     Status = MeasurePeImageAndExtend (
-               NewEventHdr.MrIndex,
+               NewEventHdr.MrIndex - 1,
                DataToHash,
                (UINTN)DataToHashLen,
                &DigestList
-- 
2.29.2.windows.2

[PATCH V1 3/3] OvmfPkg/SecTpmMeasurementLib: Fix the mapping error of PCR and RTMR index

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

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

TDVF has the feature to do RTMR measurement in SEC phase. In the same time
it builds a GUID hob which carries the hash value of the measurement so
that in DXE phase a td event can be created based on this GUID Hob. There
is a mapping error between TPM PCR index and RTMR index according to UEFI
2.10. That PCR6 is missing in the mapping. This patch fixes this issue.

Cc: Erdem Aktas <erdemaktas@google.com> [ruleof2]
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: Arti Gupta <ARGU@microsoft.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 .../Library/SecTpmMeasurementLib/SecTpmMeasurementLibTdx.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/SecurityPkg/Library/SecTpmMeasurementLib/SecTpmMeasurementLibTdx.c b/SecurityPkg/Library/SecTpmMeasurementLib/SecTpmMeasurementLibTdx.c
index 38887b172dc0..36bfa373fe0f 100644
--- a/SecurityPkg/Library/SecTpmMeasurementLib/SecTpmMeasurementLibTdx.c
+++ b/SecurityPkg/Library/SecTpmMeasurementLib/SecTpmMeasurementLibTdx.c
@@ -33,12 +33,11 @@ typedef struct {
 /**
   Get the mapped RTMR index based on the input PCRIndex.
   RTMR[0]  => PCR[1,7]
-  RTMR[1]  => PCR[2,3,4,5]
+  RTMR[1]  => PCR[2,3,4,5,6]
   RTMR[2]  => PCR[8~15]
   RTMR[3]  => NA
   Note:
     PCR[0] is mapped to MRTD and should not appear here.
-    PCR[6] is reserved for OEM. It is not used.
 
    @param[in] PCRIndex The input PCR index
 
@@ -51,7 +50,7 @@ GetMappedRtmrIndex (
 {
   UINT8  RtmrIndex;
 
-  if ((PCRIndex == 6) || (PCRIndex == 0) || (PCRIndex > 15)) {
+  if ((PCRIndex == 0) || (PCRIndex > 15)) {
     DEBUG ((DEBUG_ERROR, "Invalid PCRIndex(%d) map to MR Index.\n", PCRIndex));
     ASSERT (FALSE);
     return INVALID_PCR2MR_INDEX;
@@ -60,7 +59,7 @@ GetMappedRtmrIndex (
   RtmrIndex = 0;
   if ((PCRIndex == 1) || (PCRIndex == 7)) {
     RtmrIndex = 0;
-  } else if ((PCRIndex >= 2) && (PCRIndex < 6)) {
+  } else if ((PCRIndex >= 2) && (PCRIndex <= 6)) {
     RtmrIndex = 1;
   } else if ((PCRIndex >= 8) && (PCRIndex <= 15)) {
     RtmrIndex = 2;
-- 
2.29.2.windows.2

Re: [PATCH V1 0/3] Fix incorrect implementation in CcMeasurement

[Yao, Jiewen]

All Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Wednesday, December 14, 2022 3:14 PM
> To: devel@edk2.groups.io
> Cc: Xu, Min M <min.m.xu@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Arti Gupta <ARGU@microsoft.com>
> Subject: [PATCH V1 0/3] Fix incorrect implementation in CcMeasurement
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4179
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4184
> 
> This patch-set fix the incorrect implementation in CcMeasurement.
> 
> Patch#1:
>   Incorrect protocol and structure version
> 
> Patch#2/3:
>   Incorrect mapping between TPM PCR index and TDX Measurement Register
>   index.
> 
> Code: https://github.com/mxu9/edk2/tree/CcMapping.v1
> 
> Cc: Erdem Aktas <erdemaktas@google.com> [ruleof2]
> 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: Arti Gupta <ARGU@microsoft.com>
> Reported-by: Arti Gupta <ARGU@microsoft.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> 
> Min M Xu (3):
>   OvmfPkg/TdTcg2Dxe: Fix incorrect protocol and structure version
>   OvmfPkg/TdTcg2Dxe: Fix the mapping error between PCR index and MR
>     index
>   OvmfPkg/SecTpmMeasurementLib: Fix the mapping error of PCR and RTMR
>     index
> 
>  OvmfPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c        | 93 ++++++++++++-------
>  .../SecTpmMeasurementLibTdx.c                 |  7 +-
>  2 files changed, 65 insertions(+), 35 deletions(-)
> 
> --
> 2.29.2.windows.2

Re: [edk2-devel] [PATCH V1 0/3] Fix incorrect implementation in CcMeasurement

[Yao, Jiewen]

Merged https://github.com/tianocore/edk2/pull/3773

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
> Jiewen
> Sent: Thursday, December 15, 2022 12:25 AM
> To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
> Cc: Aktas, Erdem <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Tom Lendacky <thomas.lendacky@amd.com>; Arti
> Gupta <ARGU@microsoft.com>
> Subject: Re: [edk2-devel] [PATCH V1 0/3] Fix incorrect implementation in
> CcMeasurement
> 
> All Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> > -----Original Message-----
> > From: Xu, Min M <min.m.xu@intel.com>
> > Sent: Wednesday, December 14, 2022 3:14 PM
> > To: devel@edk2.groups.io
> > Cc: Xu, Min M <min.m.xu@intel.com>; Aktas, Erdem
> > <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Yao,
> > Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> > <thomas.lendacky@amd.com>; Arti Gupta <ARGU@microsoft.com>
> > Subject: [PATCH V1 0/3] Fix incorrect implementation in CcMeasurement
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4179
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4184
> >
> > This patch-set fix the incorrect implementation in CcMeasurement.
> >
> > Patch#1:
> >   Incorrect protocol and structure version
> >
> > Patch#2/3:
> >   Incorrect mapping between TPM PCR index and TDX Measurement
> Register
> >   index.
> >
> > Code: https://github.com/mxu9/edk2/tree/CcMapping.v1
> >
> > Cc: Erdem Aktas <erdemaktas@google.com> [ruleof2]
> > 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: Arti Gupta <ARGU@microsoft.com>
> > Reported-by: Arti Gupta <ARGU@microsoft.com>
> > Signed-off-by: Min Xu <min.m.xu@intel.com>
> >
> > Min M Xu (3):
> >   OvmfPkg/TdTcg2Dxe: Fix incorrect protocol and structure version
> >   OvmfPkg/TdTcg2Dxe: Fix the mapping error between PCR index and MR
> >     index
> >   OvmfPkg/SecTpmMeasurementLib: Fix the mapping error of PCR and
> RTMR
> >     index
> >
> >  OvmfPkg/IntelTdx/TdTcg2Dxe/TdTcg2Dxe.c        | 93 ++++++++++++-------
> >  .../SecTpmMeasurementLibTdx.c                 |  7 +-
> >  2 files changed, 65 insertions(+), 35 deletions(-)
> >
> > --
> > 2.29.2.windows.2
> 
> 
> 
> 
>