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 09C4C9414A3 for ; Sun, 5 Nov 2023 11:00:41 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=bBWBmayvIWinUVpNDade7JDV81E8mGr26iPUqxQsa8E=; 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=1699182040; v=1; b=ffJlPgo6FJ/0SJRNXRRSUJyIEzHYgpXGoq2Na1MdaqLaIZ+a5O1UXwjvqRrS7c25H9fEkXC3 sV82pbBQHjSV1iGFbQnMWqTStSztCrjvnKZlYmOCqGX5wTu4kqJqJ/lBFLmCKXEUHTajJGo29Lw gNbdXaG+0ybLUmEuU0MQUqhg= X-Received: by 127.0.0.2 with SMTP id BLtyYY7687511x3BwpqDYkGN; Sun, 05 Nov 2023 03:00:40 -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.28044.1699182039756260631 for ; Sun, 05 Nov 2023 03:00:40 -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-148-pzS55GrUPW2Ov7MB05rnnQ-1; Sun, 05 Nov 2023 06:00:37 -0500 X-MC-Unique: pzS55GrUPW2Ov7MB05rnnQ-1 X-Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (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 41DAD85A58A; Sun, 5 Nov 2023 11:00:37 +0000 (UTC) X-Received: from [10.39.192.38] (unknown [10.39.192.38]) by smtp.corp.redhat.com (Postfix) with ESMTPS id BAC3D2166B26; Sun, 5 Nov 2023 11:00:35 +0000 (UTC) Message-ID: <9dffc6c9-af88-23fb-42ef-8e4ede2002b1@redhat.com> Date: Sun, 5 Nov 2023 12:00:34 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Fix CP Exception when CET enable To: Jiaxin Wu , devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Zeng Star , Gerd Hoffmann , Rahul Kumar References: <20231103183743.17308-1-jiaxin.wu@intel.com> From: "Laszlo Ersek" In-Reply-To: <20231103183743.17308-1-jiaxin.wu@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.6 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: EjF499Gj7SFUvfMHjTtg3B2vx7686176AA= 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=ffJlPgo6; 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 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). >=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, 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. >=20 > Note: WRITE_UNPROTECT_RO_PAGES () must be called pair with > WRITE_PROTECT_RO_PAGES () in same function. >=20 > Change-Id: I4e126697efcd8dbfb4887da034d8691bfca969e3 (1) Please drop the Change-Id line; it is not meaningful in the upstream repo. > 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(-) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSm= mCpuDxeSmm/PiSmmCpuDxeSmm.h > index 654935dc76..5d167899ff 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -1551,29 +1551,51 @@ VOID > SmmWaitForApArrival ( > VOID > ); > =20 > /** > - 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. > =20 > - @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 > ); > =20 > /** > - Enable Write Protect on pages marked as read-only. > + Write protect read-only pages. > + > + @param[out] WriteProtect If Cr0.WP should be enabled. > =20 > - @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. > =20 > +/// > +/// 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 =3D ((AsmReadCr4 () & CR4_CET_ENABLE) !=3D 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/UefiCpu= Pkg/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 =3D NULL; > // If memory used by SMM page table has been mareked as ReadOnly. > // > BOOLEAN mIsReadOnlyPageTable =3D FALSE; > =20 > /** > - 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. > =20 > - @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; > =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; > - if (*WpEnabled) { > - if (*CetEnabled) { > - // > - // CET must be disabled if WP is disabled. Disable CET before clea= ring CR0.WP. > - // > - DisableCet (); > - } > - > + Cr0.UintN =3D AsmReadCr0 (); > + *WriteProtect =3D (Cr0.Bits.WP !=3D 0) ? TRUE : FALSE; (8) Can you remove the " ? TRUE : FALSE" part? > + if (*WriteProtect) { > Cr0.Bits.WP =3D 0; > AsmWriteCr0 (Cr0.UintN); > } > } > =20 > /** > - Enable Write Protect on pages marked as read-only. > + Write protect read-only pages. > + > + @param[out] WriteProtect If Cr0.WP should be enabled. > =20 > - @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; > =20 > - if (WpEnabled) { > + if (WriteProtect) { > 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. > @@ -119,11 +102,11 @@ BOOLEAN > InitializePageTablePool ( > IN UINTN PoolPages > ) > { > VOID *Buffer; > - BOOLEAN WpEnabled; > + BOOLEAN WriteProtect; > BOOLEAN CetEnabled; > =20 > // > // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one p= age for > // header. > @@ -157,13 +140,21 @@ 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. > + // > + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled); > + > 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 > + // > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled); > } > =20 > return TRUE; > } > =20 (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; > =20 > SmmGetSystemConfigurationTable (&gEdkiiPiSmmMemoryAttributesTableGuid,= (VOID **)&MemoryAttributesTable); > if (MemoryAttributesTable =3D=3D NULL) { > DEBUG ((DEBUG_INFO, "MemoryAttributesTable - NULL\n")); > @@ -1055,11 +1046,14 @@ 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. > + // > + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled); > =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 +1079,15 @@ SetMemMapAttributes ( > ); > =20 > MemoryMap =3D NEXT_MEMORY_DESCRIPTOR (MemoryMap, DescriptorSize); > } > =20 > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled); > + // > + // Enable the WP and restore CET to enable > + // > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled); > + > FreePool (Map); > =20 > PatchSmmSaveStateMap (); > PatchGdtIdtMap (); > =20 > @@ -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; > =20 > PERF_FUNCTION_BEGIN (); > =20 > DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n")); > =20 > - DisableReadOnlyPageWriteProtect (&WpEnabled, &CetEnabled); > + // > + // CET must be disabled if WP is disabled. > + // > + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled); > =20 > if (mUefiMemoryMap !=3D NULL) { > MemoryMapEntryCount =3D mUefiMemoryMapSize/mUefiDescriptorSize; > MemoryMap =3D mUefiMemoryMap; > for (Index =3D 0; Index < MemoryMapEntryCount; Index++) { > @@ -1479,11 +1480,14 @@ SetUefiMemMapAttributes ( > =20 > Entry =3D NEXT_MEMORY_DESCRIPTOR (Entry, mUefiMemoryAttributesTabl= e->DescriptorSize); > } > } > =20 > - EnableReadOnlyPageWriteProtect (WpEnabled, CetEnabled); > + // > + // Enable the WP and restore CET to enable > + // > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled); > =20 > // > // Do not free mUefiMemoryAttributesTable, it will be checked in IsSmm= CommBufferForbiddenAddress(). > // > =20 > @@ -1870,34 +1874,34 @@ IfReadOnlyPageTableNeeded ( > VOID > SetPageTableAttributes ( > VOID > ) > { > - BOOLEAN WpEnabled; > + BOOLEAN WriteProtect; > BOOLEAN CetEnabled; > =20 > if (!IfReadOnlyPageTableNeeded ()) { > return; > } > =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*= . (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); > =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 (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 =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..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; > =20 > PERF_FUNCTION_BEGIN (); > =20 > PageTable =3D AsmReadCr3 (); > @@ -604,11 +604,15 @@ 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. > + // > + WRITE_UNPROTECT_RO_PAGES (WriteProtect, CetEnabled); > + > // > // [0, 4k] may be non-present. > // > PreviousAddress =3D ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & = BIT1) !=3D 0) ? BASE_4KB : 0; > =20 > @@ -670,11 +674,14 @@ 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 > + // > + WRITE_PROTECT_RO_PAGES (WriteProtect, CetEnabled); > =20 > // > // Flush TLB > // > CpuFlushTlb (); Thanks! Laszlo -=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 (#110680): https://edk2.groups.io/g/devel/message/110680 Mute This Topic: https://groups.io/mt/102370465/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-