* [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
@ 2023-11-03 18:37 Wu, Jiaxin
2023-11-05 11:00 ` Laszlo Ersek
0 siblings, 1 reply; 3+ messages in thread
From: Wu, Jiaxin @ 2023-11-03 18:37 UTC (permalink / raw)
To: devel
Cc: Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann, Rahul Kumar,
Laszlo Ersek
Shadow stack will stop update after CET disable (DisableCet in
DisableReadOnlyPageWriteProtect), but normal smi stack will be
continue updated with the function return and enter
(DisableReadOnlyPageWriteProtect & EnableReadOnlyPageWriteProtect),
thus leading stack mismatch after CET re-enabled (EnableCet in
EnableReadOnlyPageWriteProtect).
Normal smi stack and shadow stack must be matched when CET enable,
otherwise CP Exception will happen, which is caused by a near RET
instruction (See SDM Vol 3, 6.15-Control Protection Exception).
With above requirement, define below 2 macros instead of functions
for WP & CET operation:
WRITE_UNPROTECT_RO_PAGES (Wp, Cet)
WRITE_PROTECT_RO_PAGES (Wp, Cet)
Because "CET" feature disable & enable must be in the same
function to avoid shadow stack and normal SMI stack mismatch.
Note: WRITE_UNPROTECT_RO_PAGES () must be called pair with
WRITE_PROTECT_RO_PAGES () in same function.
Change-Id: I4e126697efcd8dbfb4887da034d8691bfca969e3
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 | 46 ++++++++---
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 96 +++++++++++-----------
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 13 ++-
3 files changed, 94 insertions(+), 61 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 654935dc76..5d167899ff 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -1551,29 +1551,51 @@ 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.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[out] WriteProtect If Cr0.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
);
+///
+/// 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.
+///
+#define WRITE_UNPROTECT_RO_PAGES(Wp, Cet) \
+{ \
+ Cet = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0); \
+ if (Cet) { \
+ DisableCet (); \
+ } \
+ SmmWriteUnprotectReadOnlyPage(&Wp); \
+}
+
+#define WRITE_PROTECT_RO_PAGES(Wp, Cet) \
+{ \
+ SmmWriteProtectReadOnlyPage(Wp); \
+ if (Cet) { \
+ EnableCet (); \
+ } \
+}
+
#endif
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 6f49866615..8edfddd3ea 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.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) ? TRUE : FALSE;
+ if (*WriteProtect) {
Cr0.Bits.WP = 0;
AsmWriteCr0 (Cr0.UintN);
}
}
/**
- Enable Write Protect on pages marked as read-only.
+ Write protect read-only pages.
+
+ @param[out] WriteProtect If Cr0.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,21 @@ InitializePageTablePool (
//
// If page table memory has been marked as RO, mark the new pool pages as read-only.
//
if (mIsReadOnlyPageTable) {
- DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+ //
+ // CET must be disabled if WP is disabled.
+ //
+ WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
+
SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
- EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+
+ //
+ // Enable the WP and restore CET to enable
+ //
+ WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
}
return TRUE;
}
@@ -1009,11 +1000,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 +1046,14 @@ SetMemMapAttributes (
Status = PageTableParse (PageTable, mPagingMode, Map, &Count);
}
ASSERT_RETURN_ERROR (Status);
- DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+ //
+ // CET must be disabled if WP is disabled.
+ //
+ 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 +1079,15 @@ SetMemMapAttributes (
);
MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, DescriptorSize);
}
- EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+ //
+ // Enable the WP and restore CET to enable
+ //
+ WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
+
FreePool (Map);
PatchSmmSaveStateMap ();
PatchGdtIdtMap ();
@@ -1392,18 +1390,21 @@ 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);
+ //
+ // CET must be disabled if WP is disabled.
+ //
+ WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
if (mUefiMemoryMap != NULL) {
MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
MemoryMap = mUefiMemoryMap;
for (Index = 0; Index < MemoryMapEntryCount; Index++) {
@@ -1479,11 +1480,14 @@ SetUefiMemMapAttributes (
Entry = NEXT_MEMORY_DESCRIPTOR (Entry, mUefiMemoryAttributesTable->DescriptorSize);
}
}
- EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+ //
+ // Enable the WP and restore CET to enable
+ //
+ WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
//
// Do not free mUefiMemoryAttributesTable, it will be checked in IsSmmCommBufferForbiddenAddress().
//
@@ -1870,34 +1874,34 @@ IfReadOnlyPageTableNeeded (
VOID
SetPageTableAttributes (
VOID
)
{
- BOOLEAN WpEnabled;
+ BOOLEAN WriteProtect;
BOOLEAN CetEnabled;
if (!IfReadOnlyPageTableNeeded ()) {
return;
}
PERF_FUNCTION_BEGIN ();
DEBUG ((DEBUG_INFO, "SetPageTableAttributes\n"));
//
- // Disable write protection, because we need mark page table to be write protected.
- // We need *write* page table memory, to mark itself to be *read only*.
+ // CET must be disabled if WP is disabled.
//
- DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+ 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.
+ // Enable the WP and restore CET to enable
//
- EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
+ WRITE_PROTECT_RO_PAGES (WriteProtect, 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..8f6a2d440e 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,15 @@ InitPaging (
Limit = BASE_4GB;
} else {
Limit = (IsRestrictedMemoryAccess ()) ? LShiftU64 (1, mPhysicalAddressBits) : BASE_4GB;
}
- DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+ //
+ // CET must be disabled if WP is disabled.
+ //
+ WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
+
//
// [0, 4k] may be non-present.
//
PreviousAddress = ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0) ? BASE_4KB : 0;
@@ -670,11 +674,14 @@ InitPaging (
//
Status = ConvertMemoryPageAttributes (PageTable, mPagingMode, PreviousAddress, Limit - PreviousAddress, MemoryAttrMask, TRUE, NULL);
ASSERT_RETURN_ERROR (Status);
}
- EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+ //
+ // Enable the WP and restore CET to enable
+ //
+ 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 (#110664): https://edk2.groups.io/g/devel/message/110664
Mute This Topic: https://groups.io/mt/102370465/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] 3+ messages in thread
* Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
2023-11-03 18:37 [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable Wu, Jiaxin
@ 2023-11-05 11:00 ` Laszlo Ersek
2023-11-06 2:47 ` Wu, Jiaxin
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2023-11-05 11:00 UTC (permalink / raw)
To: Jiaxin Wu, devel; +Cc: Eric Dong, Ray Ni, Zeng Star, Gerd Hoffmann, Rahul Kumar
Hi Jiaxin,
looks great; now I'm only asking for a few light touch-ups:
On 11/3/23 19:37, Jiaxin Wu wrote:
> Shadow stack will stop update after CET disable (DisableCet in
> DisableReadOnlyPageWriteProtect), but normal smi stack will be
> continue updated with the function return and enter
> (DisableReadOnlyPageWriteProtect & EnableReadOnlyPageWriteProtect),
> thus leading stack mismatch after CET re-enabled (EnableCet in
> EnableReadOnlyPageWriteProtect).
>
> Normal smi stack and shadow stack must be matched when CET enable,
> otherwise CP Exception will happen, which is caused by a near RET
> instruction (See SDM Vol 3, 6.15-Control Protection Exception).
>
> With above requirement, define below 2 macros instead of functions
> for WP & CET operation:
> WRITE_UNPROTECT_RO_PAGES (Wp, Cet)
> WRITE_PROTECT_RO_PAGES (Wp, Cet)
> Because "CET" feature disable & enable must be in the same
> function to avoid shadow stack and normal SMI stack mismatch.
>
> Note: WRITE_UNPROTECT_RO_PAGES () must be called pair with
> WRITE_PROTECT_RO_PAGES () in same function.
>
> Change-Id: I4e126697efcd8dbfb4887da034d8691bfca969e3
(1) Please drop the Change-Id line; it is not meaningful in the upstream
repo.
> 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 | 46 ++++++++---
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 96 +++++++++++-----------
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 13 ++-
> 3 files changed, 94 insertions(+), 61 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 654935dc76..5d167899ff 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1551,29 +1551,51 @@ 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.WP is enabled.
(2) The comment references to the WP bit are not consistent. We should
either stick with Cr0.WP or Cr0.Bits.WP, but not mix them.
I understand this inconsistency exists pre-patch, but because we're
modifying the same sentences, I think it would be OK to clean up the WP
bit references as well, at the same time.
>
> - @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[out] WriteProtect If Cr0.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
> );
(3) If, under (2), you opt for preserving "Cr0.Bits.WP", then please use
that term here too.
>
> +///
> +/// 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.
> +///
> +#define WRITE_UNPROTECT_RO_PAGES(Wp, Cet) \
> +{ \
> + Cet = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0); \
> + if (Cet) { \
> + DisableCet (); \
> + } \
> + SmmWriteUnprotectReadOnlyPage(&Wp); \
> +}
> +
> +#define WRITE_PROTECT_RO_PAGES(Wp, Cet) \
> +{ \
> + SmmWriteProtectReadOnlyPage(Wp); \
> + if (Cet) { \
> + EnableCet (); \
> + } \
> +}
> +
> #endif
(4) Assuming the ECC Check Plugin tolerates it, I recommend adding two
entries to the documentation here:
/// @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().
I recommend this because it clarifies that neither Wp nor Cet are
supposed to have side effects (for example, "Cet" is evaluated multiple
times in WRITE_UNPROTECT_RO_PAGES(), so calling the macro with something
like *CetVariable++ would not work well).
(5) A space character is missing right after each of
"SmmWriteUnprotectReadOnlyPage" and "SmmWriteProtectReadOnlyPage",
before the parens.
(6) It would be more idiomatic to #define these macros as:
do { \
... \
} while (FALSE)
because this replacement text is more suitable to be followed by a
semicolon ";".
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 6f49866615..8edfddd3ea 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.WP is enabled.
>
> - @param[out] WpEnabled If Cr0.WP is enabled.
> - @param[out] CetEnabled If CET is enabled.
> **/
> VOID
(7) If you decide to touch up the function comment in the header file,
then please keep this in sync.
> -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) ? TRUE : FALSE;
(8) Can you remove the " ? TRUE : FALSE" part?
> + if (*WriteProtect) {
> Cr0.Bits.WP = 0;
> AsmWriteCr0 (Cr0.UintN);
> }
> }
>
> /**
> - Enable Write Protect on pages marked as read-only.
> + Write protect read-only pages.
> +
> + @param[out] WriteProtect If Cr0.WP should be enabled.
>
> - @param[out] WpEnabled If Cr0.WP should be enabled.
> - @param[out] CetEnabled If CET should be enabled.
> **/
(9) If you decide to touch up the function comment in the header file,
then please keep this in sync.
> 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,21 @@ InitializePageTablePool (
>
> //
> // If page table memory has been marked as RO, mark the new pool pages as read-only.
> //
> if (mIsReadOnlyPageTable) {
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + //
> + // CET must be disabled if WP is disabled.
> + //
> + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
> +
> SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> +
> + //
> + // Enable the WP and restore CET to enable
> + //
> + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> }
>
> return TRUE;
> }
>
(10) Here on the other hand I suggest *removing* these comments. The
macros are really well defined at this point, so I think just invoking
the macros should suffice. Up to you, anyway; if you'd like to keep the
comments, I won't insist. :)
If you decide to remove the comments, then please keep the rest of the
patch (below) consistent as well.
> @@ -1009,11 +1000,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 +1046,14 @@ SetMemMapAttributes (
> Status = PageTableParse (PageTable, mPagingMode, Map, &Count);
> }
>
> ASSERT_RETURN_ERROR (Status);
>
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + //
> + // CET must be disabled if WP is disabled.
> + //
> + 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 +1079,15 @@ SetMemMapAttributes (
> );
>
> MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, DescriptorSize);
> }
>
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> + //
> + // Enable the WP and restore CET to enable
> + //
> + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> +
> FreePool (Map);
>
> PatchSmmSaveStateMap ();
> PatchGdtIdtMap ();
>
> @@ -1392,18 +1390,21 @@ 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);
> + //
> + // CET must be disabled if WP is disabled.
> + //
> + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
>
> if (mUefiMemoryMap != NULL) {
> MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
> MemoryMap = mUefiMemoryMap;
> for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> @@ -1479,11 +1480,14 @@ SetUefiMemMapAttributes (
>
> Entry = NEXT_MEMORY_DESCRIPTOR (Entry, mUefiMemoryAttributesTable->DescriptorSize);
> }
> }
>
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> + //
> + // Enable the WP and restore CET to enable
> + //
> + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
>
> //
> // Do not free mUefiMemoryAttributesTable, it will be checked in IsSmmCommBufferForbiddenAddress().
> //
>
> @@ -1870,34 +1874,34 @@ IfReadOnlyPageTableNeeded (
> VOID
> SetPageTableAttributes (
> VOID
> )
> {
> - BOOLEAN WpEnabled;
> + BOOLEAN WriteProtect;
> BOOLEAN CetEnabled;
>
> if (!IfReadOnlyPageTableNeeded ()) {
> return;
> }
>
> PERF_FUNCTION_BEGIN ();
> DEBUG ((DEBUG_INFO, "SetPageTableAttributes\n"));
>
> //
> - // Disable write protection, because we need mark page table to be write protected.
> - // We need *write* page table memory, to mark itself to be *read only*.
(11) These two comment lines seem important, and not related to the
patch -- they explain *why* we are doing the WP / CET twiddling. So I
recommend keeping these (and only removing the comment line below)!
> + // CET must be disabled if WP is disabled.
> //
> - 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.
> + // Enable the WP and restore CET to enable
(12) same as (11) here.
> //
> - EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
> + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> +
(13) Whoa, the *original* EnableReadOnlyPageWriteProtect() call passes
the TRUE constant, not WpEnabled!
Was that intentional, or an independent oversight?
If it was an independent oversight in the original code, then I agree we
can fix it, and I'm not even asking for a separate patch, but please
mention it in the commit message.
> 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..8f6a2d440e 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,15 @@ InitPaging (
> Limit = BASE_4GB;
> } else {
> Limit = (IsRestrictedMemoryAccess ()) ? LShiftU64 (1, mPhysicalAddressBits) : BASE_4GB;
> }
>
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + //
> + // CET must be disabled if WP is disabled.
> + //
> + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
> +
> //
> // [0, 4k] may be non-present.
> //
> PreviousAddress = ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0) ? BASE_4KB : 0;
>
> @@ -670,11 +674,14 @@ InitPaging (
> //
> Status = ConvertMemoryPageAttributes (PageTable, mPagingMode, PreviousAddress, Limit - PreviousAddress, MemoryAttrMask, TRUE, NULL);
> ASSERT_RETURN_ERROR (Status);
> }
>
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> + //
> + // Enable the WP and restore CET to enable
> + //
> + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
>
> //
> // Flush TLB
> //
> CpuFlushTlb ();
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110680): https://edk2.groups.io/g/devel/message/110680
Mute This Topic: https://groups.io/mt/102370465/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] 3+ messages in thread
* Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable
2023-11-05 11:00 ` Laszlo Ersek
@ 2023-11-06 2:47 ` Wu, Jiaxin
0 siblings, 0 replies; 3+ messages in thread
From: Wu, Jiaxin @ 2023-11-06 2:47 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io
Cc: Dong, Eric, Ni, Ray, Zeng, Star, Gerd Hoffmann, Kumar, Rahul R
Thanks Laszlo, all fixed in version 3.
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Sunday, November 5, 2023 7:01 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; 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>
> Subject: Re: [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception
> when CET enable
>
> Hi Jiaxin,
>
> looks great; now I'm only asking for a few light touch-ups:
>
> On 11/3/23 19:37, Jiaxin Wu wrote:
> > Shadow stack will stop update after CET disable (DisableCet in
> > DisableReadOnlyPageWriteProtect), but normal smi stack will be
> > continue updated with the function return and enter
> > (DisableReadOnlyPageWriteProtect & EnableReadOnlyPageWriteProtect),
> > thus leading stack mismatch after CET re-enabled (EnableCet in
> > EnableReadOnlyPageWriteProtect).
> >
> > Normal smi stack and shadow stack must be matched when CET enable,
> > otherwise CP Exception will happen, which is caused by a near RET
> > instruction (See SDM Vol 3, 6.15-Control Protection Exception).
> >
> > With above requirement, define below 2 macros instead of functions
> > for WP & CET operation:
> > WRITE_UNPROTECT_RO_PAGES (Wp, Cet)
> > WRITE_PROTECT_RO_PAGES (Wp, Cet)
> > Because "CET" feature disable & enable must be in the same
> > function to avoid shadow stack and normal SMI stack mismatch.
> >
> > Note: WRITE_UNPROTECT_RO_PAGES () must be called pair with
> > WRITE_PROTECT_RO_PAGES () in same function.
> >
> > Change-Id: I4e126697efcd8dbfb4887da034d8691bfca969e3
>
> (1) Please drop the Change-Id line; it is not meaningful in the upstream
> repo.
>
> > 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 | 46
> ++++++++---
> > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 96
> +++++++++++-----------
> > UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 13 ++-
> > 3 files changed, 94 insertions(+), 61 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > index 654935dc76..5d167899ff 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > @@ -1551,29 +1551,51 @@ 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.WP is enabled.
>
> (2) The comment references to the WP bit are not consistent. We should
> either stick with Cr0.WP or Cr0.Bits.WP, but not mix them.
>
> I understand this inconsistency exists pre-patch, but because we're
> modifying the same sentences, I think it would be OK to clean up the WP
> bit references as well, at the same time.
>
> >
> > - @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[out] WriteProtect If Cr0.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
> > );
>
> (3) If, under (2), you opt for preserving "Cr0.Bits.WP", then please use
> that term here too.
>
> >
> > +///
> > +/// 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.
> > +///
> > +#define WRITE_UNPROTECT_RO_PAGES(Wp, Cet) \
> > +{ \
> > + Cet = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0); \
> > + if (Cet) { \
> > + DisableCet (); \
> > + } \
> > + SmmWriteUnprotectReadOnlyPage(&Wp); \
> > +}
> > +
> > +#define WRITE_PROTECT_RO_PAGES(Wp, Cet) \
> > +{ \
> > + SmmWriteProtectReadOnlyPage(Wp); \
> > + if (Cet) { \
> > + EnableCet (); \
> > + } \
> > +}
> > +
> > #endif
>
> (4) Assuming the ECC Check Plugin tolerates it, I recommend adding two
> entries to the documentation here:
>
> /// @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().
>
> I recommend this because it clarifies that neither Wp nor Cet are
> supposed to have side effects (for example, "Cet" is evaluated multiple
> times in WRITE_UNPROTECT_RO_PAGES(), so calling the macro with
> something
> like *CetVariable++ would not work well).
>
>
> (5) A space character is missing right after each of
> "SmmWriteUnprotectReadOnlyPage" and "SmmWriteProtectReadOnlyPage",
> before the parens.
>
>
> (6) It would be more idiomatic to #define these macros as:
>
> do { \
> ... \
> } while (FALSE)
>
> because this replacement text is more suitable to be followed by a
> semicolon ";".
>
>
> > diff --git
> a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > index 6f49866615..8edfddd3ea 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.WP is enabled.
> >
> > - @param[out] WpEnabled If Cr0.WP is enabled.
> > - @param[out] CetEnabled If CET is enabled.
> > **/
> > VOID
>
> (7) If you decide to touch up the function comment in the header file,
> then please keep this in sync.
>
>
> > -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) ? TRUE : FALSE;
>
> (8) Can you remove the " ? TRUE : FALSE" part?
>
>
> > + if (*WriteProtect) {
> > Cr0.Bits.WP = 0;
> > AsmWriteCr0 (Cr0.UintN);
> > }
> > }
> >
> > /**
> > - Enable Write Protect on pages marked as read-only.
> > + Write protect read-only pages.
> > +
> > + @param[out] WriteProtect If Cr0.WP should be enabled.
> >
> > - @param[out] WpEnabled If Cr0.WP should be enabled.
> > - @param[out] CetEnabled If CET should be enabled.
> > **/
>
> (9) If you decide to touch up the function comment in the header file,
> then please keep this in sync.
>
> > 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,21 @@ InitializePageTablePool (
> >
> > //
> > // If page table memory has been marked as RO, mark the new pool pages
> as read-only.
> > //
> > if (mIsReadOnlyPageTable) {
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
> > +
> > SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer,
> EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > +
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> > }
> >
> > return TRUE;
> > }
> >
>
> (10) Here on the other hand I suggest *removing* these comments. The
> macros are really well defined at this point, so I think just invoking
> the macros should suffice. Up to you, anyway; if you'd like to keep the
> comments, I won't insist. :)
>
> If you decide to remove the comments, then please keep the rest of the
> patch (below) consistent as well.
>
> > @@ -1009,11 +1000,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 +1046,14 @@ SetMemMapAttributes (
> > Status = PageTableParse (PageTable, mPagingMode, Map, &Count);
> > }
> >
> > ASSERT_RETURN_ERROR (Status);
> >
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + 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 +1079,15 @@ SetMemMapAttributes (
> > );
> >
> > MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap,
> DescriptorSize);
> > }
> >
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> > +
> > FreePool (Map);
> >
> > PatchSmmSaveStateMap ();
> > PatchGdtIdtMap ();
> >
> > @@ -1392,18 +1390,21 @@ 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);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
> >
> > if (mUefiMemoryMap != NULL) {
> > MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
> > MemoryMap = mUefiMemoryMap;
> > for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> > @@ -1479,11 +1480,14 @@ SetUefiMemMapAttributes (
> >
> > Entry = NEXT_MEMORY_DESCRIPTOR (Entry,
> mUefiMemoryAttributesTable->DescriptorSize);
> > }
> > }
> >
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> >
> > //
> > // Do not free mUefiMemoryAttributesTable, it will be checked in
> IsSmmCommBufferForbiddenAddress().
> > //
> >
> > @@ -1870,34 +1874,34 @@ IfReadOnlyPageTableNeeded (
> > VOID
> > SetPageTableAttributes (
> > VOID
> > )
> > {
> > - BOOLEAN WpEnabled;
> > + BOOLEAN WriteProtect;
> > BOOLEAN CetEnabled;
> >
> > if (!IfReadOnlyPageTableNeeded ()) {
> > return;
> > }
> >
> > PERF_FUNCTION_BEGIN ();
> > DEBUG ((DEBUG_INFO, "SetPageTableAttributes\n"));
> >
> > //
> > - // Disable write protection, because we need mark page table to be write
> protected.
> > - // We need *write* page table memory, to mark itself to be *read only*.
>
> (11) These two comment lines seem important, and not related to the
> patch -- they explain *why* we are doing the WP / CET twiddling. So I
> recommend keeping these (and only removing the comment line below)!
>
> > + // CET must be disabled if WP is disabled.
> > //
> > - 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.
> > + // Enable the WP and restore CET to enable
>
> (12) same as (11) here.
>
> > //
> > - EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
> > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> > +
>
> (13) Whoa, the *original* EnableReadOnlyPageWriteProtect() call passes
> the TRUE constant, not WpEnabled!
>
> Was that intentional, or an independent oversight?
>
> If it was an independent oversight in the original code, then I agree we
> can fix it, and I'm not even asking for a separate patch, but please
> mention it in the commit message.
>
> > 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..8f6a2d440e 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,15 @@ InitPaging (
> > Limit = BASE_4GB;
> > } else {
> > Limit = (IsRestrictedMemoryAccess ()) ? LShiftU64 (1,
> mPhysicalAddressBits) : BASE_4GB;
> > }
> >
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
> > +
> > //
> > // [0, 4k] may be non-present.
> > //
> > PreviousAddress = ((PcdGet8 (PcdNullPointerDetectionPropertyMask) &
> BIT1) != 0) ? BASE_4KB : 0;
> >
> > @@ -670,11 +674,14 @@ InitPaging (
> > //
> > Status = ConvertMemoryPageAttributes (PageTable, mPagingMode,
> PreviousAddress, Limit - PreviousAddress, MemoryAttrMask, TRUE, NULL);
> > ASSERT_RETURN_ERROR (Status);
> > }
> >
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> >
> > //
> > // Flush TLB
> > //
> > CpuFlushTlb ();
>
> Thanks!
> Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110696): https://edk2.groups.io/g/devel/message/110696
Mute This Topic: https://groups.io/mt/102370465/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-11-06 2:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-03 18:37 [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable Wu, Jiaxin
2023-11-05 11:00 ` Laszlo Ersek
2023-11-06 2:47 ` Wu, Jiaxin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox