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

["Laszlo Ersek" <lersek@redhat.com>]

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.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 (#110850): https://edk2.groups.io/g/devel/message/110850
Mute This Topic: https://groups.io/mt/102434876/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-