Hi Ray & Laszlo,
Any more comments to this?
Thanks,
Jiaxin
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> Jiaxin
> Sent: Tuesday, November 7, 2023 9:25 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Zeng, Star
> <star.z...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Kumar, Rahul R
> <rahul.r.ku...@intel.com>; Laszlo Ersek <ler...@redhat.com>
> Subject: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP
> Exception when CET enable
>
> Root cause:
> 1. Before DisableReadonlyPageWriteProtect() is called, the return
> address (#1) is pushed in shadow stack.
> 2. CET is disabled.
> 3. DisableReadonlyPageWriteProtect() returns to #1.
> 4. Page table is modified.
> 5. EnableReadonlyPageWriteProtect() is called, but the return
> address (#2) is not pushed in shadow stack.
> 6. CET is enabled.
> 7. EnableReadonlyPageWriteProtect() returns to #2.
> #CP exception happens because the actual return address (#2)
> doesn't match the return address stored in shadow stack (#1).
>
> Analysis:
> Shadow stack will stop update after CET disable (DisableCet() in
> DisableReadOnlyPageWriteProtect), but normal smi stack will be
> continue updated with the function called and return
> (DisableReadOnlyPageWriteProtect & EnableReadOnlyPageWriteProtect),
> thus leading stack mismatch after CET re-enabled (EnableCet() in
> EnableReadOnlyPageWriteProtect).
>
> According SDM Vol 3, 6.15-Control Protection Exception:
> Normal smi stack and shadow stack must be matched when CET enable,
> otherwise CP Exception will happen, which is caused by a near RET
> instruction.
>
> CET is disabled in DisableCet(), while can be enabled in
> EnableCet(). This way won't cause the problem because they are
> implemented in a way that return address of DisableCet() is
> poped out from shadow stack (Incsspq performs a pop to increases
> the shadow stack) and EnableCet() doesn't use "RET" but "JMP" to
> return to caller. So calling EnableCet() and DisableCet() doesn't
> have the same issue as calling DisableReadonlyPageWriteProtect()
> and EnableReadonlyPageWriteProtect().
>
> With above root cause & analysis, define below 2 macros instead of
> functions for WP & CET operation:
> WRITE_UNPROTECT_RO_PAGES (Wp, Cet)
> WRITE_PROTECT_RO_PAGES (Wp, Cet)
> Because DisableCet() & EnableCet() must be in the same function
> to avoid shadow stack and normal SMI stack mismatch.
>
> Note: WRITE_UNPROTECT_RO_PAGES () must be called pair with
> WRITE_PROTECT_RO_PAGES () in same function.
>
> Cc: Eric Dong <eric.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>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Signed-off-by: Jiaxin Wu <jiaxin...@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 59
> +++++++++++++----
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 73
> +++++++++-------------
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 7 ++-
> 3 files changed, 81 insertions(+), 58 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 654935dc76..20ada465c2 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -1551,29 +1551,64 @@ VOID
> SmmWaitForApArrival (
> VOID
> );
>
> /**
> - Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> + Write unprotect read-only pages if Cr0.Bits.WP is 1.
> +
> + @param[out] WriteProtect If Cr0.Bits.WP is enabled.
>
> - @param[out] WpEnabled If Cr0.WP is enabled.
> - @param[out] CetEnabled If CET is enabled.
> **/
> VOID
> -DisableReadOnlyPageWriteProtect (
> - OUT BOOLEAN *WpEnabled,
> - OUT BOOLEAN *CetEnabled
> +SmmWriteUnprotectReadOnlyPage (
> + OUT BOOLEAN *WriteProtect
> );
>
> /**
> - Enable Write Protect on pages marked as read-only.
> + Write protect read-only pages.
> +
> + @param[in] WriteProtect If Cr0.Bits.WP should be enabled.
>
> - @param[out] WpEnabled If Cr0.WP should be enabled.
> - @param[out] CetEnabled If CET should be enabled.
> **/
> VOID
> -EnableReadOnlyPageWriteProtect (
> - BOOLEAN WpEnabled,
> - BOOLEAN CetEnabled
> +SmmWriteProtectReadOnlyPage (
> + IN BOOLEAN WriteProtect
> );
>
> +///
> +/// Define macros to encapsulate the write unprotect/protect
> +/// read-only pages.
> +/// Below pieces of logic are defined as macros and not functions
> +/// because "CET" feature disable & enable must be in the same
> +/// function to avoid shadow stack and normal SMI stack mismatch,
> +/// thus WRITE_UNPROTECT_RO_PAGES () must be called pair with
> +/// WRITE_PROTECT_RO_PAGES () in same function.
> +///
> +/// @param[in,out] Wp A BOOLEAN variable local to the containing
> +/// function, carrying write protection status from
> +/// WRITE_UNPROTECT_RO_PAGES() to
> +/// WRITE_PROTECT_RO_PAGES().
> +///
> +/// @param[in,out] Cet A BOOLEAN variable local to the containing
> +/// function, carrying control flow integrity
> +/// enforcement status from
> +/// WRITE_UNPROTECT_RO_PAGES() to
> +/// WRITE_PROTECT_RO_PAGES().
> +///
> +#define WRITE_UNPROTECT_RO_PAGES(Wp, Cet) \
> + do { \
> + Cet = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0); \
> + if (Cet) { \
> + DisableCet (); \
> + } \
> + SmmWriteUnprotectReadOnlyPage (&Wp); \
> + } while (FALSE)
> +
> +#define WRITE_PROTECT_RO_PAGES(Wp, Cet) \
> + do { \
> + SmmWriteProtectReadOnlyPage (Wp); \
> + if (Cet) { \
> + EnableCet (); \
> + } \
> + } while (FALSE)
> +
> #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 6f49866615..3d445df213 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -39,64 +39,47 @@ PAGE_TABLE_POOL *mPageTablePool = NULL;
> // If memory used by SMM page table has been mareked as ReadOnly.
> //
> BOOLEAN mIsReadOnlyPageTable = FALSE;
>
> /**
> - Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1.
> + Write unprotect read-only pages if Cr0.Bits.WP is 1.
> +
> + @param[out] WriteProtect If Cr0.Bits.WP is enabled.
>
> - @param[out] WpEnabled If Cr0.WP is enabled.
> - @param[out] CetEnabled If CET is enabled.
> **/
> VOID
> -DisableReadOnlyPageWriteProtect (
> - OUT BOOLEAN *WpEnabled,
> - OUT BOOLEAN *CetEnabled
> +SmmWriteUnprotectReadOnlyPage (
> + OUT BOOLEAN *WriteProtect
> )
> {
> IA32_CR0 Cr0;
>
> - *CetEnabled = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0) ? TRUE : FALSE;
> - Cr0.UintN = AsmReadCr0 ();
> - *WpEnabled = (Cr0.Bits.WP != 0) ? TRUE : FALSE;
> - if (*WpEnabled) {
> - if (*CetEnabled) {
> - //
> - // CET must be disabled if WP is disabled. Disable CET before clearing
> CR0.WP.
> - //
> - DisableCet ();
> - }
> -
> + Cr0.UintN = AsmReadCr0 ();
> + *WriteProtect = (Cr0.Bits.WP != 0);
> + if (*WriteProtect) {
> Cr0.Bits.WP = 0;
> AsmWriteCr0 (Cr0.UintN);
> }
> }
>
> /**
> - Enable Write Protect on pages marked as read-only.
> + Write protect read-only pages.
> +
> + @param[in] WriteProtect If Cr0.Bits.WP should be enabled.
>
> - @param[out] WpEnabled If Cr0.WP should be enabled.
> - @param[out] CetEnabled If CET should be enabled.
> **/
> VOID
> -EnableReadOnlyPageWriteProtect (
> - BOOLEAN WpEnabled,
> - BOOLEAN CetEnabled
> +SmmWriteProtectReadOnlyPage (
> + IN BOOLEAN WriteProtect
> )
> {
> IA32_CR0 Cr0;
>
> - if (WpEnabled) {
> + if (WriteProtect) {
> Cr0.UintN = AsmReadCr0 ();
> Cr0.Bits.WP = 1;
> AsmWriteCr0 (Cr0.UintN);
> -
> - if (CetEnabled) {
> - //
> - // re-enable CET.
> - //
> - EnableCet ();
> - }
> }
> }
>
> /**
> Initialize a buffer pool for page table use only.
> @@ -119,11 +102,11 @@ BOOLEAN
> InitializePageTablePool (
> IN UINTN PoolPages
> )
> {
> VOID *Buffer;
> - BOOLEAN WpEnabled;
> + BOOLEAN WriteProtect;
> BOOLEAN CetEnabled;
>
> //
> // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one
> page for
> // header.
> @@ -157,13 +140,15 @@ InitializePageTablePool (
>
> //
> // If page table memory has been marked as RO, mark the new pool pages as
> read-only.
> //
> if (mIsReadOnlyPageTable) {
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
> +
> SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer,
> EFI_PAGES_TO_SIZE (PoolPages), EFI_MEMORY_RO);
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> +
> + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> }
>
> return TRUE;
> }
>
> @@ -1009,11 +994,11 @@ SetMemMapAttributes (
> UINTN PageTable;
> EFI_STATUS Status;
> IA32_MAP_ENTRY *Map;
> UINTN Count;
> UINT64 MemoryAttribute;
> - BOOLEAN WpEnabled;
> + BOOLEAN WriteProtect;
> BOOLEAN CetEnabled;
>
> SmmGetSystemConfigurationTable
> (&gEdkiiPiSmmMemoryAttributesTableGuid, (VOID
> **)&MemoryAttributesTable);
> if (MemoryAttributesTable == NULL) {
> DEBUG ((DEBUG_INFO, "MemoryAttributesTable - NULL\n"));
> @@ -1055,11 +1040,11 @@ SetMemMapAttributes (
> Status = PageTableParse (PageTable, mPagingMode, Map, &Count);
> }
>
> ASSERT_RETURN_ERROR (Status);
>
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
>
> MemoryMap = MemoryMapStart;
> for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx, 0x%x\n",
> MemoryMap->PhysicalStart, MemoryMap->NumberOfPages));
> if (MemoryMap->Type == EfiRuntimeServicesCode) {
> @@ -1085,11 +1070,12 @@ SetMemMapAttributes (
> );
>
> MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap,
> DescriptorSize);
> }
>
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> +
> FreePool (Map);
>
> PatchSmmSaveStateMap ();
> PatchGdtIdtMap ();
>
> @@ -1392,18 +1378,18 @@ SetUefiMemMapAttributes (
> EFI_STATUS Status;
> EFI_MEMORY_DESCRIPTOR *MemoryMap;
> UINTN MemoryMapEntryCount;
> UINTN Index;
> EFI_MEMORY_DESCRIPTOR *Entry;
> - BOOLEAN WpEnabled;
> + BOOLEAN WriteProtect;
> BOOLEAN CetEnabled;
>
> PERF_FUNCTION_BEGIN ();
>
> DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n"));
>
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
>
> if (mUefiMemoryMap != NULL) {
> MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
> MemoryMap = mUefiMemoryMap;
> for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> @@ -1479,11 +1465,11 @@ SetUefiMemMapAttributes (
>
> Entry = NEXT_MEMORY_DESCRIPTOR (Entry,
> mUefiMemoryAttributesTable->DescriptorSize);
> }
> }
>
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
>
> //
> // Do not free mUefiMemoryAttributesTable, it will be checked in
> IsSmmCommBufferForbiddenAddress().
> //
>
> @@ -1870,11 +1856,11 @@ IfReadOnlyPageTableNeeded (
> VOID
> SetPageTableAttributes (
> VOID
> )
> {
> - BOOLEAN WpEnabled;
> + BOOLEAN WriteProtect;
> BOOLEAN CetEnabled;
>
> if (!IfReadOnlyPageTableNeeded ()) {
> return;
> }
> @@ -1884,20 +1870,21 @@ SetPageTableAttributes (
>
> //
> // Disable write protection, because we need mark page table to be write
> protected.
> // We need *write* page table memory, to mark itself to be *read only*.
> //
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
>
> // Set memory used by page table as Read Only.
> DEBUG ((DEBUG_INFO, "Start...\n"));
> EnablePageTableProtection ();
>
> //
> // Enable write protection, after page table attribute updated.
> //
> - EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
> + WRITE_PROTECT_RO_PAGES (TRUE, CetEnabled);
> +
> mIsReadOnlyPageTable = TRUE;
>
> //
> // Flush TLB after mark all page table pool as read only.
> //
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> index 7ac3c66f91..8142d3ceac 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
> @@ -592,11 +592,11 @@ InitPaging (
> UINT64 Base;
> UINT64 Length;
> UINT64 Limit;
> UINT64 PreviousAddress;
> UINT64 MemoryAttrMask;
> - BOOLEAN WpEnabled;
> + BOOLEAN WriteProtect;
> BOOLEAN CetEnabled;
>
> PERF_FUNCTION_BEGIN ();
>
> PageTable = AsmReadCr3 ();
> @@ -604,11 +604,12 @@ InitPaging (
> Limit = BASE_4GB;
> } else {
> Limit = (IsRestrictedMemoryAccess ()) ? LShiftU64 (1,
> mPhysicalAddressBits) : BASE_4GB;
> }
>
> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
> +
> //
> // [0, 4k] may be non-present.
> //
> PreviousAddress = ((PcdGet8 (PcdNullPointerDetectionPropertyMask) &
> BIT1) != 0) ? BASE_4KB : 0;
>
> @@ -670,11 +671,11 @@ InitPaging (
> //
> Status = ConvertMemoryPageAttributes (PageTable, mPagingMode,
> PreviousAddress, Limit - PreviousAddress, MemoryAttrMask, TRUE, NULL);
> ASSERT_RETURN_ERROR (Status);
> }
>
> - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
>
> //
> // Flush TLB
> //
> CpuFlushTlb ();
> --
> 2.16.2.windows.1
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110845): https://edk2.groups.io/g/devel/message/110845
Mute This Topic: https://groups.io/mt/102434876/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
