* [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
@ 2023-11-07 1:24 Wu, Jiaxin
2023-11-07 18:56 ` Laszlo Ersek
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Wu, Jiaxin @ 2023-11-07 1:24 UTC (permalink / raw)
To: devel
Cc: Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann, Rahul Kumar,
Laszlo Ersek
Root cause:
1. Before DisableReadonlyPageWriteProtect() is called, the return
address (#1) is pushed in shadow stack.
2. CET is disabled.
3. DisableReadonlyPageWriteProtect() returns to #1.
4. Page table is modified.
5. EnableReadonlyPageWriteProtect() is called, but the return
address (#2) is not pushed in shadow stack.
6. CET is enabled.
7. EnableReadonlyPageWriteProtect() returns to #2.
#CP exception happens because the actual return address (#2)
doesn't match the return address stored in shadow stack (#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.
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 (#110780): https://edk2.groups.io/g/devel/message/110780
Mute This Topic: https://groups.io/mt/102434876/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] 11+ messages in thread
* Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
[not found] <179532CD4E894831.20624@groups.io>
@ 2023-11-07 12:01 ` Wu, Jiaxin
2023-11-07 13:08 ` Laszlo Ersek
0 siblings, 1 reply; 11+ messages in thread
From: Wu, Jiaxin @ 2023-11-07 12:01 UTC (permalink / raw)
To: devel@edk2.groups.io, Wu, Jiaxin, Gao, Liming, Kinney, Michael D
Cc: Dong, Eric, Ni, Ray, Zeng, Star, Gerd Hoffmann, Kumar, Rahul R,
Laszlo Ersek
Hi Ray & Laszlo,
Any more comments to this?
Thanks,
Jiaxin
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> Jiaxin
> Sent: Tuesday, November 7, 2023 9:25 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP
> Exception when CET enable
>
> Root cause:
> 1. Before DisableReadonlyPageWriteProtect() is called, the return
> address (#1) is pushed in shadow stack.
> 2. CET is disabled.
> 3. DisableReadonlyPageWriteProtect() returns to #1.
> 4. Page table is modified.
> 5. EnableReadonlyPageWriteProtect() is called, but the return
> address (#2) is not pushed in shadow stack.
> 6. CET is enabled.
> 7. EnableReadonlyPageWriteProtect() returns to #2.
> #CP exception happens because the actual return address (#2)
> doesn't match the return address stored in shadow stack (#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.
>
> 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 (#110845): https://edk2.groups.io/g/devel/message/110845
Mute This Topic: https://groups.io/mt/102434876/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
2023-11-07 12:01 ` Wu, Jiaxin
@ 2023-11-07 13:08 ` Laszlo Ersek
0 siblings, 0 replies; 11+ messages in thread
From: Laszlo Ersek @ 2023-11-07 13:08 UTC (permalink / raw)
To: Wu, Jiaxin, devel@edk2.groups.io, Gao, Liming, Kinney, Michael D
Cc: Dong, Eric, Ni, Ray, Zeng, Star, Gerd Hoffmann, Kumar, Rahul R
On 11/7/23 13:01, Wu, Jiaxin wrote:
> Hi Ray & Laszlo,
>
> Any more comments to this?
It's in my review queue.
Laszlo
>
> Thanks,
> Jiaxin
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
>> Jiaxin
>> Sent: Tuesday, November 7, 2023 9:25 AM
>> To: devel@edk2.groups.io
>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
>> <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R
>> <rahul.r.kumar@intel.com>; Laszlo Ersek <lersek@redhat.com>
>> Subject: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP
>> Exception when CET enable
>>
>> Root cause:
>> 1. Before DisableReadonlyPageWriteProtect() is called, the return
>> address (#1) is pushed in shadow stack.
>> 2. CET is disabled.
>> 3. DisableReadonlyPageWriteProtect() returns to #1.
>> 4. Page table is modified.
>> 5. EnableReadonlyPageWriteProtect() is called, but the return
>> address (#2) is not pushed in shadow stack.
>> 6. CET is enabled.
>> 7. EnableReadonlyPageWriteProtect() returns to #2.
>> #CP exception happens because the actual return address (#2)
>> doesn't match the return address stored in shadow stack (#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.
>>
>> 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 (#110850): https://edk2.groups.io/g/devel/message/110850
Mute This Topic: https://groups.io/mt/102434876/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
2023-11-07 1:24 [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable Wu, Jiaxin
@ 2023-11-07 18:56 ` Laszlo Ersek
2023-11-08 1:17 ` Wu, Jiaxin
2023-11-10 0:01 ` Wu, Jiaxin
2023-11-09 3:45 ` Ni, Ray
2023-11-09 5:06 ` Dong, Eric
2 siblings, 2 replies; 11+ messages in thread
From: Laszlo Ersek @ 2023-11-07 18:56 UTC (permalink / raw)
To: devel, jiaxin.wu; +Cc: Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann, Rahul Kumar
On 11/7/23 02:24, Wu, Jiaxin wrote:
> Root cause:
> 1. Before DisableReadonlyPageWriteProtect() is called, the return
> address (#1) is pushed in shadow stack.
> 2. CET is disabled.
> 3. DisableReadonlyPageWriteProtect() returns to #1.
> 4. Page table is modified.
> 5. EnableReadonlyPageWriteProtect() is called, but the return
> address (#2) is not pushed in shadow stack.
> 6. CET is enabled.
> 7. EnableReadonlyPageWriteProtect() returns to #2.
> #CP exception happens because the actual return address (#2)
> doesn't match the return address stored in shadow stack (#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.
>
> 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(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110875): https://edk2.groups.io/g/devel/message/110875
Mute This Topic: https://groups.io/mt/102434876/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
2023-11-07 18:56 ` Laszlo Ersek
@ 2023-11-08 1:17 ` Wu, Jiaxin
2023-11-10 0:01 ` Wu, Jiaxin
1 sibling, 0 replies; 11+ messages in thread
From: Wu, Jiaxin @ 2023-11-08 1:17 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io, Gao, Liming,
Kinney, Michael D
Cc: Dong, Eric, Ni, Ray, Zeng, Star, Gerd Hoffmann, Kumar, Rahul R
Hi Liming & Mike & Ray,
Could you help approve this change for the coming edk2 stable tag? This is critical bug fix in smm cpu driver to handler the CET check failure, I think we need this change for the stable tag.
Thanks,
Jiaxin
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, November 8, 2023 2:57 AM
> To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP
> Exception when CET enable
>
> On 11/7/23 02:24, Wu, Jiaxin wrote:
> > Root cause:
> > 1. Before DisableReadonlyPageWriteProtect() is called, the return
> > address (#1) is pushed in shadow stack.
> > 2. CET is disabled.
> > 3. DisableReadonlyPageWriteProtect() returns to #1.
> > 4. Page table is modified.
> > 5. EnableReadonlyPageWriteProtect() is called, but the return
> > address (#2) is not pushed in shadow stack.
> > 6. CET is enabled.
> > 7. EnableReadonlyPageWriteProtect() returns to #2.
> > #CP exception happens because the actual return address (#2)
> > doesn't match the return address stored in shadow stack (#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.
> >
> > 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(-)
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110880): https://edk2.groups.io/g/devel/message/110880
Mute This Topic: https://groups.io/mt/102434876/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
2023-11-07 1:24 [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable Wu, Jiaxin
2023-11-07 18:56 ` Laszlo Ersek
@ 2023-11-09 3:45 ` Ni, Ray
2023-11-09 5:06 ` Dong, Eric
2 siblings, 0 replies; 11+ messages in thread
From: Ni, Ray @ 2023-11-09 3:45 UTC (permalink / raw)
To: Wu, Jiaxin, devel@edk2.groups.io
Cc: Dong, Eric, Zeng, Star, Gerd Hoffmann, Kumar, Rahul R,
Laszlo Ersek
[-- Attachment #1: Type: text/plain, Size: 14453 bytes --]
Reviewed-by: Ray Ni <ray.ni@intel.com>
Thanks,
Ray
________________________________
From: Wu, Jiaxin <jiaxin.wu@intel.com>
Sent: Tuesday, November 7, 2023 9:24 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
Root cause:
1. Before DisableReadonlyPageWriteProtect() is called, the return
address (#1) is pushed in shadow stack.
2. CET is disabled.
3. DisableReadonlyPageWriteProtect() returns to #1.
4. Page table is modified.
5. EnableReadonlyPageWriteProtect() is called, but the return
address (#2) is not pushed in shadow stack.
6. CET is enabled.
7. EnableReadonlyPageWriteProtect() returns to #2.
#CP exception happens because the actual return address (#2)
doesn't match the return address stored in shadow stack (#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.
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 (#110946): https://edk2.groups.io/g/devel/message/110946
Mute This Topic: https://groups.io/mt/102434876/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
[-- Attachment #2: Type: text/html, Size: 24079 bytes --]
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
2023-11-07 1:24 [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable Wu, Jiaxin
2023-11-07 18:56 ` Laszlo Ersek
2023-11-09 3:45 ` Ni, Ray
@ 2023-11-09 5:06 ` Dong, Eric
2 siblings, 0 replies; 11+ messages in thread
From: Dong, Eric @ 2023-11-09 5:06 UTC (permalink / raw)
To: Wu, Jiaxin, devel@edk2.groups.io
Cc: Ni, Ray, Zeng, Star, Gerd Hoffmann, Kumar, Rahul R, Laszlo Ersek
Reviewed-by: Eric Dong <eric.dong@intel.com>
-----Original Message-----
From: Wu, Jiaxin <jiaxin.wu@intel.com>
Sent: Tuesday, November 7, 2023 9:25 AM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
Root cause:
1. Before DisableReadonlyPageWriteProtect() is called, the return address (#1) is pushed in shadow stack.
2. CET is disabled.
3. DisableReadonlyPageWriteProtect() returns to #1.
4. Page table is modified.
5. EnableReadonlyPageWriteProtect() is called, but the return address (#2) is not pushed in shadow stack.
6. CET is enabled.
7. EnableReadonlyPageWriteProtect() returns to #2.
#CP exception happens because the actual return address (#2) doesn't match the return address stored in shadow stack (#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.
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 (#110948): https://edk2.groups.io/g/devel/message/110948
Mute This Topic: https://groups.io/mt/102434876/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] 11+ messages in thread
* Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
2023-11-07 18:56 ` Laszlo Ersek
2023-11-08 1:17 ` Wu, Jiaxin
@ 2023-11-10 0:01 ` Wu, Jiaxin
2023-11-10 0:25 ` Michael D Kinney
1 sibling, 1 reply; 11+ messages in thread
From: Wu, Jiaxin @ 2023-11-10 0:01 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io, Gao, Liming,
Kinney, Michael D
Cc: Dong, Eric, Ni, Ray, Zeng, Star, Gerd Hoffmann, Kumar, Rahul R
Hi Liming & Mike,
Could you help approve & merge this patch into stable tag? It has got below reviewed-by:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Reviewed-by: Eric Dong <eric.dong@intel.com>
I also created the PR: https://github.com/tianocore/edk2/pull/4867
Thanks,
Jiaxin
> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Wednesday, November 8, 2023 9:17 AM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Gao, Liming
> <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>
> Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP
> Exception when CET enable
>
> Hi Liming & Mike & Ray,
>
> Could you help approve this change for the coming edk2 stable tag? This is
> critical bug fix in smm cpu driver to handler the CET check failure, I think we
> need this change for the stable tag.
>
> Thanks,
> Jiaxin
>
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: Wednesday, November 8, 2023 2:57 AM
> > To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> > <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul
> R
> > <rahul.r.kumar@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix
> CP
> > Exception when CET enable
> >
> > On 11/7/23 02:24, Wu, Jiaxin wrote:
> > > Root cause:
> > > 1. Before DisableReadonlyPageWriteProtect() is called, the return
> > > address (#1) is pushed in shadow stack.
> > > 2. CET is disabled.
> > > 3. DisableReadonlyPageWriteProtect() returns to #1.
> > > 4. Page table is modified.
> > > 5. EnableReadonlyPageWriteProtect() is called, but the return
> > > address (#2) is not pushed in shadow stack.
> > > 6. CET is enabled.
> > > 7. EnableReadonlyPageWriteProtect() returns to #2.
> > > #CP exception happens because the actual return address (#2)
> > > doesn't match the return address stored in shadow stack (#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.
> > >
> > > 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(-)
> >
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111001): https://edk2.groups.io/g/devel/message/111001
Mute This Topic: https://groups.io/mt/102434876/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
2023-11-10 0:01 ` Wu, Jiaxin
@ 2023-11-10 0:25 ` Michael D Kinney
2023-11-10 0:43 ` Wu, Jiaxin
0 siblings, 1 reply; 11+ messages in thread
From: Michael D Kinney @ 2023-11-10 0:25 UTC (permalink / raw)
To: Wu, Jiaxin, Laszlo Ersek, devel@edk2.groups.io, Gao, Liming
Cc: Dong, Eric, Ni, Ray, Zeng, Star, Gerd Hoffmann, Kumar, Rahul R,
Kinney, Michael D
I approve this change for edk2-stable202311
The PR looks out of sync with this email patch.
Can you please update PR with latest patch and commit
message that was reviewed and add review tags?
Mike
> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Thursday, November 9, 2023 4:01 PM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Gao,
> Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP
> Exception when CET enable
>
> Hi Liming & Mike,
>
> Could you help approve & merge this patch into stable tag? It has got
> below reviewed-by:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> Reviewed-by: Eric Dong <eric.dong@intel.com>
>
> I also created the PR: https://github.com/tianocore/edk2/pull/4867
>
> Thanks,
> Jiaxin
>
>
> > -----Original Message-----
> > From: Wu, Jiaxin
> > Sent: Wednesday, November 8, 2023 9:17 AM
> > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Gao,
> Liming
> > <gaoliming@byosoft.com.cn>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Zeng, Star
> > <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar,
> Rahul R
> > <rahul.r.kumar@intel.com>
> > Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix
> CP
> > Exception when CET enable
> >
> > Hi Liming & Mike & Ray,
> >
> > Could you help approve this change for the coming edk2 stable tag?
> This is
> > critical bug fix in smm cpu driver to handler the CET check failure,
> I think we
> > need this change for the stable tag.
> >
> > Thanks,
> > Jiaxin
> >
> > > -----Original Message-----
> > > From: Laszlo Ersek <lersek@redhat.com>
> > > Sent: Wednesday, November 8, 2023 2:57 AM
> > > To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> > > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Zeng, Star
> > > <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar,
> Rahul
> > R
> > > <rahul.r.kumar@intel.com>
> > > Subject: Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm:
> Fix
> > CP
> > > Exception when CET enable
> > >
> > > On 11/7/23 02:24, Wu, Jiaxin wrote:
> > > > Root cause:
> > > > 1. Before DisableReadonlyPageWriteProtect() is called, the
> return
> > > > address (#1) is pushed in shadow stack.
> > > > 2. CET is disabled.
> > > > 3. DisableReadonlyPageWriteProtect() returns to #1.
> > > > 4. Page table is modified.
> > > > 5. EnableReadonlyPageWriteProtect() is called, but the return
> > > > address (#2) is not pushed in shadow stack.
> > > > 6. CET is enabled.
> > > > 7. EnableReadonlyPageWriteProtect() returns to #2.
> > > > #CP exception happens because the actual return address (#2)
> > > > doesn't match the return address stored in shadow stack (#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.
> > > >
> > > > 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(-)
> > >
> > > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111002): https://edk2.groups.io/g/devel/message/111002
Mute This Topic: https://groups.io/mt/102434876/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
2023-11-10 0:25 ` Michael D Kinney
@ 2023-11-10 0:43 ` Wu, Jiaxin
2023-11-10 8:36 ` Ni, Ray
0 siblings, 1 reply; 11+ messages in thread
From: Wu, Jiaxin @ 2023-11-10 0:43 UTC (permalink / raw)
To: Kinney, Michael D, Laszlo Ersek, devel@edk2.groups.io,
Gao, Liming
Cc: Dong, Eric, Ni, Ray, Zeng, Star, Gerd Hoffmann, Kumar, Rahul R
Thank you, Mike, the PR (https://github.com/tianocore/edk2/pull/4867) has been synced & updated with reviewed by tag, and we can merge once pass the CI check.
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Friday, November 10, 2023 8:25 AM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP
> Exception when CET enable
>
> I approve this change for edk2-stable202311
>
> The PR looks out of sync with this email patch.
>
> Can you please update PR with latest patch and commit
> message that was reviewed and add review tags?
>
> Mike
>
> > -----Original Message-----
> > From: Wu, Jiaxin <jiaxin.wu@intel.com>
> > Sent: Thursday, November 9, 2023 4:01 PM
> > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Gao,
> > Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> > Kumar, Rahul R <rahul.r.kumar@intel.com>
> > Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix
> CP
> > Exception when CET enable
> >
> > Hi Liming & Mike,
> >
> > Could you help approve & merge this patch into stable tag? It has got
> > below reviewed-by:
> >
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > Reviewed-by: Ray Ni <ray.ni@intel.com>
> > Reviewed-by: Eric Dong <eric.dong@intel.com>
> >
> > I also created the PR: https://github.com/tianocore/edk2/pull/4867
> >
> > Thanks,
> > Jiaxin
> >
> >
> > > -----Original Message-----
> > > From: Wu, Jiaxin
> > > Sent: Wednesday, November 8, 2023 9:17 AM
> > > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Gao,
> > Liming
> > > <gaoliming@byosoft.com.cn>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>
> > > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > Zeng, Star
> > > <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar,
> > Rahul R
> > > <rahul.r.kumar@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix
> > CP
> > > Exception when CET enable
> > >
> > > Hi Liming & Mike & Ray,
> > >
> > > Could you help approve this change for the coming edk2 stable tag?
> > This is
> > > critical bug fix in smm cpu driver to handler the CET check failure,
> > I think we
> > > need this change for the stable tag.
> > >
> > > Thanks,
> > > Jiaxin
> > >
> > > > -----Original Message-----
> > > > From: Laszlo Ersek <lersek@redhat.com>
> > > > Sent: Wednesday, November 8, 2023 2:57 AM
> > > > To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> > > > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > Zeng, Star
> > > > <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar,
> > Rahul
> > > R
> > > > <rahul.r.kumar@intel.com>
> > > > Subject: Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm:
> > Fix
> > > CP
> > > > Exception when CET enable
> > > >
> > > > On 11/7/23 02:24, Wu, Jiaxin wrote:
> > > > > Root cause:
> > > > > 1. Before DisableReadonlyPageWriteProtect() is called, the
> > return
> > > > > address (#1) is pushed in shadow stack.
> > > > > 2. CET is disabled.
> > > > > 3. DisableReadonlyPageWriteProtect() returns to #1.
> > > > > 4. Page table is modified.
> > > > > 5. EnableReadonlyPageWriteProtect() is called, but the return
> > > > > address (#2) is not pushed in shadow stack.
> > > > > 6. CET is enabled.
> > > > > 7. EnableReadonlyPageWriteProtect() returns to #2.
> > > > > #CP exception happens because the actual return address (#2)
> > > > > doesn't match the return address stored in shadow stack (#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.
> > > > >
> > > > > 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(-)
> > > >
> > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111004): https://edk2.groups.io/g/devel/message/111004
Mute This Topic: https://groups.io/mt/102434876/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
2023-11-10 0:43 ` Wu, Jiaxin
@ 2023-11-10 8:36 ` Ni, Ray
0 siblings, 0 replies; 11+ messages in thread
From: Ni, Ray @ 2023-11-10 8:36 UTC (permalink / raw)
To: Wu, Jiaxin, Kinney, Michael D, Laszlo Ersek, devel@edk2.groups.io,
Gao, Liming
Cc: Dong, Eric, Zeng, Star, Gerd Hoffmann, Kumar, Rahul R
[-- Attachment #1: Type: text/plain, Size: 7712 bytes --]
merged.
Thanks,
Ray
________________________________
From: Wu, Jiaxin <jiaxin.wu@intel.com>
Sent: Friday, November 10, 2023 8:43 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io <devel@edk2.groups.io>; Gao, Liming <gaoliming@byosoft.com.cn>
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
Thank you, Mike, the PR (https://github.com/tianocore/edk2/pull/4867) has been synced & updated with reviewed by tag, and we can merge once pass the CI check.
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Friday, November 10, 2023 8:25 AM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP
> Exception when CET enable
>
> I approve this change for edk2-stable202311
>
> The PR looks out of sync with this email patch.
>
> Can you please update PR with latest patch and commit
> message that was reviewed and add review tags?
>
> Mike
>
> > -----Original Message-----
> > From: Wu, Jiaxin <jiaxin.wu@intel.com>
> > Sent: Thursday, November 9, 2023 4:01 PM
> > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Gao,
> > Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> > Kumar, Rahul R <rahul.r.kumar@intel.com>
> > Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix
> CP
> > Exception when CET enable
> >
> > Hi Liming & Mike,
> >
> > Could you help approve & merge this patch into stable tag? It has got
> > below reviewed-by:
> >
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > Reviewed-by: Ray Ni <ray.ni@intel.com>
> > Reviewed-by: Eric Dong <eric.dong@intel.com>
> >
> > I also created the PR: https://github.com/tianocore/edk2/pull/4867
> >
> > Thanks,
> > Jiaxin
> >
> >
> > > -----Original Message-----
> > > From: Wu, Jiaxin
> > > Sent: Wednesday, November 8, 2023 9:17 AM
> > > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Gao,
> > Liming
> > > <gaoliming@byosoft.com.cn>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>
> > > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > Zeng, Star
> > > <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar,
> > Rahul R
> > > <rahul.r.kumar@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix
> > CP
> > > Exception when CET enable
> > >
> > > Hi Liming & Mike & Ray,
> > >
> > > Could you help approve this change for the coming edk2 stable tag?
> > This is
> > > critical bug fix in smm cpu driver to handler the CET check failure,
> > I think we
> > > need this change for the stable tag.
> > >
> > > Thanks,
> > > Jiaxin
> > >
> > > > -----Original Message-----
> > > > From: Laszlo Ersek <lersek@redhat.com>
> > > > Sent: Wednesday, November 8, 2023 2:57 AM
> > > > To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> > > > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > Zeng, Star
> > > > <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar,
> > Rahul
> > > R
> > > > <rahul.r.kumar@intel.com>
> > > > Subject: Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm:
> > Fix
> > > CP
> > > > Exception when CET enable
> > > >
> > > > On 11/7/23 02:24, Wu, Jiaxin wrote:
> > > > > Root cause:
> > > > > 1. Before DisableReadonlyPageWriteProtect() is called, the
> > return
> > > > > address (#1) is pushed in shadow stack.
> > > > > 2. CET is disabled.
> > > > > 3. DisableReadonlyPageWriteProtect() returns to #1.
> > > > > 4. Page table is modified.
> > > > > 5. EnableReadonlyPageWriteProtect() is called, but the return
> > > > > address (#2) is not pushed in shadow stack.
> > > > > 6. CET is enabled.
> > > > > 7. EnableReadonlyPageWriteProtect() returns to #2.
> > > > > #CP exception happens because the actual return address (#2)
> > > > > doesn't match the return address stored in shadow stack (#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.
> > > > >
> > > > > 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(-)
> > > >
> > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111022): https://edk2.groups.io/g/devel/message/111022
Mute This Topic: https://groups.io/mt/102434876/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
[-- Attachment #2: Type: text/html, Size: 12288 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-11-10 8:36 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-07 1:24 [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable Wu, Jiaxin
2023-11-07 18:56 ` Laszlo Ersek
2023-11-08 1:17 ` Wu, Jiaxin
2023-11-10 0:01 ` Wu, Jiaxin
2023-11-10 0:25 ` Michael D Kinney
2023-11-10 0:43 ` Wu, Jiaxin
2023-11-10 8:36 ` Ni, Ray
2023-11-09 3:45 ` Ni, Ray
2023-11-09 5:06 ` Dong, Eric
[not found] <179532CD4E894831.20624@groups.io>
2023-11-07 12:01 ` Wu, Jiaxin
2023-11-07 13:08 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox