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(-)
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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
