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 D5E87AC120C for ; Fri, 3 Nov 2023 14:30:00 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=TD3HUwoVQFx34tWs1+LSgjmKA7vUaHHX66txZOK+W2M=; 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=1699021799; v=1; b=e6Un7ORYxk5D1VuIYeW3mzqRVc36/87DPgx+PbXgmuf4myDLS30fsKGWet7WvxR/B5NV/dM1 M3q7WxUP39SsMbUBWbuuIF++QZssUJEWQFQ+TjGqlJzD2txva8VeBTxRXRE6iLRhTBVoo3pZlTu 1IahoRjPxHAkZCrEWxAEJKzI= X-Received: by 127.0.0.2 with SMTP id gfNpYY7687511xiy8jnsxd05; Fri, 03 Nov 2023 07:29:59 -0700 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.web11.52846.1699021798713679752 for ; Fri, 03 Nov 2023 07:29:58 -0700 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-504-B9G8cHsXM_ul_mnWIoQY0Q-1; Fri, 03 Nov 2023 10:29:54 -0400 X-MC-Unique: B9G8cHsXM_ul_mnWIoQY0Q-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 BFFD929AA38E; Fri, 3 Nov 2023 14:29:53 +0000 (UTC) X-Received: from [10.39.192.20] (unknown [10.39.192.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3BC1E25C1; Fri, 3 Nov 2023 14:29:52 +0000 (UTC) Message-ID: <8f942ffe-3467-80f7-38ae-c4bd7aabf7ec@redhat.com> Date: Fri, 3 Nov 2023 15:29:50 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v1] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable To: devel@edk2.groups.io, jiaxin.wu@intel.com Cc: Eric Dong , Ray Ni , Zeng Star , Gerd Hoffmann , Rahul Kumar References: <20231103121405.3356-1-jiaxin.wu@intel.com> From: "Laszlo Ersek" In-Reply-To: <20231103121405.3356-1-jiaxin.wu@intel.com> 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: UeTukkbncUA33TSn2Czrq5cTx7686176AA= 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=e6Un7ORY; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.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 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). >=20 > 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). >=20 > With above requirement, CET feature enable & disable must be in the > same function to avoid stack mismatch issue. >=20 > Cc: Eric Dong > Cc: Ray Ni > Cc: Zeng Star > Cc: Gerd Hoffmann > Cc: Rahul Kumar > Signed-off-by: Jiaxin Wu > --- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 10 +- > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 104 ++++++++++++++-= ------ > UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 19 +++- > 3 files changed, 91 insertions(+), 42 deletions(-) I have two comments: (1) both the pre-patch code and the post-patch code have several instances of the following pattern: Boolean =3D (Expression !=3D 0) ? TRUE : FALSE; This is an anti-pattern. It should only be: Boolean =3D Expression !=3D 0; or if you prefer the parentheses, then Boolean =3D (Expression !=3D 0); I recommend cleaning up all instances of this, independently of the actual issue. (2) The critical information is in the last paragraph of the commit message ("CET feature enable & disable must be in the same function to avoid stack mismatch"); however, that critical information is not visible anywhere in the new code. People will not understand why the code is littered with EnableCet / DisableCet calls. In fact, I only realized the weight of the commit message after I first looked at the patch, and deduced that the patch did, functionally speaking, nothing at all! So here's what I recommend: please replace the functions - EnableReadOnlyPageWriteProtect() - DisableReadOnlyPageWriteProtect() with *macros* - WRITE_PROTECT_RO_PAGES() - WRITE_UNPROTECT_RO_PAGES() These macros should continue taking two parameters (Wp and Cet). The WP manipulation can be factored out to helper functions if necessary, but the CET manipulation needs to be expanded inline. (I've also renamed the APIs because the current API names are awkward. "Enable Write Protection" is just better expressed as "Write Protect", and "Disable Write Protection" is just better expressed as "Write Unprotect", in my opinion.) And then, comments on the macro definitions should explain that these pieces of logic are defined as macros and not functions because "CET feature enable & disable must be in the same function to avoid stack mismatch". Thanks! Laszlo >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSm= mCpuDxeSmm/PiSmmCpuDxeSmm.h > index 654935dc76..daa843b057 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -1554,26 +1554,24 @@ SmmWaitForApArrival ( > =20 > /** > Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1= . > =20 > @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 > ); > =20 > /** > Enable Write Protect on pages marked as read-only. > =20 > @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 > ); > =20 > #endif > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpu= Pkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > index 6f49866615..2c198a161a 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > @@ -42,61 +42,44 @@ BOOLEAN mIsReadOnlyPageTable =3D FALSE; > =20 > /** > Disable Write Protect on pages marked as read-only if Cr0.Bits.WP is 1= . > =20 > @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; > =20 > - *CetEnabled =3D ((AsmReadCr4 () & CR4_CET_ENABLE) !=3D 0) ? TRUE : FAL= SE; > - Cr0.UintN =3D AsmReadCr0 (); > - *WpEnabled =3D (Cr0.Bits.WP !=3D 0) ? TRUE : FALSE; > + 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 clea= ring CR0.WP. > - // > - DisableCet (); > - } > - > Cr0.Bits.WP =3D 0; > AsmWriteCr0 (Cr0.UintN); > } > } > =20 > /** > Enable Write Protect on pages marked as read-only. > =20 > @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; > =20 > if (WpEnabled) { > Cr0.UintN =3D AsmReadCr0 (); > Cr0.Bits.WP =3D 1; > AsmWriteCr0 (Cr0.UintN); > - > - if (CetEnabled) { > - // > - // re-enable CET. > - // > - EnableCet (); > - } > } > } > =20 > /** > Initialize a buffer pool for page table use only. > @@ -157,13 +140,29 @@ InitializePageTablePool ( > =20 > // > // 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 =3D ((AsmReadCr4 () & CR4_CET_ENABLE) !=3D 0) ? TRUE : FA= LSE; > + if (CetEnabled) { > + DisableCet (); > + } > + > + DisableReadOnlyPageWriteProtect (&WpEnabled); > + > SmmSetMemoryAttributes ((EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAG= ES_TO_SIZE (PoolPages), EFI_MEMORY_RO); > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled); > + > + // > + // Enable the WP and restore CET to enable > + // > + EnableReadOnlyPageWriteProtect (WpEnabled); > + if (CetEnabled) { > + EnableCet (); > + } > } > =20 > return TRUE; > } > =20 > @@ -1055,11 +1054,19 @@ SetMemMapAttributes ( > Status =3D PageTableParse (PageTable, mPagingMode, Map, &Count); > } > =20 > ASSERT_RETURN_ERROR (Status); > =20 > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled); > + // > + // CET must be disabled if WP is disabled. > + // > + CetEnabled =3D ((AsmReadCr4 () & CR4_CET_ENABLE) !=3D 0) ? TRUE : FALS= E; > + if (CetEnabled) { > + DisableCet (); > + } > + > + DisableReadOnlyPageWriteProtect (&WpEnabled); > =20 > 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 +1092,18 @@ SetMemMapAttributes ( > ); > =20 > MemoryMap =3D NEXT_MEMORY_DESCRIPTOR (MemoryMap, DescriptorSize); > } > =20 > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled); > + // > + // Enable the WP and restore CET to enable > + // > + EnableReadOnlyPageWriteProtect (WpEnabled); > + if (CetEnabled) { > + EnableCet (); > + } > + > FreePool (Map); > =20 > PatchSmmSaveStateMap (); > PatchGdtIdtMap (); > =20 > @@ -1399,11 +1413,19 @@ SetUefiMemMapAttributes ( > =20 > PERF_FUNCTION_BEGIN (); > =20 > DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n")); > =20 > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled); > + // > + // CET must be disabled if WP is disabled. > + // > + CetEnabled =3D ((AsmReadCr4 () & CR4_CET_ENABLE) !=3D 0) ? TRUE : FALS= E; > + if (CetEnabled) { > + DisableCet (); > + } > + > + DisableReadOnlyPageWriteProtect (&WpEnabled); > =20 > if (mUefiMemoryMap !=3D NULL) { > MemoryMapEntryCount =3D mUefiMemoryMapSize/mUefiDescriptorSize; > MemoryMap =3D mUefiMemoryMap; > for (Index =3D 0; Index < MemoryMapEntryCount; Index++) { > @@ -1479,11 +1501,17 @@ SetUefiMemMapAttributes ( > =20 > Entry =3D NEXT_MEMORY_DESCRIPTOR (Entry, mUefiMemoryAttributesTabl= e->DescriptorSize); > } > } > =20 > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled); > + // > + // Enable the WP and restore CET to enable > + // > + EnableReadOnlyPageWriteProtect (WpEnabled); > + if (CetEnabled) { > + EnableCet (); > + } > =20 > // > // Do not free mUefiMemoryAttributesTable, it will be checked in IsSmm= CommBufferForbiddenAddress(). > // > =20 > @@ -1881,23 +1909,31 @@ SetPageTableAttributes ( > =20 > PERF_FUNCTION_BEGIN (); > DEBUG ((DEBUG_INFO, "SetPageTableAttributes\n")); > =20 > // > - // Disable write protection, because we need mark page table to be wri= te 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 =3D ((AsmReadCr4 () & CR4_CET_ENABLE) !=3D 0) ? TRUE : FALS= E; > + if (CetEnabled) { > + DisableCet (); > + } > + > + DisableReadOnlyPageWriteProtect (&WpEnabled); > =20 > // Set memory used by page table as Read Only. > DEBUG ((DEBUG_INFO, "Start...\n")); > EnablePageTableProtection (); > =20 > // > - // 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 =3D TRUE; > =20 > // > // Flush TLB after mark all page table pool as read only. > // > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpu= DxeSmm/SmmProfile.c > index 7ac3c66f91..71fdf5f34a 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > @@ -604,11 +604,20 @@ InitPaging ( > Limit =3D BASE_4GB; > } else { > Limit =3D (IsRestrictedMemoryAccess ()) ? LShiftU64 (1, mPhysicalAdd= ressBits) : BASE_4GB; > } > =20 > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled); > + // > + // CET must be disabled if WP is disabled. > + // > + CetEnabled =3D ((AsmReadCr4 () & CR4_CET_ENABLE) !=3D 0) ? TRUE : FALS= E; > + if (CetEnabled) { > + DisableCet (); > + } > + > + DisableReadOnlyPageWriteProtect (&WpEnabled); > + > // > // [0, 4k] may be non-present. > // > PreviousAddress =3D ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & = BIT1) !=3D 0) ? BASE_4KB : 0; > =20 > @@ -670,11 +679,17 @@ InitPaging ( > // > Status =3D ConvertMemoryPageAttributes (PageTable, mPagingMode, Prev= iousAddress, Limit - PreviousAddress, MemoryAttrMask, TRUE, NULL); > ASSERT_RETURN_ERROR (Status); > } > =20 > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled); > + // > + // Enable the WP and restore CET to enable > + // > + EnableReadOnlyPageWriteProtect (WpEnabled); > + if (CetEnabled) { > + EnableCet (); > + } > =20 > // > // Flush TLB > // > CpuFlushTlb (); -=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 (#110628): https://edk2.groups.io/g/devel/message/110628 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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-