[PATCH V1 0/3] Fix TDVF issues

[PATCH V1 0/3] Fix TDVF issues

[Min Xu]

This patch-set fix below TDVF issues:
Patch#1: Initialize Status in IoExit
Patch#2: Extend EFI boot variable to PCR[1] according to TCG PC Client
         PFP spec.
Patch#3: Refactor error handle of SetOrClearSharedBit so that the caller
         can handle the returned error.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
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>
Cc: Michael Roth <michael.roth@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>

Min M Xu (3):
  OvmfPkg/CcExitLib: Initialize Status in IoExit
  SecurityPkg/TdTcg2Dxe: Extend EFI boot variable to PCR[1]
  OvmfPkg/BaseMemEncryptTdxLib: Refactor error handle of
    SetOrClearSharedBit

 .../BaseMemEncryptTdxLib/MemoryEncryption.c   | 48 +++++++++++++++----
 OvmfPkg/Library/CcExitLib/CcExitVeHandler.c   |  9 ++--
 SecurityPkg/Tcg/TdTcg2Dxe/TdTcg2Dxe.c         |  6 +--
 3 files changed, 46 insertions(+), 17 deletions(-)

-- 
2.29.2.windows.2

[PATCH V1 1/3] OvmfPkg/CcExitLib: Initialize Status in IoExit

[Min Xu]

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

