public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-devel] [PATCH v1] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
@ 2023-11-03 12:14 Wu, Jiaxin
  2023-11-03 13:56 ` Laszlo Ersek
  2023-11-03 14:29 ` Laszlo Ersek
  0 siblings, 2 replies; 6+ messages in thread
From: Wu, Jiaxin @ 2023-11-03 12:14 UTC (permalink / raw)
  To: devel; +Cc: Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann, Rahul Kumar

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, CET feature enable & disable must be in the
same function to avoid stack mismatch issue.

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>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  10 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 104 ++++++++++++++-------
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c             |  19 +++-
 3 files changed, 91 insertions(+), 42 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 654935dc76..daa843b057 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -1554,26 +1554,24 @@ SmmWaitForApArrival (
 
 /**
   Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
 
   @param[out]  WpEnabled      If Cr0.WP is enabled.
-  @param[out]  CetEnabled     If CET is enabled.
+
 **/
 VOID
 DisableReadOnlyPageWriteProtect (
-  OUT BOOLEAN  *WpEnabled,
-  OUT BOOLEAN  *CetEnabled
+  OUT BOOLEAN  *WpEnabled
   );
 
 /**
   Enable Write Protect on pages marked as read-only.
 
   @param[out]  WpEnabled      If Cr0.WP should be enabled.
-  @param[out]  CetEnabled     If CET should be enabled.
+
 **/
 VOID
 EnableReadOnlyPageWriteProtect (
-  BOOLEAN  WpEnabled,
-  BOOLEAN  CetEnabled
+  BOOLEAN  WpEnabled
   );
 
 #endif
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 6f49866615..2c198a161a 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -42,61 +42,44 @@ BOOLEAN  mIsReadOnlyPageTable = FALSE;
 
 /**
   Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
 
   @param[out]  WpEnabled      If Cr0.WP is enabled.
-  @param[out]  CetEnabled     If CET is enabled.
+
 **/
 VOID
 DisableReadOnlyPageWriteProtect (
-  OUT BOOLEAN  *WpEnabled,
-  OUT BOOLEAN  *CetEnabled
+  OUT BOOLEAN  *WpEnabled
   )
 {
   IA32_CR0  Cr0;
 
-  *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
-  Cr0.UintN   = AsmReadCr0 ();
-  *WpEnabled  = (Cr0.Bits.WP != 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.Bits.WP = 0;
     AsmWriteCr0 (Cr0.UintN);
   }
 }
 
 /**
   Enable Write Protect on pages marked as read-only.
 
   @param[out]  WpEnabled      If Cr0.WP should be enabled.
-  @param[out]  CetEnabled     If CET should be enabled.
+
 **/
 VOID
 EnableReadOnlyPageWriteProtect (
-  BOOLEAN  WpEnabled,
-  BOOLEAN  CetEnabled
+  BOOLEAN  WpEnabled
   )
 {
   IA32_CR0  Cr0;
 
   if (WpEnabled) {
     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.
@@ -157,13 +140,29 @@ InitializePageTablePool (
 
   //
   // If page table memory has been marked as RO, mark the new pool pages as read-only.
   //
   if (mIsReadOnlyPageTable) {
-    DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+    //
+    // CET must be disabled if WP is disabled.
+    //
+    CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
+    if (CetEnabled) {
+      DisableCet ();
+    }
+
+    DisableReadOnlyPageWriteProtect (&WpEnabled);
+
     SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
-    EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+
+    //
+    // Enable the WP and restore CET to enable
+    //
+    EnableReadOnlyPageWriteProtect (WpEnabled);
+    if (CetEnabled) {
+      EnableCet ();
+    }
   }
 
   return TRUE;
 }
 
@@ -1055,11 +1054,19 @@ SetMemMapAttributes (
     Status = PageTableParse (PageTable, mPagingMode, Map, &Count);
   }
 
   ASSERT_RETURN_ERROR (Status);
 
-  DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+  //
+  // CET must be disabled if WP is disabled.
+  //
+  CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
+  if (CetEnabled) {
+    DisableCet ();
+  }
+
+  DisableReadOnlyPageWriteProtect (&WpEnabled);
 
   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 +1092,18 @@ SetMemMapAttributes (
       );
 
     MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, DescriptorSize);
   }
 
-  EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+  //
+  // Enable the WP and restore CET to enable
+  //
+  EnableReadOnlyPageWriteProtect (WpEnabled);
+  if (CetEnabled) {
+    EnableCet ();
+  }
+
   FreePool (Map);
 
   PatchSmmSaveStateMap ();
   PatchGdtIdtMap ();
 
@@ -1399,11 +1413,19 @@ SetUefiMemMapAttributes (
 
   PERF_FUNCTION_BEGIN ();
 
   DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n"));
 
-  DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+  //
+  // CET must be disabled if WP is disabled.
+  //
+  CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
+  if (CetEnabled) {
+    DisableCet ();
+  }
+
+  DisableReadOnlyPageWriteProtect (&WpEnabled);
 
   if (mUefiMemoryMap != NULL) {
     MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
     MemoryMap           = mUefiMemoryMap;
     for (Index = 0; Index < MemoryMapEntryCount; Index++) {
@@ -1479,11 +1501,17 @@ SetUefiMemMapAttributes (
 
       Entry = NEXT_MEMORY_DESCRIPTOR (Entry, mUefiMemoryAttributesTable->DescriptorSize);
     }
   }
 
-  EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+  //
+  // Enable the WP and restore CET to enable
+  //
+  EnableReadOnlyPageWriteProtect (WpEnabled);
+  if (CetEnabled) {
+    EnableCet ();
+  }
 
   //
   // Do not free mUefiMemoryAttributesTable, it will be checked in IsSmmCommBufferForbiddenAddress().
   //
 
@@ -1881,23 +1909,31 @@ SetPageTableAttributes (
 
   PERF_FUNCTION_BEGIN ();
   DEBUG ((DEBUG_INFO, "SetPageTableAttributes\n"));
 
   //
-  // 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*.
+  // CET must be disabled if WP is disabled.
   //
-  DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+  CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
+  if (CetEnabled) {
+    DisableCet ();
+  }
+
+  DisableReadOnlyPageWriteProtect (&WpEnabled);
 
   // Set memory used by page table as Read Only.
   DEBUG ((DEBUG_INFO, "Start...\n"));
   EnablePageTableProtection ();
 
   //
-  // Enable write protection, after page table attribute updated.
+  // Enable the WP and restore CET to enable
   //
-  EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
+  EnableReadOnlyPageWriteProtect (WpEnabled);
+  if (CetEnabled) {
+    EnableCet ();
+  }
+
   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..71fdf5f34a 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -604,11 +604,20 @@ InitPaging (
     Limit = BASE_4GB;
   } else {
     Limit = (IsRestrictedMemoryAccess ()) ? LShiftU64 (1, mPhysicalAddressBits) : BASE_4GB;
   }
 
-  DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+  //
+  // CET must be disabled if WP is disabled.
+  //
+  CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
+  if (CetEnabled) {
+    DisableCet ();
+  }
+
+  DisableReadOnlyPageWriteProtect (&WpEnabled);
+
   //
   // [0, 4k] may be non-present.
   //
   PreviousAddress = ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0) ? BASE_4KB : 0;
 
@@ -670,11 +679,17 @@ InitPaging (
     //
     Status = ConvertMemoryPageAttributes (PageTable, mPagingMode, PreviousAddress, Limit - PreviousAddress, MemoryAttrMask, TRUE, NULL);
     ASSERT_RETURN_ERROR (Status);
   }
 
-  EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+  //
+  // Enable the WP and restore CET to enable
+  //
+  EnableReadOnlyPageWriteProtect (WpEnabled);
+  if (CetEnabled) {
+    EnableCet ();
+  }
 
   //
   // Flush TLB
   //
   CpuFlushTlb ();
-- 
2.16.2.windows.1



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



^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-11-03 18:38 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-03 12:14 [edk2-devel] [PATCH v1] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable Wu, Jiaxin
2023-11-03 13:56 ` Laszlo Ersek
2023-11-03 14:10   ` Wu, Jiaxin
2023-11-03 14:12     ` Laszlo Ersek
2023-11-03 14:29 ` Laszlo Ersek
2023-11-03 18:38   ` Wu, Jiaxin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox