[edk2-devel] [PATCH v3] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

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

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 (See SDM Vol 3, 6.15-Control Protection Exception).

With above requirement, define below 2 macros instead of functions
for WP & CET operation:
WRITE_UNPROTECT_RO_PAGES (Wp, Cet)
WRITE_PROTECT_RO_PAGES (Wp, Cet)
Because "CET" feature disable & enable 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.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         | 59 +++++++++++++----
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 73 +++++++++-------------
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c             |  7 ++-
 3 files changed, 81 insertions(+), 58 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 654935dc76..20ada465c2 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -1551,29 +1551,64 @@ VOID
 SmmWaitForApArrival (
   VOID
   );
 
 /**
-  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
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 6f49866615..3d445df213 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -39,64 +39,47 @@ PAGE_TABLE_POOL  *mPageTablePool = NULL;
 // If memory used by SMM page table has been mareked as ReadOnly.
 //
 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 ();
-    }
   }
 }
 
 /**
   Initialize a buffer pool for page table use only.
@@ -119,11 +102,11 @@ BOOLEAN
 InitializePageTablePool (
   IN UINTN  PoolPages
   )
 {
   VOID     *Buffer;
-  BOOLEAN  WpEnabled;
+  BOOLEAN  WriteProtect;
   BOOLEAN  CetEnabled;
 
   //
   // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
   // header.
@@ -157,13 +140,15 @@ 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;
 }
 
@@ -1009,11 +994,11 @@ SetMemMapAttributes (
   UINTN                                 PageTable;
   EFI_STATUS                            Status;
   IA32_MAP_ENTRY                        *Map;
   UINTN                                 Count;
   UINT64                                MemoryAttribute;
-  BOOLEAN                               WpEnabled;
+  BOOLEAN                               WriteProtect;
   BOOLEAN                               CetEnabled;
 
   SmmGetSystemConfigurationTable (&gEdkiiPiSmmMemoryAttributesTableGuid, (VOID **)&MemoryAttributesTable);
   if (MemoryAttributesTable == NULL) {
     DEBUG ((DEBUG_INFO, "MemoryAttributesTable - NULL\n"));
@@ -1055,11 +1040,11 @@ SetMemMapAttributes (
     Status = PageTableParse (PageTable, mPagingMode, Map, &Count);
   }
 
   ASSERT_RETURN_ERROR (Status);
 
-  DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+  WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
 
   MemoryMap = MemoryMapStart;
   for (Index = 0; Index < MemoryMapEntryCount; Index++) {
     DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx, 0x%x\n", MemoryMap->PhysicalStart, MemoryMap->NumberOfPages));
     if (MemoryMap->Type == EfiRuntimeServicesCode) {
@@ -1085,11 +1070,12 @@ SetMemMapAttributes (
       );
 
     MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, DescriptorSize);
   }
 
-  EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+  WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
+
   FreePool (Map);
 
   PatchSmmSaveStateMap ();
   PatchGdtIdtMap ();
 
@@ -1392,18 +1378,18 @@ SetUefiMemMapAttributes (
   EFI_STATUS             Status;
   EFI_MEMORY_DESCRIPTOR  *MemoryMap;
   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;
     MemoryMap           = mUefiMemoryMap;
     for (Index = 0; Index < MemoryMapEntryCount; Index++) {
@@ -1479,11 +1465,11 @@ SetUefiMemMapAttributes (
 
       Entry = NEXT_MEMORY_DESCRIPTOR (Entry, mUefiMemoryAttributesTable->DescriptorSize);
     }
   }
 
-  EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+  WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
 
   //
   // Do not free mUefiMemoryAttributesTable, it will be checked in IsSmmCommBufferForbiddenAddress().
   //
 
@@ -1870,11 +1856,11 @@ IfReadOnlyPageTableNeeded (
 VOID
 SetPageTableAttributes (
   VOID
   )
 {
-  BOOLEAN  WpEnabled;
+  BOOLEAN  WriteProtect;
   BOOLEAN  CetEnabled;
 
   if (!IfReadOnlyPageTableNeeded ()) {
     return;
   }
@@ -1884,20 +1870,21 @@ 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"));
   EnablePageTableProtection ();
 
   //
   // Enable write protection, after page table attribute updated.
   //
-  EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
+  WRITE_PROTECT_RO_PAGES (TRUE, CetEnabled);
+
   mIsReadOnlyPageTable = TRUE;
 
   //
   // Flush TLB after mark all page table pool as read only.
   //
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
index 7ac3c66f91..8142d3ceac 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -592,11 +592,11 @@ InitPaging (
   UINT64         Base;
   UINT64         Length;
   UINT64         Limit;
   UINT64         PreviousAddress;
   UINT64         MemoryAttrMask;
-  BOOLEAN        WpEnabled;
+  BOOLEAN        WriteProtect;
   BOOLEAN        CetEnabled;
 
   PERF_FUNCTION_BEGIN ();
 
   PageTable = AsmReadCr3 ();
@@ -604,11 +604,12 @@ InitPaging (
     Limit = BASE_4GB;
   } else {
     Limit = (IsRestrictedMemoryAccess ()) ? LShiftU64 (1, mPhysicalAddressBits) : BASE_4GB;
   }
 
-  DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+  WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
+
   //
   // [0, 4k] may be non-present.
   //
   PreviousAddress = ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0) ? BASE_4KB : 0;
 
@@ -670,11 +671,11 @@ InitPaging (
     //
     Status = ConvertMemoryPageAttributes (PageTable, mPagingMode, PreviousAddress, Limit - PreviousAddress, MemoryAttrMask, TRUE, NULL);
     ASSERT_RETURN_ERROR (Status);
   }
 
-  EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+  WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
 
   //
   // Flush TLB
   //
   CpuFlushTlb ();
-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110695): https://edk2.groups.io/g/devel/message/110695
Mute This Topic: https://groups.io/mt/102413389/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-