Thanks Laszlo, all fixed in version 3.
> -----Original Message-----
> From: Laszlo Ersek <ler...@redhat.com>
> Sent: Sunday, November 5, 2023 7:01 PM
> To: Wu, Jiaxin <jiaxin...@intel.com>; 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>
> Subject: Re: [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception
> when CET enable
>
> Hi Jiaxin,
>
> looks great; now I'm only asking for a few light touch-ups:
>
> On 11/3/23 19:37, Jiaxin Wu 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, define below 2 macros instead of functions
> > for WP & CET operation:
> > WRITE_UNPROTECT_RO_PAGES (Wp, Cet)
> > WRITE_PROTECT_RO_PAGES (Wp, Cet)
> > Because "CET" feature disable & enable 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.
> >
> > Change-Id: I4e126697efcd8dbfb4887da034d8691bfca969e3
>
> (1) Please drop the Change-Id line; it is not meaningful in the upstream
> repo.
>
> > 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 | 46
> ++++++++---
> > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 96
> +++++++++++-----------
> > UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 13 ++-
> > 3 files changed, 94 insertions(+), 61 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > index 654935dc76..5d167899ff 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > @@ -1551,29 +1551,51 @@ 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.WP is enabled.
>
> (2) The comment references to the WP bit are not consistent. We should
> either stick with Cr0.WP or Cr0.Bits.WP, but not mix them.
>
> I understand this inconsistency exists pre-patch, but because we're
> modifying the same sentences, I think it would be OK to clean up the WP
> bit references as well, at the same time.
>
> >
> > - @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[out] WriteProtect If Cr0.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
> > );
>
> (3) If, under (2), you opt for preserving "Cr0.Bits.WP", then please use
> that term here too.
>
> >
> > +///
> > +/// 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.
> > +///
> > +#define WRITE_UNPROTECT_RO_PAGES(Wp, Cet) \
> > +{ \
> > + Cet = ((AsmReadCr4 () & CR4_CET_ENABLE) != 0); \
> > + if (Cet) { \
> > + DisableCet (); \
> > + } \
> > + SmmWriteUnprotectReadOnlyPage(&Wp); \
> > +}
> > +
> > +#define WRITE_PROTECT_RO_PAGES(Wp, Cet) \
> > +{ \
> > + SmmWriteProtectReadOnlyPage(Wp); \
> > + if (Cet) { \
> > + EnableCet (); \
> > + } \
> > +}
> > +
> > #endif
>
> (4) Assuming the ECC Check Plugin tolerates it, I recommend adding two
> entries to the documentation here:
>
> /// @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().
>
> I recommend this because it clarifies that neither Wp nor Cet are
> supposed to have side effects (for example, "Cet" is evaluated multiple
> times in WRITE_UNPROTECT_RO_PAGES(), so calling the macro with
> something
> like *CetVariable++ would not work well).
>
>
> (5) A space character is missing right after each of
> "SmmWriteUnprotectReadOnlyPage" and "SmmWriteProtectReadOnlyPage",
> before the parens.
>
>
> (6) It would be more idiomatic to #define these macros as:
>
> do { \
> ... \
> } while (FALSE)
>
> because this replacement text is more suitable to be followed by a
> semicolon ";".
>
>
> > diff --git
> a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > index 6f49866615..8edfddd3ea 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.WP is enabled.
> >
> > - @param[out] WpEnabled If Cr0.WP is enabled.
> > - @param[out] CetEnabled If CET is enabled.
> > **/
> > VOID
>
> (7) If you decide to touch up the function comment in the header file,
> then please keep this in sync.
>
>
> > -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) ? TRUE : FALSE;
>
> (8) Can you remove the " ? TRUE : FALSE" part?
>
>
> > + if (*WriteProtect) {
> > Cr0.Bits.WP = 0;
> > AsmWriteCr0 (Cr0.UintN);
> > }
> > }
> >
> > /**
> > - Enable Write Protect on pages marked as read-only.
> > + Write protect read-only pages.
> > +
> > + @param[out] WriteProtect If Cr0.WP should be enabled.
> >
> > - @param[out] WpEnabled If Cr0.WP should be enabled.
> > - @param[out] CetEnabled If CET should be enabled.
> > **/
>
> (9) If you decide to touch up the function comment in the header file,
> then please keep this in sync.
>
> > 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,21 @@ 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.
> > + //
> > + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
> > +
> > 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
> > + //
> > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> > }
> >
> > return TRUE;
> > }
> >
>
> (10) Here on the other hand I suggest *removing* these comments. The
> macros are really well defined at this point, so I think just invoking
> the macros should suffice. Up to you, anyway; if you'd like to keep the
> comments, I won't insist. :)
>
> If you decide to remove the comments, then please keep the rest of the
> patch (below) consistent as well.
>
> > @@ -1009,11 +1000,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 +1046,14 @@ SetMemMapAttributes (
> > Status = PageTableParse (PageTable, mPagingMode, Map, &Count);
> > }
> >
> > ASSERT_RETURN_ERROR (Status);
> >
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + 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 +1079,15 @@ SetMemMapAttributes (
> > );
> >
> > MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap,
> DescriptorSize);
> > }
> >
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> > +
> > FreePool (Map);
> >
> > PatchSmmSaveStateMap ();
> > PatchGdtIdtMap ();
> >
> > @@ -1392,18 +1390,21 @@ 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);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
> >
> > if (mUefiMemoryMap != NULL) {
> > MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
> > MemoryMap = mUefiMemoryMap;
> > for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> > @@ -1479,11 +1480,14 @@ SetUefiMemMapAttributes (
> >
> > Entry = NEXT_MEMORY_DESCRIPTOR (Entry,
> mUefiMemoryAttributesTable->DescriptorSize);
> > }
> > }
> >
> > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled);
> > + //
> > + // Enable the WP and restore CET to enable
> > + //
> > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> >
> > //
> > // Do not free mUefiMemoryAttributesTable, it will be checked in
> IsSmmCommBufferForbiddenAddress().
> > //
> >
> > @@ -1870,34 +1874,34 @@ IfReadOnlyPageTableNeeded (
> > VOID
> > SetPageTableAttributes (
> > VOID
> > )
> > {
> > - BOOLEAN WpEnabled;
> > + BOOLEAN WriteProtect;
> > BOOLEAN CetEnabled;
> >
> > if (!IfReadOnlyPageTableNeeded ()) {
> > return;
> > }
> >
> > 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*.
>
> (11) These two comment lines seem important, and not related to the
> patch -- they explain *why* we are doing the WP / CET twiddling. So I
> recommend keeping these (and only removing the comment line below)!
>
> > + // CET must be disabled if WP is disabled.
> > //
> > - 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.
> > + // Enable the WP and restore CET to enable
>
> (12) same as (11) here.
>
> > //
> > - EnableReadOnlyPageWriteProtect (TRUE, CetEnabled);
> > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> > +
>
> (13) Whoa, the *original* EnableReadOnlyPageWriteProtect() call passes
> the TRUE constant, not WpEnabled!
>
> Was that intentional, or an independent oversight?
>
> If it was an independent oversight in the original code, then I agree we
> can fix it, and I'm not even asking for a separate patch, but please
> mention it in the commit message.
>
> > 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..8f6a2d440e 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,15 @@ InitPaging (
> > Limit = BASE_4GB;
> > } else {
> > Limit = (IsRestrictedMemoryAccess ()) ? LShiftU64 (1,
> mPhysicalAddressBits) : BASE_4GB;
> > }
> >
> > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled);
> > + //
> > + // CET must be disabled if WP is disabled.
> > + //
> > + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled);
> > +
> > //
> > // [0, 4k] may be non-present.
> > //
> > PreviousAddress = ((PcdGet8 (PcdNullPointerDetectionPropertyMask) &
> BIT1) != 0) ? BASE_4KB : 0;
> >
> > @@ -670,11 +674,14 @@ 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
> > + //
> > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled);
> >
> > //
> > // Flush TLB
> > //
> > CpuFlushTlb ();
>
> Thanks!
> Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110696): https://edk2.groups.io/g/devel/message/110696
Mute This Topic: https://groups.io/mt/102370465/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
