UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable

Root cause:
1. Before DisableReadonlyPageWriteProtect() is called, the return
address (coconut-svsm#1) is pushed in shadow stack.
2. CET is disabled.
3. DisableReadonlyPageWriteProtect() returns to coconut-svsm#1.
4. Page table is modified.
5. EnableReadonlyPageWriteProtect() is called, but the return
address (coconut-svsm#2) is not pushed in shadow stack.
6. CET is enabled.
7. EnableReadonlyPageWriteProtect() returns to coconut-svsm#2.
#CP exception happens because the actual return address (coconut-svsm#2)
doesn't match the return address stored in shadow stack (coconut-svsm#1).

Analysis:
Shadow stack will stop update after CET disable (DisableCet() in
DisableReadOnlyPageWriteProtect), but normal smi stack will be
continue updated with the function called and return
(DisableReadOnlyPageWriteProtect & EnableReadOnlyPageWriteProtect),
thus leading stack mismatch after CET re-enabled (EnableCet() in
EnableReadOnlyPageWriteProtect).

According SDM Vol 3, 6.15-Control Protection Exception:
Normal smi stack and shadow stack must be matched when CET enable,
otherwise CP Exception will happen, which is caused by a near RET
instruction.

CET is disabled in DisableCet(), while can be enabled in
EnableCet(). This way won't cause the problem because they are
implemented in a way that return address of DisableCet() is
poped out from shadow stack (Incsspq performs a pop to increases
the shadow stack) and EnableCet() doesn't use "RET" but "JMP" to
return to caller. So calling EnableCet() and DisableCet() doesn't
have the same issue as calling DisableReadonlyPageWriteProtect()
and EnableReadonlyPageWriteProtect().

With above root cause & analysis, define below 2 macros instead of
functions for WP & CET operation:
WRITE_UNPROTECT_RO_PAGES (Wp, Cet)
WRITE_PROTECT_RO_PAGES (Wp, Cet)
Because DisableCet() & EnableCet() must be in the same function
to avoid shadow stack and normal SMI stack mismatch.

Note: WRITE_UNPROTECT_RO_PAGES () must be called pair with
WRITE_PROTECT_RO_PAGES () in same function.

Change-Id: I4e126697efcd8dbfb4887da034d8691bfca969e3
Cc: Eric Dong <[email protected]>
Cc: Ray Ni <[email protected]>
Cc: Zeng Star <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Rahul Kumar <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Signed-off-by: Jiaxin Wu <[email protected]>
Reviewed-by: Laszlo Ersek <[email protected]>
Reviewed-by: Ray Ni <[email protected]>
Reviewed-by: Eric Dong <[email protected]>

Author: jiaxinwu

UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h

@@ -1553,27 +1553,62 @@ SmmWaitForApArrival (
   );
 
 /**
-  Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
+  Write unprotect read-only pages if Cr0.Bits.WP is 1.
+
+  @param[out]  WriteProtect      If Cr0.Bits.WP is enabled.
 
-  @param[out]  WpEnabled      If Cr0.WP is enabled.
-  @param[out]  CetEnabled     If CET is enabled.
 **/
 VOID
-DisableReadOnlyPageWriteProtect (
-  OUT BOOLEAN  *WpEnabled,
-  OUT BOOLEAN  *CetEnabled
+SmmWriteUnprotectReadOnlyPage (
+  OUT BOOLEAN  *WriteProtect
   );
 
 /**
-  Enable Write Protect on pages marked as read-only.
+  Write protect read-only pages.
+
+  @param[in]  WriteProtect      If Cr0.Bits.WP should be enabled.
 
-  @param[out]  WpEnabled      If Cr0.WP should be enabled.
-  @param[out]  CetEnabled     If CET should be enabled.
 **/
 VOID
-EnableReadOnlyPageWriteProtect (
-  BOOLEAN  WpEnabled,
-  BOOLEAN  CetEnabled
+SmmWriteProtectReadOnlyPage (
+  IN  BOOLEAN  WriteProtect
   );
 
+///
+/// Define macros to encapsulate the write unprotect/protect
+/// read-only pages.
+/// Below pieces of logic are defined as macros and not functions
+/// because "CET" feature disable & enable must be in the same
+/// function to avoid shadow stack and normal SMI stack mismatch,
+/// thus WRITE_UNPROTECT_RO_PAGES () must be called pair with
+/// WRITE_PROTECT_RO_PAGES () in same function.
+///
+/// @param[in,out] Wp   A BOOLEAN variable local to the containing
+///                     function, carrying write protection status from
+///                     WRITE_UNPROTECT_RO_PAGES() to
+///                     WRITE_PROTECT_RO_PAGES().
+///
+/// @param[in,out] Cet  A BOOLEAN variable local to the containing
+///                     function, carrying control flow integrity
+///                     enforcement status from
+///                     WRITE_UNPROTECT_RO_PAGES() to
+///                     WRITE_PROTECT_RO_PAGES().
+///
+#define WRITE_UNPROTECT_RO_PAGES(Wp, Cet) \
+  do { \
+    Cet = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0); \
+    if (Cet) { \
+      DisableCet (); \
+    } \
+    SmmWriteUnprotectReadOnlyPage (&Wp); \
+  } while (FALSE)
+
+#define WRITE_PROTECT_RO_PAGES(Wp, Cet) \
+  do { \
+    SmmWriteProtectReadOnlyPage (Wp); \
+    if (Cet) { \
+      EnableCet (); \
+    } \
+  } while (FALSE)
+
 #endif

UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c

@@ -41,60 +41,43 @@ PAGE_TABLE_POOL  *mPageTablePool = NULL;
 BOOLEAN  mIsReadOnlyPageTable = FALSE;
 
 /**
-  Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
+  Write unprotect read-only pages if Cr0.Bits.WP is 1.
+
+  @param[out]  WriteProtect      If Cr0.Bits.WP is enabled.
 
-  @param[out]  WpEnabled      If Cr0.WP is enabled.
-  @param[out]  CetEnabled     If CET is enabled.
 **/
 VOID
-DisableReadOnlyPageWriteProtect (
-  OUT BOOLEAN  *WpEnabled,
-  OUT BOOLEAN  *CetEnabled
+SmmWriteUnprotectReadOnlyPage (
+  OUT BOOLEAN  *WriteProtect
   )
 {
   IA32_CR0  Cr0;
 
-  *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
-  Cr0.UintN   = AsmReadCr0 ();
-  *WpEnabled  = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
-  if (*WpEnabled) {
-    if (*CetEnabled) {
-      //
-      // CET must be disabled if WP is disabled. Disable CET before clearing CR0.WP.
-      //
-      DisableCet ();
-    }
-
+  Cr0.UintN     = AsmReadCr0 ();
+  *WriteProtect = (Cr0.Bits.WP != 0);
+  if (*WriteProtect) {
     Cr0.Bits.WP = 0;
     AsmWriteCr0 (Cr0.UintN);
   }
 }
 
 /**
-  Enable Write Protect on pages marked as read-only.
+  Write protect read-only pages.
+
+  @param[in]  WriteProtect      If Cr0.Bits.WP should be enabled.
 
-  @param[out]  WpEnabled      If Cr0.WP should be enabled.
-  @param[out]  CetEnabled     If CET should be enabled.
 **/
 VOID
-EnableReadOnlyPageWriteProtect (
-  BOOLEAN  WpEnabled,
-  BOOLEAN  CetEnabled
+SmmWriteProtectReadOnlyPage (
+  IN  BOOLEAN  WriteProtect
   )
 {
   IA32_CR0  Cr0;
 
-  if (WpEnabled) {
+  if (WriteProtect) {
     Cr0.UintN   = AsmReadCr0 ();
     Cr0.Bits.WP = 1;
     AsmWriteCr0 (Cr0.UintN);
-
-    if (CetEnabled) {
-      //
-      // re-enable CET.
-      //
-      EnableCet ();
-    }
   }
 }
 
@@ -121,7 +104,7 @@ InitializePageTablePool (
   )
 {
   VOID     *Buffer;
-  BOOLEAN  WpEnabled;
+  BOOLEAN  WriteProtect;
   BOOLEAN  CetEnabled;
 
   //
@@ -159,9 +142,11 @@ InitializePageTablePool (
   // If page table memory has been marked as RO, mark the new pool pages as read-only.
   //
   if (mIsReadOnlyPageTable) {
-    DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+    WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
+
     SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
-    EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+
+    WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
   }
 
   return TRUE;
@@ -1011,7 +996,7 @@ SetMemMapAttributes (
   IA32_MAP_ENTRY                        *Map;
   UINTN                                 Count;
   UINT64                                MemoryAttribute;
-  BOOLEAN                               WpEnabled;
+  BOOLEAN                               WriteProtect;
   BOOLEAN                               CetEnabled;
 
   SmmGetSystemConfigurationTable (&gEdkiiPiSmmMemoryAttributesTableGuid, (VOID **)&MemoryAttributesTable);
@@ -1057,7 +1042,7 @@ SetMemMapAttributes (
 
   ASSERT_RETURN_ERROR (Status);
 
-  DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+  WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
 
   MemoryMap = MemoryMapStart;
   for (Index = 0; Index < MemoryMapEntryCount; Index++) {
@@ -1087,7 +1072,8 @@ SetMemMapAttributes (
     MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, DescriptorSize);
   }
 
-  EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+  WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
+
   FreePool (Map);
 
   PatchSmmSaveStateMap ();
@@ -1394,14 +1380,14 @@ SetUefiMemMapAttributes (
   UINTN                  MemoryMapEntryCount;
   UINTN                  Index;
   EFI_MEMORY_DESCRIPTOR  *Entry;
-  BOOLEAN                WpEnabled;
+  BOOLEAN                WriteProtect;
   BOOLEAN                CetEnabled;
 
   PERF_FUNCTION_BEGIN ();
 
   DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n"));
 
-  DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+  WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
 
   if (mUefiMemoryMap != NULL) {
     MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
@@ -1481,7 +1467,7 @@ SetUefiMemMapAttributes (
     }
   }
 
-  EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+  WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
 
   //
   // Do not free mUefiMemoryAttributesTable, it will be checked in IsSmmCommBufferForbiddenAddress().
@@ -1872,7 +1858,7 @@ SetPageTableAttributes (
   VOID
   )
 {
-  BOOLEAN  WpEnabled;
+  BOOLEAN  WriteProtect;
   BOOLEAN  CetEnabled;
 
   if (!IfReadOnlyPageTableNeeded ()) {
@@ -1886,7 +1872,7 @@ SetPageTableAttributes (
   // Disable write protection, because we need mark page table to be write protected.
   // We need *write* page table memory, to mark itself to be *read only*.
   //
-  DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+  WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
 
   // Set memory used by page table as Read Only.
   DEBUG ((DEBUG_INFO, "Start...\n"));
@@ -1895,7 +1881,8 @@ SetPageTableAttributes (
   //
   // Enable write protection, after page table attribute updated.
   //
-  EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
+  WRITE_PROTECT_RO_PAGES (TRUE, CetEnabled);
+
   mIsReadOnlyPageTable = TRUE;
 
   //

UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c

@@ -594,7 +594,7 @@ InitPaging (
   UINT64         Limit;
   UINT64         PreviousAddress;
   UINT64         MemoryAttrMask;
-  BOOLEAN        WpEnabled;
+  BOOLEAN        WriteProtect;
   BOOLEAN        CetEnabled;
 
   PERF_FUNCTION_BEGIN ();
@@ -606,7 +606,8 @@ InitPaging (
     Limit = (IsRestrictedMemoryAccess ()) ? LShiftU64 (1, mPhysicalAddressBits) : BASE_4GB;
   }
 
-  DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+  WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
+
   //
   // [0, 4k] may be non-present.
   //
@@ -672,7 +673,7 @@ InitPaging (
     ASSERT_RETURN_ERROR (Status);
   }
 
-  EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+  WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
 
   //
   // Flush TLB