UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 10 +- UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 104 ++++++++++++++------- UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 19 +++- 3 files changed, 91 insertions(+), 42 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, CET feature enable & disable must be in the
same function to avoid stack mismatch issue.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 10 +-
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 104 ++++++++++++++-------
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 19 +++-
3 files changed, 91 insertions(+), 42 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 654935dc76..daa843b057 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -1554,26 +1554,24 @@ SmmWaitForApArrival (
/**
Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
@param[out] WpEnabled If Cr0.WP is enabled.
- @param[out] CetEnabled If CET is enabled.
+
**/
VOID
DisableReadOnlyPageWriteProtect (
- OUT BOOLEAN *WpEnabled,
- OUT BOOLEAN *CetEnabled
+ OUT BOOLEAN *WpEnabled
);
/**
Enable Write Protect on pages marked as read-only.
@param[out] WpEnabled If Cr0.WP should be enabled.
- @param[out] CetEnabled If CET should be enabled.
+
**/
VOID
EnableReadOnlyPageWriteProtect (
- BOOLEAN WpEnabled,
- BOOLEAN CetEnabled
+ BOOLEAN WpEnabled
);
#endif
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 6f49866615..2c198a161a 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -42,61 +42,44 @@ BOOLEAN mIsReadOnlyPageTable = FALSE;
/**
Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
@param[out] WpEnabled If Cr0.WP is enabled.
- @param[out] CetEnabled If CET is enabled.
+
**/
VOID
DisableReadOnlyPageWriteProtect (
- OUT BOOLEAN *WpEnabled,
- OUT BOOLEAN *CetEnabled
+ OUT BOOLEAN *WpEnabled
)
{
IA32_CR0 Cr0;
- *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
- Cr0.UintN = AsmReadCr0 ();
- *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
+ Cr0.UintN = AsmReadCr0 ();
+ *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
if (*WpEnabled) {
- if (*CetEnabled) {
- //
- // CET must be disabled if WP is disabled. Disable CET before clearing CR0.WP.
- //
- DisableCet ();
- }
-
Cr0.Bits.WP = 0;
AsmWriteCr0 (Cr0.UintN);
}
}
/**
Enable Write Protect on pages marked as read-only.
@param[out] WpEnabled If Cr0.WP should be enabled.
- @param[out] CetEnabled If CET should be enabled.
+
**/
VOID
EnableReadOnlyPageWriteProtect (
- BOOLEAN WpEnabled,
- BOOLEAN CetEnabled
+ BOOLEAN WpEnabled
)
{
IA32_CR0 Cr0;
if (WpEnabled) {
Cr0.UintN = AsmReadCr0 ();
Cr0.Bits.WP = 1;
AsmWriteCr0 (Cr0.UintN);
-
- if (CetEnabled) {
- //
- // re-enable CET.
- //
- EnableCet ();
- }
}
}
/**
Initialize a buffer pool for page table use only.
@@ -157,13 +140,29 @@ InitializePageTablePool (
//
// If page table memory has been marked as RO, mark the new pool pages as read-only.
//
if (mIsReadOnlyPageTable) {
- DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+ //
+ // CET must be disabled if WP is disabled.
+ //
+ CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
+ if (CetEnabled) {
+ DisableCet ();
+ }
+
+ DisableReadOnlyPageWriteProtect (&WpEnabled);
+
SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
- EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+
+ //
+ // Enable the WP and restore CET to enable
+ //
+ EnableReadOnlyPageWriteProtect (WpEnabled);
+ if (CetEnabled) {
+ EnableCet ();
+ }
}
return TRUE;
}
@@ -1055,11 +1054,19 @@ SetMemMapAttributes (
Status = PageTableParse (PageTable, mPagingMode, Map, &Count);
}
ASSERT_RETURN_ERROR (Status);
- DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+ //
+ // CET must be disabled if WP is disabled.
+ //
+ CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
+ if (CetEnabled) {
+ DisableCet ();
+ }
+
+ DisableReadOnlyPageWriteProtect (&WpEnabled);
MemoryMap = MemoryMapStart;
for (Index = 0; Index < MemoryMapEntryCount; Index++) {
DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx, 0x%x\n", MemoryMap->PhysicalStart, MemoryMap->NumberOfPages));
if (MemoryMap->Type == EfiRuntimeServicesCode) {
@@ -1085,11 +1092,18 @@ SetMemMapAttributes (
);
MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, DescriptorSize);
}
- EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+ //
+ // Enable the WP and restore CET to enable
+ //
+ EnableReadOnlyPageWriteProtect (WpEnabled);
+ if (CetEnabled) {
+ EnableCet ();
+ }
+
FreePool (Map);
PatchSmmSaveStateMap ();
PatchGdtIdtMap ();
@@ -1399,11 +1413,19 @@ SetUefiMemMapAttributes (
PERF_FUNCTION_BEGIN ();
DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n"));
- DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+ //
+ // CET must be disabled if WP is disabled.
+ //
+ CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
+ if (CetEnabled) {
+ DisableCet ();
+ }
+
+ DisableReadOnlyPageWriteProtect (&WpEnabled);
if (mUefiMemoryMap != NULL) {
MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
MemoryMap = mUefiMemoryMap;
for (Index = 0; Index < MemoryMapEntryCount; Index++) {
@@ -1479,11 +1501,17 @@ SetUefiMemMapAttributes (
Entry = NEXT_MEMORY_DESCRIPTOR (Entry, mUefiMemoryAttributesTable->DescriptorSize);
}
}
- EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+ //
+ // Enable the WP and restore CET to enable
+ //
+ EnableReadOnlyPageWriteProtect (WpEnabled);
+ if (CetEnabled) {
+ EnableCet ();
+ }
//
// Do not free mUefiMemoryAttributesTable, it will be checked in IsSmmCommBufferForbiddenAddress().
//
@@ -1881,23 +1909,31 @@ SetPageTableAttributes (
PERF_FUNCTION_BEGIN ();
DEBUG ((DEBUG_INFO, "SetPageTableAttributes\n"));
//
- // Disable write protection, because we need mark page table to be write protected.
- // We need *write* page table memory, to mark itself to be *read only*.
+ // CET must be disabled if WP is disabled.
//
- DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+ CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
+ if (CetEnabled) {
+ DisableCet ();
+ }
+
+ DisableReadOnlyPageWriteProtect (&WpEnabled);
// Set memory used by page table as Read Only.
DEBUG ((DEBUG_INFO, "Start...\n"));
EnablePageTableProtection ();
//
- // Enable write protection, after page table attribute updated.
+ // Enable the WP and restore CET to enable
//
- EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
+ EnableReadOnlyPageWriteProtect (WpEnabled);
+ if (CetEnabled) {
+ EnableCet ();
+ }
+
mIsReadOnlyPageTable = TRUE;
//
// Flush TLB after mark all page table pool as read only.
//
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
index 7ac3c66f91..71fdf5f34a 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -604,11 +604,20 @@ InitPaging (
Limit = BASE_4GB;
} else {
Limit = (IsRestrictedMemoryAccess ()) ? LShiftU64 (1, mPhysicalAddressBits) : BASE_4GB;
}
- DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
+ //
+ // CET must be disabled if WP is disabled.
+ //
+ CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
+ if (CetEnabled) {
+ DisableCet ();
+ }
+
+ DisableReadOnlyPageWriteProtect (&WpEnabled);
+
//
// [0, 4k] may be non-present.
//
PreviousAddress = ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0) ? BASE_4KB : 0;
@@ -670,11 +679,17 @@ InitPaging (
//
Status = ConvertMemoryPageAttributes (PageTable, mPagingMode, PreviousAddress, Limit - PreviousAddress, MemoryAttrMask, TRUE, NULL);
ASSERT_RETURN_ERROR (Status);
}
- EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
+ //
+ // Enable the WP and restore CET to enable
+ //
+ EnableReadOnlyPageWriteProtect (WpEnabled);
+ if (CetEnabled) {
+ EnableCet ();
+ }
//
// Flush TLB
//
CpuFlushTlb ();
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110619): https://edk2.groups.io/g/devel/message/110619
Mute This Topic: https://groups.io/mt/102362300/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-On 11/3/23 13:14, Wu, Jiaxin 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, CET feature enable & disable must be in the
> same function to avoid stack mismatch issue.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 10 +-
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 104 ++++++++++++++-------
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 19 +++-
> 3 files changed, 91 insertions(+), 42 deletions(-)
I have two comments:
(1) both the pre-patch code and the post-patch code have several
instances of the following pattern:
Boolean = (Expression != 0) ? TRUE : FALSE;
This is an anti-pattern. It should only be:
Boolean = Expression != 0;
or if you prefer the parentheses, then
Boolean = (Expression != 0);
I recommend cleaning up all instances of this, independently of the
actual issue.
(2) The critical information is in the last paragraph of the commit
message ("CET feature enable & disable must be in the same function to
avoid stack mismatch"); however, that critical information is not
visible anywhere in the new code. People will not understand why the
code is littered with EnableCet / DisableCet calls.
In fact, I only realized the weight of the commit message after I first
looked at the patch, and deduced that the patch did, functionally
speaking, nothing at all!
So here's what I recommend: please replace the functions
- EnableReadOnlyPageWriteProtect()
- DisableReadOnlyPageWriteProtect()
with *macros*
- WRITE_PROTECT_RO_PAGES()
- WRITE_UNPROTECT_RO_PAGES()
These macros should continue taking two parameters (Wp and Cet). The WP
manipulation can be factored out to helper functions if necessary, but
the CET manipulation needs to be expanded inline.
(I've also renamed the APIs because the current API names are awkward.
"Enable Write Protection" is just better expressed as "Write Protect",
and "Disable Write Protection" is just better expressed as "Write
Unprotect", in my opinion.)
And then, comments on the macro definitions should explain that these
pieces of logic are defined as macros and not functions because "CET
feature enable & disable must be in the same function to avoid stack
mismatch".
Thanks!
Laszlo
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 654935dc76..daa843b057 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1554,26 +1554,24 @@ SmmWaitForApArrival (
>
> /**
> Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
>
> @param[out] WpEnabled If Cr0.WP is enabled.
> - @param[out] CetEnabled If CET is enabled.
> +
> **/
> VOID
> DisableReadOnlyPageWriteProtect (
> - OUT BOOLEAN *WpEnabled,
> - OUT BOOLEAN *CetEnabled
> + OUT BOOLEAN *WpEnabled
> );
>
> /**
> Enable Write Protect on pages marked as read-only.
>
> @param[out] WpEnabled If Cr0.WP should be enabled.
> - @param[out] CetEnabled If CET should be enabled.
> +
> **/
> VOID
> EnableReadOnlyPageWriteProtect (
> - BOOLEAN WpEnabled,
> - BOOLEAN CetEnabled
> + BOOLEAN WpEnabled
> );
>
> #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 6f49866615..2c198a161a 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -42,61 +42,44 @@ BOOLEAN mIsReadOnlyPageTable = FALSE;
>
> /**
> Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
>
> @param[out] WpEnabled If Cr0.WP is enabled.
> - @param[out] CetEnabled If CET is enabled.
> +
> **/
> VOID
> DisableReadOnlyPageWriteProtect (
> - OUT BOOLEAN *WpEnabled,
> - OUT BOOLEAN *CetEnabled
> + OUT BOOLEAN *WpEnabled
> )
> {
> IA32_CR0 Cr0;
>
> - *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> - Cr0.UintN = AsmReadCr0 ();
> - *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> + Cr0.UintN = AsmReadCr0 ();
> + *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> if (*WpEnabled) {
> - if (*CetEnabled) {
> - //
> - // CET must be disabled if WP is disabled. Disable CET before clearing CR0.WP.
> - //
> - DisableCet ();
> - }
> -
> Cr0.Bits.WP = 0;
> AsmWriteCr0 (Cr0.UintN);
> }
> }
>
> /**
> Enable Write Protect on pages marked as read-only.
>
> @param[out] WpEnabled If Cr0.WP should be enabled.
> - @param[out] CetEnabled If CET should be enabled.
> +
> **/
> VOID
> EnableReadOnlyPageWriteProtect (
> - BOOLEAN WpEnabled,
> - BOOLEAN CetEnabled
> + BOOLEAN WpEnabled
> )
> {
> IA32_CR0 Cr0;
>
> if (WpEnabled) {
> Cr0.UintN = AsmReadCr0 ();
> Cr0.Bits.WP = 1;
> AsmWriteCr0 (Cr0.UintN);
> -
> - if (CetEnabled) {
> - //
> - // re-enable CET.
> - //
> - EnableCet ();
> - }
> }
> }
>
> /**
> Initialize a buffer pool for page table use only.
> @@ -157,13 +140,29 @@ InitializePageTablePool (
>
> //
> // If page table memory has been marked as RO, mark the new pool pages as read-only.
> //
> if (mIsReadOnlyPageTable) {
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + //
> + // CET must be disabled if WP is disabled.
> + //
> + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> + if (CetEnabled) {
> + DisableCet ();
> + }
> +
> + DisableReadOnlyPageWriteProtect (&WpEnabled);
> +
> SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> +
> + //
> + // Enable the WP and restore CET to enable
> + //
> + EnableReadOnlyPageWriteProtect (WpEnabled);
> + if (CetEnabled) {
> + EnableCet ();
> + }
> }
>
> return TRUE;
> }
>
> @@ -1055,11 +1054,19 @@ SetMemMapAttributes (
> Status = PageTableParse (PageTable, mPagingMode, Map, &Count);
> }
>
> ASSERT_RETURN_ERROR (Status);
>
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + //
> + // CET must be disabled if WP is disabled.
> + //
> + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> + if (CetEnabled) {
> + DisableCet ();
> + }
> +
> + DisableReadOnlyPageWriteProtect (&WpEnabled);
>
> MemoryMap = MemoryMapStart;
> for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx, 0x%x\n", MemoryMap->PhysicalStart, MemoryMap->NumberOfPages));
> if (MemoryMap->Type == EfiRuntimeServicesCode) {
> @@ -1085,11 +1092,18 @@ SetMemMapAttributes (
> );
>
> MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, DescriptorSize);
> }
>
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> + //
> + // Enable the WP and restore CET to enable
> + //
> + EnableReadOnlyPageWriteProtect (WpEnabled);
> + if (CetEnabled) {
> + EnableCet ();
> + }
> +
> FreePool (Map);
>
> PatchSmmSaveStateMap ();
> PatchGdtIdtMap ();
>
> @@ -1399,11 +1413,19 @@ SetUefiMemMapAttributes (
>
> PERF_FUNCTION_BEGIN ();
>
> DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n"));
>
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + //
> + // CET must be disabled if WP is disabled.
> + //
> + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> + if (CetEnabled) {
> + DisableCet ();
> + }
> +
> + DisableReadOnlyPageWriteProtect (&WpEnabled);
>
> if (mUefiMemoryMap != NULL) {
> MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
> MemoryMap = mUefiMemoryMap;
> for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> @@ -1479,11 +1501,17 @@ SetUefiMemMapAttributes (
>
> Entry = NEXT_MEMORY_DESCRIPTOR (Entry, mUefiMemoryAttributesTable->DescriptorSize);
> }
> }
>
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> + //
> + // Enable the WP and restore CET to enable
> + //
> + EnableReadOnlyPageWriteProtect (WpEnabled);
> + if (CetEnabled) {
> + EnableCet ();
> + }
>
> //
> // Do not free mUefiMemoryAttributesTable, it will be checked in IsSmmCommBufferForbiddenAddress().
> //
>
> @@ -1881,23 +1909,31 @@ SetPageTableAttributes (
>
> PERF_FUNCTION_BEGIN ();
> DEBUG ((DEBUG_INFO, "SetPageTableAttributes\n"));
>
> //
> - // Disable write protection, because we need mark page table to be write protected.
> - // We need *write* page table memory, to mark itself to be *read only*.
> + // CET must be disabled if WP is disabled.
> //
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> + if (CetEnabled) {
> + DisableCet ();
> + }
> +
> + DisableReadOnlyPageWriteProtect (&WpEnabled);
>
> // Set memory used by page table as Read Only.
> DEBUG ((DEBUG_INFO, "Start...\n"));
> EnablePageTableProtection ();
>
> //
> - // Enable write protection, after page table attribute updated.
> + // Enable the WP and restore CET to enable
> //
> - EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
> + EnableReadOnlyPageWriteProtect (WpEnabled);
> + if (CetEnabled) {
> + EnableCet ();
> + }
> +
> mIsReadOnlyPageTable = TRUE;
>
> //
> // Flush TLB after mark all page table pool as read only.
> //
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> index 7ac3c66f91..71fdf5f34a 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> @@ -604,11 +604,20 @@ InitPaging (
> Limit = BASE_4GB;
> } else {
> Limit = (IsRestrictedMemoryAccess ()) ? LShiftU64 (1, mPhysicalAddressBits) : BASE_4GB;
> }
>
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + //
> + // CET must be disabled if WP is disabled.
> + //
> + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> + if (CetEnabled) {
> + DisableCet ();
> + }
> +
> + DisableReadOnlyPageWriteProtect (&WpEnabled);
> +
> //
> // [0, 4k] may be non-present.
> //
> PreviousAddress = ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0) ? BASE_4KB : 0;
>
> @@ -670,11 +679,17 @@ InitPaging (
> //
> Status = ConvertMemoryPageAttributes (PageTable, mPagingMode, PreviousAddress, Limit - PreviousAddress, MemoryAttrMask, TRUE, NULL);
> ASSERT_RETURN_ERROR (Status);
> }
>
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> + //
> + // Enable the WP and restore CET to enable
> + //
> + EnableReadOnlyPageWriteProtect (WpEnabled);
> + if (CetEnabled) {
> + EnableCet ();
> + }
>
> //
> // Flush TLB
> //
> CpuFlushTlb ();
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110628): https://edk2.groups.io/g/devel/message/110628
Mute This Topic: https://groups.io/mt/102362300/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Laszlo,
Thanks comments.
>
> I have two comments:
>
>
> (1) both the pre-patch code and the post-patch code have several
> instances of the following pattern:
>
> Boolean = (Expression != 0) ? TRUE : FALSE;
>
> This is an anti-pattern. It should only be:
>
> Boolean = Expression != 0;
>
> or if you prefer the parentheses, then
>
> Boolean = (Expression != 0);
>
> I recommend cleaning up all instances of this, independently of the
> actual issue.
>
Agree, I will clean it in next version.
>
> (2) The critical information is in the last paragraph of the commit
> message ("CET feature enable & disable must be in the same function to
> avoid stack mismatch"); however, that critical information is not
> visible anywhere in the new code. People will not understand why the
> code is littered with EnableCet / DisableCet calls.
>
> In fact, I only realized the weight of the commit message after I first
> looked at the patch, and deduced that the patch did, functionally
> speaking, nothing at all!
>
> So here's what I recommend: please replace the functions
>
> - EnableReadOnlyPageWriteProtect()
> - DisableReadOnlyPageWriteProtect()
>
> with *macros*
>
> - WRITE_PROTECT_RO_PAGES()
> - WRITE_UNPROTECT_RO_PAGES()
>
ok, agree, I will refine the patch with those macros definition.
> These macros should continue taking two parameters (Wp and Cet). The WP
> manipulation can be factored out to helper functions if necessary, but
> the CET manipulation needs to be expanded inline.
>
> (I've also renamed the APIs because the current API names are awkward.
> "Enable Write Protection" is just better expressed as "Write Protect",
> and "Disable Write Protection" is just better expressed as "Write
> Unprotect", in my opinion.)
>
yes. I will rename by following the suggestion.
> And then, comments on the macro definitions should explain that these
> pieces of logic are defined as macros and not functions because "CET
> feature enable & disable must be in the same function to avoid stack
> mismatch".
>
> Thanks!
> Laszlo
>
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > index 654935dc76..daa843b057 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > @@ -1554,26 +1554,24 @@ SmmWaitForApArrival (
> >
> > /**
> > Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> >
> > @param[out] WpEnabled If Cr0.WP is enabled.
> > - @param[out] CetEnabled If CET is enabled.
> > +
> > **/
> > VOID
> > DisableReadOnlyPageWriteProtect (
> > - OUT BOOLEAN *WpEnabled,
> > - OUT BOOLEAN *CetEnabled
> > + OUT BOOLEAN *WpEnabled
> > );
> >
> > /**
> > Enable Write Protect on pages marked as read-only.
> >
> > @param[out] WpEnabled If Cr0.WP should be enabled.
> > - @param[out] CetEnabled If CET should be enabled.
> > +
> > **/
> > VOID
> > EnableReadOnlyPageWriteProtect (
> > - BOOLEAN WpEnabled,
> > - BOOLEAN CetEnabled
> > + BOOLEAN WpEnabled
> > );
> >
> > #endif
> > diff --git
> a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > index 6f49866615..2c198a161a 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > @@ -42,61 +42,44 @@ BOOLEAN mIsReadOnlyPageTable = FALSE;
> >
> > /**
> > Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> >
> > @param[out] WpEnabled If Cr0.WP is enabled.
> > - @param[out] CetEnabled If CET is enabled.
> > +
> > **/
> > VOID
> > DisableReadOnlyPageWriteProtect (
> > - OUT BOOLEAN *WpEnabled,
> > - OUT BOOLEAN *CetEnabled
> > + OUT BOOLEAN *WpEnabled
> > )
> > {
> > IA32_CR0 Cr0;
> >
> > - *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> > - Cr0.UintN = AsmReadCr0 ();
> > - *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> > + Cr0.UintN = AsmReadCr0 ();
> > + *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> > if (*WpEnabled) {
> > - if (*CetEnabled) {
> > - //
> > - // CET must be disabled if WP is disabled. Disable CET before clearing
> CR0.WP.
> > - //
> > - DisableCet ();
> > - }
> > -
> > Cr0.Bits.WP = 0;
> > AsmWriteCr0 (Cr0.UintN);
> > }
> > }
> >
> > /**
> > Enable Write Protect on pages marked as read-only.
> >
> > @param[out] WpEnabled If Cr0.WP should be enabled.
> > - @param[out] CetEnabled If CET should be enabled.
> > +
> > **/
> > VOID
> > EnableReadOnlyPageWriteProtect (
> > - BOOLEAN WpEnabled,
> > - BOOLEAN CetEnabled
> > + BOOLEAN WpEnabled
> > )
> > {
> > IA32_CR0 Cr0;
> >
> > if (WpEnabled) {
> > Cr0.UintN = AsmReadCr0 ();
> > Cr0.Bits.WP = 1;
> > AsmWriteCr0 (Cr0.UintN);
> > -
> > - if (CetEnabled) {
> > - //
> > - // re-enable CET.
> > - //
> > - EnableCet ();
> > - }
> > }
> > }
> >
> > /**
> > Initialize a buffer pool for page table use only.
> > @@ -157,13 +140,29 @@ InitializePageTablePool (
> >
> > //
> > // If page table memory has been marked as RO, mark the new pool pages
> as read-only.
> > //
> > if (mIsReadOnlyPageTable) {
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> > + if (CetEnabled) {
> > + DisableCet ();
> > + }
> > +
> > + DisableReadOnlyPageWriteProtect (&WpEnabled);
> > +
> > SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer,
> EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > +
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + EnableReadOnlyPageWriteProtect (WpEnabled);
> > + if (CetEnabled) {
> > + EnableCet ();
> > + }
> > }
> >
> > return TRUE;
> > }
> >
> > @@ -1055,11 +1054,19 @@ SetMemMapAttributes (
> > Status = PageTableParse (PageTable, mPagingMode, Map, &Count);
> > }
> >
> > ASSERT_RETURN_ERROR (Status);
> >
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> > + if (CetEnabled) {
> > + DisableCet ();
> > + }
> > +
> > + DisableReadOnlyPageWriteProtect (&WpEnabled);
> >
> > MemoryMap = MemoryMapStart;
> > for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> > DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx,
> 0x%x\n", MemoryMap->PhysicalStart, MemoryMap->NumberOfPages));
> > if (MemoryMap->Type == EfiRuntimeServicesCode) {
> > @@ -1085,11 +1092,18 @@ SetMemMapAttributes (
> > );
> >
> > MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap,
> DescriptorSize);
> > }
> >
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + EnableReadOnlyPageWriteProtect (WpEnabled);
> > + if (CetEnabled) {
> > + EnableCet ();
> > + }
> > +
> > FreePool (Map);
> >
> > PatchSmmSaveStateMap ();
> > PatchGdtIdtMap ();
> >
> > @@ -1399,11 +1413,19 @@ SetUefiMemMapAttributes (
> >
> > PERF_FUNCTION_BEGIN ();
> >
> > DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n"));
> >
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> > + if (CetEnabled) {
> > + DisableCet ();
> > + }
> > +
> > + DisableReadOnlyPageWriteProtect (&WpEnabled);
> >
> > if (mUefiMemoryMap != NULL) {
> > MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
> > MemoryMap = mUefiMemoryMap;
> > for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> > @@ -1479,11 +1501,17 @@ SetUefiMemMapAttributes (
> >
> > Entry = NEXT_MEMORY_DESCRIPTOR (Entry,
> mUefiMemoryAttributesTable->DescriptorSize);
> > }
> > }
> >
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + EnableReadOnlyPageWriteProtect (WpEnabled);
> > + if (CetEnabled) {
> > + EnableCet ();
> > + }
> >
> > //
> > // Do not free mUefiMemoryAttributesTable, it will be checked in
> IsSmmCommBufferForbiddenAddress().
> > //
> >
> > @@ -1881,23 +1909,31 @@ SetPageTableAttributes (
> >
> > PERF_FUNCTION_BEGIN ();
> > DEBUG ((DEBUG_INFO, "SetPageTableAttributes\n"));
> >
> > //
> > - // Disable write protection, because we need mark page table to be write
> protected.
> > - // We need *write* page table memory, to mark itself to be *read only*.
> > + // CET must be disabled if WP is disabled.
> > //
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> > + if (CetEnabled) {
> > + DisableCet ();
> > + }
> > +
> > + DisableReadOnlyPageWriteProtect (&WpEnabled);
> >
> > // Set memory used by page table as Read Only.
> > DEBUG ((DEBUG_INFO, "Start...\n"));
> > EnablePageTableProtection ();
> >
> > //
> > - // Enable write protection, after page table attribute updated.
> > + // Enable the WP and restore CET to enable
> > //
> > - EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
> > + EnableReadOnlyPageWriteProtect (WpEnabled);
> > + if (CetEnabled) {
> > + EnableCet ();
> > + }
> > +
> > mIsReadOnlyPageTable = TRUE;
> >
> > //
> > // Flush TLB after mark all page table pool as read only.
> > //
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> > index 7ac3c66f91..71fdf5f34a 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> > @@ -604,11 +604,20 @@ InitPaging (
> > Limit = BASE_4GB;
> > } else {
> > Limit = (IsRestrictedMemoryAccess ()) ? LShiftU64 (1,
> mPhysicalAddressBits) : BASE_4GB;
> > }
> >
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> > + if (CetEnabled) {
> > + DisableCet ();
> > + }
> > +
> > + DisableReadOnlyPageWriteProtect (&WpEnabled);
> > +
> > //
> > // [0, 4k] may be non-present.
> > //
> > PreviousAddress = ((PcdGet8 (PcdNullPointerDetectionPropertyMask) &
> BIT1) != 0) ? BASE_4KB : 0;
> >
> > @@ -670,11 +679,17 @@ InitPaging (
> > //
> > Status = ConvertMemoryPageAttributes (PageTable, mPagingMode,
> PreviousAddress, Limit - PreviousAddress, MemoryAttrMask, TRUE, NULL);
> > ASSERT_RETURN_ERROR (Status);
> > }
> >
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + EnableReadOnlyPageWriteProtect (WpEnabled);
> > + if (CetEnabled) {
> > + EnableCet ();
> > + }
> >
> > //
> > // Flush TLB
> > //
> > CpuFlushTlb ();
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110665): https://edk2.groups.io/g/devel/message/110665
Mute This Topic: https://groups.io/mt/102362300/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 11/3/23 13:14, Wu, Jiaxin 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, CET feature enable & disable must be in the
> same function to avoid stack mismatch issue.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 10 +-
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 104 ++++++++++++++-------
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 19 +++-
> 3 files changed, 91 insertions(+), 42 deletions(-)
Is this somehow related to
[edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before restoring MSR IA32_S_CET
at
https://edk2.groups.io/g/devel/message/110605
?
I'm not familiar with control flow integrity, but both patches seem to fix up problems with CET management. Therefore I would suggest to join forces and include all the patches in the same series. (Not same "patch", mind you -- different patches in the same series.) We've already asked for that other patch to be split up into series, anyway.
Thanks!
Laszlo
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 654935dc76..daa843b057 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1554,26 +1554,24 @@ SmmWaitForApArrival (
>
> /**
> Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
>
> @param[out] WpEnabled If Cr0.WP is enabled.
> - @param[out] CetEnabled If CET is enabled.
> +
> **/
> VOID
> DisableReadOnlyPageWriteProtect (
> - OUT BOOLEAN *WpEnabled,
> - OUT BOOLEAN *CetEnabled
> + OUT BOOLEAN *WpEnabled
> );
>
> /**
> Enable Write Protect on pages marked as read-only.
>
> @param[out] WpEnabled If Cr0.WP should be enabled.
> - @param[out] CetEnabled If CET should be enabled.
> +
> **/
> VOID
> EnableReadOnlyPageWriteProtect (
> - BOOLEAN WpEnabled,
> - BOOLEAN CetEnabled
> + BOOLEAN WpEnabled
> );
>
> #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 6f49866615..2c198a161a 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -42,61 +42,44 @@ BOOLEAN mIsReadOnlyPageTable = FALSE;
>
> /**
> Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
>
> @param[out] WpEnabled If Cr0.WP is enabled.
> - @param[out] CetEnabled If CET is enabled.
> +
> **/
> VOID
> DisableReadOnlyPageWriteProtect (
> - OUT BOOLEAN *WpEnabled,
> - OUT BOOLEAN *CetEnabled
> + OUT BOOLEAN *WpEnabled
> )
> {
> IA32_CR0 Cr0;
>
> - *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> - Cr0.UintN = AsmReadCr0 ();
> - *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> + Cr0.UintN = AsmReadCr0 ();
> + *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> if (*WpEnabled) {
> - if (*CetEnabled) {
> - //
> - // CET must be disabled if WP is disabled. Disable CET before clearing CR0.WP.
> - //
> - DisableCet ();
> - }
> -
> Cr0.Bits.WP = 0;
> AsmWriteCr0 (Cr0.UintN);
> }
> }
>
> /**
> Enable Write Protect on pages marked as read-only.
>
> @param[out] WpEnabled If Cr0.WP should be enabled.
> - @param[out] CetEnabled If CET should be enabled.
> +
> **/
> VOID
> EnableReadOnlyPageWriteProtect (
> - BOOLEAN WpEnabled,
> - BOOLEAN CetEnabled
> + BOOLEAN WpEnabled
> )
> {
> IA32_CR0 Cr0;
>
> if (WpEnabled) {
> Cr0.UintN = AsmReadCr0 ();
> Cr0.Bits.WP = 1;
> AsmWriteCr0 (Cr0.UintN);
> -
> - if (CetEnabled) {
> - //
> - // re-enable CET.
> - //
> - EnableCet ();
> - }
> }
> }
>
> /**
> Initialize a buffer pool for page table use only.
> @@ -157,13 +140,29 @@ InitializePageTablePool (
>
> //
> // If page table memory has been marked as RO, mark the new pool pages as read-only.
> //
> if (mIsReadOnlyPageTable) {
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + //
> + // CET must be disabled if WP is disabled.
> + //
> + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> + if (CetEnabled) {
> + DisableCet ();
> + }
> +
> + DisableReadOnlyPageWriteProtect (&WpEnabled);
> +
> SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> +
> + //
> + // Enable the WP and restore CET to enable
> + //
> + EnableReadOnlyPageWriteProtect (WpEnabled);
> + if (CetEnabled) {
> + EnableCet ();
> + }
> }
>
> return TRUE;
> }
>
> @@ -1055,11 +1054,19 @@ SetMemMapAttributes (
> Status = PageTableParse (PageTable, mPagingMode, Map, &Count);
> }
>
> ASSERT_RETURN_ERROR (Status);
>
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + //
> + // CET must be disabled if WP is disabled.
> + //
> + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> + if (CetEnabled) {
> + DisableCet ();
> + }
> +
> + DisableReadOnlyPageWriteProtect (&WpEnabled);
>
> MemoryMap = MemoryMapStart;
> for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx, 0x%x\n", MemoryMap->PhysicalStart, MemoryMap->NumberOfPages));
> if (MemoryMap->Type == EfiRuntimeServicesCode) {
> @@ -1085,11 +1092,18 @@ SetMemMapAttributes (
> );
>
> MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, DescriptorSize);
> }
>
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> + //
> + // Enable the WP and restore CET to enable
> + //
> + EnableReadOnlyPageWriteProtect (WpEnabled);
> + if (CetEnabled) {
> + EnableCet ();
> + }
> +
> FreePool (Map);
>
> PatchSmmSaveStateMap ();
> PatchGdtIdtMap ();
>
> @@ -1399,11 +1413,19 @@ SetUefiMemMapAttributes (
>
> PERF_FUNCTION_BEGIN ();
>
> DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n"));
>
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + //
> + // CET must be disabled if WP is disabled.
> + //
> + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> + if (CetEnabled) {
> + DisableCet ();
> + }
> +
> + DisableReadOnlyPageWriteProtect (&WpEnabled);
>
> if (mUefiMemoryMap != NULL) {
> MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
> MemoryMap = mUefiMemoryMap;
> for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> @@ -1479,11 +1501,17 @@ SetUefiMemMapAttributes (
>
> Entry = NEXT_MEMORY_DESCRIPTOR (Entry, mUefiMemoryAttributesTable->DescriptorSize);
> }
> }
>
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> + //
> + // Enable the WP and restore CET to enable
> + //
> + EnableReadOnlyPageWriteProtect (WpEnabled);
> + if (CetEnabled) {
> + EnableCet ();
> + }
>
> //
> // Do not free mUefiMemoryAttributesTable, it will be checked in IsSmmCommBufferForbiddenAddress().
> //
>
> @@ -1881,23 +1909,31 @@ SetPageTableAttributes (
>
> PERF_FUNCTION_BEGIN ();
> DEBUG ((DEBUG_INFO, "SetPageTableAttributes\n"));
>
> //
> - // Disable write protection, because we need mark page table to be write protected.
> - // We need *write* page table memory, to mark itself to be *read only*.
> + // CET must be disabled if WP is disabled.
> //
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> + if (CetEnabled) {
> + DisableCet ();
> + }
> +
> + DisableReadOnlyPageWriteProtect (&WpEnabled);
>
> // Set memory used by page table as Read Only.
> DEBUG ((DEBUG_INFO, "Start...\n"));
> EnablePageTableProtection ();
>
> //
> - // Enable write protection, after page table attribute updated.
> + // Enable the WP and restore CET to enable
> //
> - EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
> + EnableReadOnlyPageWriteProtect (WpEnabled);
> + if (CetEnabled) {
> + EnableCet ();
> + }
> +
> mIsReadOnlyPageTable = TRUE;
>
> //
> // Flush TLB after mark all page table pool as read only.
> //
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> index 7ac3c66f91..71fdf5f34a 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> @@ -604,11 +604,20 @@ InitPaging (
> Limit = BASE_4GB;
> } else {
> Limit = (IsRestrictedMemoryAccess ()) ? LShiftU64 (1, mPhysicalAddressBits) : BASE_4GB;
> }
>
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + //
> + // CET must be disabled if WP is disabled.
> + //
> + CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> + if (CetEnabled) {
> + DisableCet ();
> + }
> +
> + DisableReadOnlyPageWriteProtect (&WpEnabled);
> +
> //
> // [0, 4k] may be non-present.
> //
> PreviousAddress = ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0) ? BASE_4KB : 0;
>
> @@ -670,11 +679,17 @@ InitPaging (
> //
> Status = ConvertMemoryPageAttributes (PageTable, mPagingMode, PreviousAddress, Limit - PreviousAddress, MemoryAttrMask, TRUE, NULL);
> ASSERT_RETURN_ERROR (Status);
> }
>
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> + //
> + // Enable the WP and restore CET to enable
> + //
> + EnableReadOnlyPageWriteProtect (WpEnabled);
> + if (CetEnabled) {
> + EnableCet ();
> + }
>
> //
> // Flush TLB
> //
> CpuFlushTlb ();
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110623): https://edk2.groups.io/g/devel/message/110623
Mute This Topic: https://groups.io/mt/102362300/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Laszlo, Thanks the feedback. > > Is this somehow related to > > [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before > restoring MSR IA32_S_CET > > at > > https://edk2.groups.io/g/devel/message/110605 > > ? > > I'm not familiar with control flow integrity, but both patches seem to fix up > problems with CET management. Therefore I would suggest to join forces and > include all the patches in the same series. (Not same "patch", mind you -- > different patches in the same series.) We've already asked for that other patch > to be split up into series, anyway. > I think it's total different issue. I found the system hang once we are trying enable the CET. And root caused to the stack mismatch. Actually, the issue is not the CET itself, but due to the wrong use of CET enable/disable by consumer (smm cpu driver). SMM CPU enable/disable the CET feature improperly. For the patch you mentioned, it looks to handle the CET itself init bug, right? if so, do you think we can keep to handle it separated or you still think we need join forces in same series? Thanks, Jiaxin ] > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110625): https://edk2.groups.io/g/devel/message/110625 Mute This Topic: https://groups.io/mt/102362300/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 11/3/23 15:10, Wu, Jiaxin wrote: > Hi Laszlo, > > Thanks the feedback. > >> >> Is this somehow related to >> >> [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR4.CET before >> restoring MSR IA32_S_CET >> >> at >> >> https://edk2.groups.io/g/devel/message/110605 >> >> ? >> >> I'm not familiar with control flow integrity, but both patches seem to fix up >> problems with CET management. Therefore I would suggest to join forces and >> include all the patches in the same series. (Not same "patch", mind you -- >> different patches in the same series.) We've already asked for that other patch >> to be split up into series, anyway. >> > > I think it's total different issue. I found the system hang once we are trying enable the CET. And root caused to the stack mismatch. > > Actually, the issue is not the CET itself, but due to the wrong use of CET enable/disable by consumer (smm cpu driver). SMM CPU enable/disable the CET feature improperly. > > For the patch you mentioned, it looks to handle the CET itself init bug, right? if so, do you think we can keep to handle it separated or you still think we need join forces in same series? Thanks for the explanation; that clarifies that my suggestion to include both patches in the same series was wrong. So please feel free to proceed separately. Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110626): https://edk2.groups.io/g/devel/message/110626 Mute This Topic: https://groups.io/mt/102362300/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.