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