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.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Zeng Star <star.z...@intel.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> Cc: Rahul Kumar <rahul1.ku...@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
