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 29E1CD810C0 for ; Tue, 7 Nov 2023 13:08:53 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=cJOGOdsoLlJefsjy93J7wSt30SLuaVVZmA3r6BSx8bw=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1699362531; v=1; b=MprPjcssSuAw02tku6On3wYoxRidEakdh/8msGSGGdYPMWhiLYoOccM7O0gIa9IpMVS77Zq/ AbvyB4tMWWO6ZdxvF9HG6sOah6uDKjn82vYCDhHL0c2xq1IqowhAVGmNz0bQVbaWsRXQ9DixFHU yCdVzW4DgkBaPMGNlF3pkpuk= X-Received: by 127.0.0.2 with SMTP id YbUjYY7687511x3ck0HkJ3qm; Tue, 07 Nov 2023 05:08:51 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.10147.1699362530985971567 for ; Tue, 07 Nov 2023 05:08:51 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-658-dFMD9QgFP8SvD3R9rE2tiA-1; Tue, 07 Nov 2023 08:08:46 -0500 X-MC-Unique: dFMD9QgFP8SvD3R9rE2tiA-1 X-Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 60D61830967; Tue, 7 Nov 2023 13:08:46 +0000 (UTC) X-Received: from [10.39.193.64] (unknown [10.39.193.64]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 316DB25C0; Tue, 7 Nov 2023 13:08:43 +0000 (UTC) Message-ID: Date: Tue, 7 Nov 2023 14:08:42 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable To: "Wu, Jiaxin" , "devel@edk2.groups.io" , "Gao, Liming" , "Kinney, Michael D" Cc: "Dong, Eric" , "Ni, Ray" , "Zeng, Star" , Gerd Hoffmann , "Kumar, Rahul R" References: <179532CD4E894831.20624@groups.io> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.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,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: ncmGayaAV4WsS3TSUvOkl3oLx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=MprPjcss; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) On 11/7/23 13:01, Wu, Jiaxin wrote: > Hi Ray & Laszlo, >=20 > Any more comments to this? It's in my review queue. Laszlo >=20 > Thanks, > Jiaxin >=20 >> -----Original Message----- >> From: 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 ; Ni, Ray ; Zeng, = Star >> ; Gerd Hoffmann ; Kumar, Rahul R >> ; Laszlo Ersek >> 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 >> Cc: Ray Ni >> Cc: Zeng Star >> Cc: Gerd Hoffmann >> Cc: Rahul Kumar >> Cc: Laszlo Ersek >> Signed-off-by: Jiaxin Wu >> --- >> 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 =3D ((AsmReadCr4 () & CR4_CET_ENABLE) !=3D 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 =3D NULL; >> // If memory used by SMM page table has been mareked as ReadOnly. >> // >> BOOLEAN mIsReadOnlyPageTable =3D 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 =3D ((AsmReadCr4 () & CR4_CET_ENABLE) !=3D 0) ? TRUE : FA= LSE; >> - Cr0.UintN =3D AsmReadCr0 (); >> - *WpEnabled =3D (Cr0.Bits.WP !=3D 0) ? TRUE : FALSE; >> - if (*WpEnabled) { >> - if (*CetEnabled) { >> - // >> - // CET must be disabled if WP is disabled. Disable CET before cle= aring >> CR0.WP. >> - // >> - DisableCet (); >> - } >> - >> + Cr0.UintN =3D AsmReadCr0 (); >> + *WriteProtect =3D (Cr0.Bits.WP !=3D 0); >> + if (*WriteProtect) { >> Cr0.Bits.WP =3D 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 =3D AsmReadCr0 (); >> Cr0.Bits.WP =3D 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 page= s 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 =3D=3D NULL) { >> DEBUG ((DEBUG_INFO, "MemoryAttributesTable - NULL\n")); >> @@ -1055,11 +1040,11 @@ SetMemMapAttributes ( >> Status =3D PageTableParse (PageTable, mPagingMode, Map, &Count); >> } >> >> ASSERT_RETURN_ERROR (Status); >> >> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled); >> + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled); >> >> MemoryMap =3D MemoryMapStart; >> for (Index =3D 0; Index < MemoryMapEntryCount; Index++) { >> DEBUG ((DEBUG_VERBOSE, "SetAttribute: Memory Entry - 0x%lx, 0x%x\n"= , >> MemoryMap->PhysicalStart, MemoryMap->NumberOfPages)); >> if (MemoryMap->Type =3D=3D EfiRuntimeServicesCode) { >> @@ -1085,11 +1070,12 @@ SetMemMapAttributes ( >> ); >> >> MemoryMap =3D 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 !=3D NULL) { >> MemoryMapEntryCount =3D mUefiMemoryMapSize/mUefiDescriptorSize; >> MemoryMap =3D mUefiMemoryMap; >> for (Index =3D 0; Index < MemoryMapEntryCount; Index++) { >> @@ -1479,11 +1465,11 @@ SetUefiMemMapAttributes ( >> >> Entry =3D 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 wr= ite >> 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 =3D 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 =3D AsmReadCr3 (); >> @@ -604,11 +604,12 @@ InitPaging ( >> Limit =3D BASE_4GB; >> } else { >> Limit =3D (IsRestrictedMemoryAccess ()) ? LShiftU64 (1, >> mPhysicalAddressBits) : BASE_4GB; >> } >> >> - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled); >> + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled); >> + >> // >> // [0, 4k] may be non-present. >> // >> PreviousAddress =3D ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & >> BIT1) !=3D 0) ? BASE_4KB : 0; >> >> @@ -670,11 +671,11 @@ InitPaging ( >> // >> Status =3D 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 >> >> >> >>=20 >> >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-