From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 85C5A941CFC for ; Fri, 3 Nov 2023 18:37:52 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=W10WeAXJU3cb3T4xZxxVS9tAV8TezpHL63k7Oj1NdUQ=; c=relaxed/simple; d=groups.io; h=From:To:Cc:Subject:Date:Message-Id:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe; s=20140610; t=1699036671; v=1; b=VnOw0N46LXWMrsqg+5HNu8puOx2uu/jlfG7+MH9DUJF/ZYWsWul4+gclS+ats6ZjyrJQ7uRU pXXpYPgcEE75nHGLXzlk452LSKLoiIte/3W2wNtZTAOuUTGvhXFJRxmkH8V5Cv66V5VN6Ran846 iDUPvYHY5xLSc6D0gd4qNbIs= X-Received: by 127.0.0.2 with SMTP id pCOlYY7687511xx677ZLW9Rb; Fri, 03 Nov 2023 11:37:51 -0700 X-Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.100]) by mx.groups.io with SMTP id smtpd.web11.61152.1699036669894246005 for ; Fri, 03 Nov 2023 11:37:50 -0700 X-IronPort-AV: E=McAfee;i="6600,9927,10883"; a="455490016" X-IronPort-AV: E=Sophos;i="6.03,273,1694761200"; d="scan'208";a="455490016" X-Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Nov 2023 11:37:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10883"; a="755237606" X-IronPort-AV: E=Sophos;i="6.03,273,1694761200"; d="scan'208";a="755237606" X-Received: from sh1gapp1009.ccr.corp.intel.com ([10.239.189.219]) by orsmga007.jf.intel.com with ESMTP; 03 Nov 2023 11:37:47 -0700 From: "Wu, Jiaxin" To: devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Zeng Star , Gerd Hoffmann , Rahul Kumar , Laszlo Ersek Subject: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable Date: Sat, 4 Nov 2023 02:37:43 +0800 Message-Id: <20231103183743.17308-1-jiaxin.wu@intel.com> Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,jiaxin.wu@intel.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: c2RdlOrJZBzwi3sQnObSIH93x7686176AA= X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=VnOw0N46; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=intel.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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 Cc: Eric Dong Cc: Ray Ni Cc: Zeng Star Cc: Gerd Hoffmann Cc: Rahul Kumar Cc: Laszlo Ersek Signed-off-by: Jiaxin Wu --- 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. - @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 ); +/// +/// 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 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 -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; + 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. **/ 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; } @@ -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*. + // 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 // - EnableReadOnlyPageWriteProtect (TRUE, CetEnabled); + WRITE_PROTECT_RO_PAGES (WriteProtect, 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..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 (); -- 2.16.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110664): https://edk2.groups.io/g/devel/message/110664 Mute This Topic: https://groups.io/mt/102370465/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-