On 11/7/23 13:01, Wu, Jiaxin wrote:
> Hi Ray & Laszlo,
>
> Any more comments to this?
It's in my review queue.
Laszlo
>
> 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 (#110850): https://edk2.groups.io/g/devel/message/110850
Mute This Topic: https://groups.io/mt/102434876/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
