UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 46 ++++++++--- UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 96 +++++++++++----------- UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 13 ++- 3 files changed, 94 insertions(+), 61 deletions(-)
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.