Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

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.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Laszlo Ersek <lersek@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.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-