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

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

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.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>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-