Status should be initialized otherwise it may return unexpected value.

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>
Cc: Michael Roth <michael.roth@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/Library/CcExitLib/CcExitVeHandler.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c
index 30d547d5fe55..872f772a5ac8 100644
--- a/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c
+++ b/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c
@@ -105,10 +105,11 @@ IoExit (
   UINT64   RepCnt;
   UINT64   Status;
 
-  Val   = 0;
-  Write = Veinfo->ExitQualification.Io.Direction ? FALSE : TRUE;
-  Size  = Veinfo->ExitQualification.Io.Size + 1;
-  Port  = Veinfo->ExitQualification.Io.Port;
+  Val    = 0;
+  Status = 0;
+  Write  = Veinfo->ExitQualification.Io.Direction ? FALSE : TRUE;
+  Size   = Veinfo->ExitQualification.Io.Size + 1;
+  Port   = Veinfo->ExitQualification.Io.Port;
 
   if (Veinfo->ExitQualification.Io.String) {
     //
-- 
2.29.2.windows.2

[PATCH V1 2/3] SecurityPkg/TdTcg2Dxe: Extend EFI boot variable to PCR[1]

[Min Xu]

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

According to TCG PC Client PFP spec 0021 Section 2.4.4.2 EFI boot variable
should be measured and extended to PCR[1], not PCR[5]. This patch is
proposed to fix this error.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 SecurityPkg/Tcg/TdTcg2Dxe/TdTcg2Dxe.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/SecurityPkg/Tcg/TdTcg2Dxe/TdTcg2Dxe.c b/SecurityPkg/Tcg/TdTcg2Dxe/TdTcg2Dxe.c
index d19923b0c682..59341a8c0250 100644
--- a/SecurityPkg/Tcg/TdTcg2Dxe/TdTcg2Dxe.c
+++ b/SecurityPkg/Tcg/TdTcg2Dxe/TdTcg2Dxe.c
@@ -1873,12 +1873,8 @@ ReadAndMeasureBootVariable (
   OUT     VOID      **VarData
   )
 {
-  //
-  // Boot variables are measured into (PCR[5]) RTMR[1],
-  // details in section 8.1 of TDVF design guide.
-  //
   return ReadAndMeasureVariable (
-           MapPcrToMrIndex (5),
+           MapPcrToMrIndex (1),
            EV_EFI_VARIABLE_BOOT,
            VarName,
            VendorGuid,
-- 
2.29.2.windows.2

[PATCH V1 3/3] OvmfPkg/BaseMemEncryptTdxLib: Refactor error handle of SetOrClearSharedBit

[Min Xu]

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

The previous implementation of SetOrClearSharedBit doesn't handle the
error correctly. In this patch SetOrClearSharedBit is changed to return
error code so that the caller can handle it.

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>
Cc: Michael Roth <michael.roth@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 .../BaseMemEncryptTdxLib/MemoryEncryption.c   | 48 +++++++++++++++----
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c b/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
index 503f626d75c6..5b13042512ad 100644
--- a/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
+++ b/OvmfPkg/Library/BaseMemEncryptTdxLib/MemoryEncryption.c
@@ -510,8 +510,11 @@ Split1GPageTo2M (
   @param[in]      PagetablePoint        Page table entry pointer (PTE).
   @param[in]      Mode                  Set or Clear shared bit
 
+  @retval         EFI_SUCCESS           Successfully set or clear the memory shared bit
+  @retval         Others                Other error as indicated
 **/
-STATIC VOID
+STATIC
+EFI_STATUS
 SetOrClearSharedBit (
   IN   OUT     UINT64              *PageTablePointer,
   IN           TDX_PAGETABLE_MODE  Mode,
@@ -520,7 +523,8 @@ SetOrClearSharedBit (
   )
 {
   UINT64                        AddressEncMask;
-  UINT64                        Status;
+  UINT64                        TdStatus;
+  EFI_STATUS                    Status;
   EDKII_MEMORY_ACCEPT_PROTOCOL  *MemoryAcceptProtocol;
 
   AddressEncMask = GetMemEncryptionAddressMask ();
@@ -536,16 +540,30 @@ SetOrClearSharedBit (
     PhysicalAddress   &= ~AddressEncMask;
   }
 
-  Status = TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0, NULL);
+  TdStatus = TdVmCall (TDVMCALL_MAPGPA, PhysicalAddress, Length, 0, 0, NULL);
+  if (TdStatus != 0) {
+    DEBUG ((DEBUG_ERROR, "%a: TdVmcall(MAPGPA) failed with %llx\n", __FUNCTION__, TdStatus));
+    ASSERT (FALSE);
+    return EFI_DEVICE_ERROR;
+  }
 
   //
   // If changing shared to private, must accept-page again
   //
   if (Mode == ClearSharedBit) {
     Status = gBS->LocateProtocol (&gEdkiiMemoryAcceptProtocolGuid, NULL, (VOID **)&MemoryAcceptProtocol);
-    ASSERT (!EFI_ERROR (Status));
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: Failed to locate MemoryAcceptProtocol with %r\n", __FUNCTION__, Status));
+      ASSERT (FALSE);
+      return Status;
+    }
+
     Status = MemoryAcceptProtocol->AcceptMemory (MemoryAcceptProtocol, PhysicalAddress, Length);
-    ASSERT (!EFI_ERROR (Status));
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: Failed to AcceptMemory with %r\n", __FUNCTION__, Status));
+      ASSERT (FALSE);
+      return Status;
+    }
   }
 
   DEBUG ((
@@ -558,6 +576,8 @@ SetOrClearSharedBit (
     Mode,
     Status
     ));
+
+  return EFI_SUCCESS;
 }
 
 /**
@@ -747,7 +767,11 @@ SetMemorySharedOrPrivate (
       // If we have at least 1GB to go, we can just update this entry
       //
       if (!(PhysicalAddress & (BIT30 - 1)) && (Length >= BIT30)) {
-        SetOrClearSharedBit (&PageDirectory1GEntry->Uint64, Mode, PhysicalAddress, BIT30);
+        Status = SetOrClearSharedBit (&PageDirectory1GEntry->Uint64, Mode, PhysicalAddress, BIT30);
+        if (EFI_ERROR (Status)) {
+          goto Done;
+        }
+
         DEBUG ((
           DEBUG_VERBOSE,
           "%a:%a: updated 1GB entry for Physical=0x%Lx\n",
@@ -809,7 +833,11 @@ SetMemorySharedOrPrivate (
         // If we have at least 2MB left to go, we can just update this entry
         //
         if (!(PhysicalAddress & (BIT21-1)) && (Length >= BIT21)) {
-          SetOrClearSharedBit (&PageDirectory2MEntry->Uint64, Mode, PhysicalAddress, BIT21);
+          Status = SetOrClearSharedBit (&PageDirectory2MEntry->Uint64, Mode, PhysicalAddress, BIT21);
+          if (EFI_ERROR (Status)) {
+            goto Done;
+          }
+
           PhysicalAddress += BIT21;
           Length          -= BIT21;
         } else {
@@ -856,7 +884,11 @@ SetMemorySharedOrPrivate (
           goto Done;
         }
 
-        SetOrClearSharedBit (&PageTableEntry->Uint64, Mode, PhysicalAddress, EFI_PAGE_SIZE);
+        Status = SetOrClearSharedBit (&PageTableEntry->Uint64, Mode, PhysicalAddress, EFI_PAGE_SIZE);
+        if (EFI_ERROR (Status)) {
+          goto Done;
+        }
+
         PhysicalAddress += EFI_PAGE_SIZE;
         Length          -= EFI_PAGE_SIZE;
       }
-- 
2.29.2.windows.2

Re: [PATCH V1 0/3] Fix TDVF issues

[Yao, Jiewen]

>From process perspective, I see no reason to combine them into one patch set, because each patch is individual, and they are handling different problem.
Also, there is no reason to mix the fix in SecurityPkg with the fix in OvmfPkg, if they are not related.

Please split them into 3 different patches.

With comment above, reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

Thank you
Yao, Jiewen

> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Monday, January 16, 2023 7:28 AM
> To: devel@edk2.groups.io
> Cc: Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Michael Roth <michael.roth@amd.com>
> Subject: [PATCH V1 0/3] Fix TDVF issues
> 
> This patch-set fix below TDVF issues:
> Patch#1: Initialize Status in IoExit
> Patch#2: Extend EFI boot variable to PCR[1] according to TCG PC Client
>          PFP spec.
> Patch#3: Refactor error handle of SetOrClearSharedBit so that the caller
>          can handle the returned error.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> 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>
> Cc: Michael Roth <michael.roth@amd.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> 
> Min M Xu (3):
>   OvmfPkg/CcExitLib: Initialize Status in IoExit
>   SecurityPkg/TdTcg2Dxe: Extend EFI boot variable to PCR[1]
>   OvmfPkg/BaseMemEncryptTdxLib: Refactor error handle of
>     SetOrClearSharedBit
> 
>  .../BaseMemEncryptTdxLib/MemoryEncryption.c   | 48 +++++++++++++++----
>  OvmfPkg/Library/CcExitLib/CcExitVeHandler.c   |  9 ++--
>  SecurityPkg/Tcg/TdTcg2Dxe/TdTcg2Dxe.c         |  6 +--
>  3 files changed, 46 insertions(+), 17 deletions(-)
> 
> --
> 2.29.2.windows.2

Re: [PATCH V1 0/3] Fix TDVF issues

[Min Xu]

Thanks for the comments. They're will be sent for review in separate patches.

> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Monday, January 16, 2023 8:45 AM
> To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Aktas, Erdem
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Michael Roth <michael.roth@amd.com>; Yao,
> Jiewen <jiewen.yao@intel.com>
> Subject: RE: [PATCH V1 0/3] Fix TDVF issues
> 
> From process perspective, I see no reason to combine them into one patch set,
> because each patch is individual, and they are handling different problem.
> Also, there is no reason to mix the fix in SecurityPkg with the fix in OvmfPkg, if
> they are not related.
> 
> Please split them into 3 different patches.
> 
> With comment above, reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> Thank you
> Yao, Jiewen
> 
> > -----Original Message-----
> > From: Xu, Min M <min.m.xu@intel.com>
> > Sent: Monday, January 16, 2023 7:28 AM
> > To: devel@edk2.groups.io
> > Cc: Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Aktas,
> > Erdem <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>;
> > Gerd Hoffmann <kraxel@redhat.com>; Tom Lendacky
> > <thomas.lendacky@amd.com>; Michael Roth <michael.roth@amd.com>
> > Subject: [PATCH V1 0/3] Fix TDVF issues
> >
> > This patch-set fix below TDVF issues:
> > Patch#1: Initialize Status in IoExit
> > Patch#2: Extend EFI boot variable to PCR[1] according to TCG PC Client
> >          PFP spec.
> > Patch#3: Refactor error handle of SetOrClearSharedBit so that the caller
> >          can handle the returned error.
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > 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>
> > Cc: Michael Roth <michael.roth@amd.com>
> > Signed-off-by: Min Xu <min.m.xu@intel.com>
> >
> > Min M Xu (3):
> >   OvmfPkg/CcExitLib: Initialize Status in IoExit
> >   SecurityPkg/TdTcg2Dxe: Extend EFI boot variable to PCR[1]
> >   OvmfPkg/BaseMemEncryptTdxLib: Refactor error handle of
> >     SetOrClearSharedBit
> >
> >  .../BaseMemEncryptTdxLib/MemoryEncryption.c   | 48 +++++++++++++++---
> -
> >  OvmfPkg/Library/CcExitLib/CcExitVeHandler.c   |  9 ++--
> >  SecurityPkg/Tcg/TdTcg2Dxe/TdTcg2Dxe.c         |  6 +--
> >  3 files changed, 46 insertions(+), 17 deletions(-)
> >
> > --
> > 2.29.2.windows.2