From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.2534.1604545444718406532 for ; Wed, 04 Nov 2020 19:04:05 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=jFB+SbJd; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1604545443; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=c4spb7hsSyY6+Zk1UYPeH1FMcdMLU7ec7qRPv05D6CM=; b=jFB+SbJdpJimYCNHR4Hmgyo521wwleE2CkTHbTTgFTAz5I1LQO6D5qfrDTAjd8Cp9njWTu cI0YNTKAsPoVUrxSTa98Kj3BIVKecbBIf4z00lQ2w1A3hd5uSN13XNnSIRLclUD/Fp58r0 eZx6spO3sXnPvp+PfvA9srqQxTgkspY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-69-g-IXp5TbNuOP38XQcICTmA-1; Wed, 04 Nov 2020 22:03:59 -0500 X-MC-Unique: g-IXp5TbNuOP38XQcICTmA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B681E1074646; Thu, 5 Nov 2020 03:03:57 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-163.ams2.redhat.com [10.36.112.163]) by smtp.corp.redhat.com (Postfix) with ESMTP id A921D73661; Thu, 5 Nov 2020 03:03:55 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Return level paging type for Internal CR3 To: devel@edk2.groups.io, w.sheng@intel.com Cc: Eric Dong , Ray Ni , Rahul Kumar , Jiewen Yao , Bret Barkelew References: <20201103043008.26536-1-w.sheng@intel.com> <20201103043008.26536-3-w.sheng@intel.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 5 Nov 2020 04:03:54 +0100 MIME-Version: 1.0 In-Reply-To: <20201103043008.26536-3-w.sheng@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/03/20 05:30, Sheng Wei wrote: > If mInternalCr3 is non zero, it will use the page table from mInternalCr3. > And it will use mInternalIs5LevelPaging to reflect the page table type. > If use page table from CR3, reflect the page table type by CR4 LA57 bit. > It is a fix for enable CET feature with 5 level paging. > > PiCpuSmmEntry() will generate the page table of SMM shack memory. > If CET feature is enabled, it also includes the SMM shadows shack memory. > And we need to set some attributes on SMM shadows shack memory > in PiCpuSmmEntry() when CET feature is enabled. > Since the page table of SMM shack memory is used in SMI entry, and it does not > set to CR3 in PiCpuSmmEntry(). We use mInternalCr3 as page table root > when PiCpuSmmEntry() calls ConvertMemoryPageAttributes(). We need to use > mInternalIs5LevelPaging determining whether 5-level paging is enabled or not. > If mInternalCr3 is zero, ConvertMemoryPageAttributes() will use the page table > in CR3, and refects the page table type by CR4 LA57 bit. > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3015 > > Signed-off-by: Sheng Wei > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > Cc: Jiewen Yao > --- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 10 +++++ > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 43 ++++++++++++++++++++-- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 2 + > 3 files changed, 52 insertions(+), 3 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 7fb3a2d9e4..3eb6af62a7 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -951,6 +951,16 @@ GetPageTableBase ( > VOID > ); > > +/** > + This function set the internal page table type to 5 level paging or 4 level paging. > + > + @param Is5LevelPaging TRUE means 5 level paging. FALSE means 4 level paging. > +**/ > +VOID > +SetPageTableType ( > + IN BOOLEAN Is5LevelPaging > + ); > + > /** > This function sets the attributes for the memory region specified by BaseAddress and > Length from their current attributes to the attributes specified by Attributes. > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > index d67f036aea..890782a394 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > @@ -33,6 +33,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = { > }; > > UINTN mInternalCr3; > +BOOLEAN mInternalIs5LevelPaging = FALSE; > > /** > Set the internal page table base address. > @@ -65,6 +66,44 @@ GetPageTableBase ( > return (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64); > } > > +/** > + This function set the internal page table type to 5 level paging or 4 level paging. > + > + @param Is5LevelPaging TRUE means 5 level paging. FALSE means 4 level paging. > +**/ > +VOID > +SetPageTableType ( > + IN BOOLEAN Is5LevelPaging > + ) > +{ > + mInternalIs5LevelPaging = Is5LevelPaging; > +} > + > +/** > + Return if the page table is 5 level paging. > + > + @return TRUE The page table base is 5 level paging. > + @return FALSE The page table base is 4 level paging. > +**/ > +STATIC > +BOOLEAN > +Is5LevelPageTableBase ( > + VOID > + ) > +{ > + IA32_CR4 Cr4; > + > + // If mInternalCr3 is non zero, it will use the page table from mInternalCr3. > + // And it will use mInternalIs5LevelPaging to reflect the page table type. > + if (mInternalCr3 != 0) { > + return mInternalIs5LevelPaging; > + } > + > + // If use page table from CR3, reflect the page table type by CR4 LA57 bit. > + Cr4.UintN = AsmReadCr4 (); > + return (BOOLEAN) (Cr4.Bits.LA57 == 1); > +} > + > /** > Return length according to page attributes. > > @@ -131,7 +170,6 @@ GetPageTableEntry ( > UINT64 *L3PageTable; > UINT64 *L4PageTable; > UINT64 *L5PageTable; > - IA32_CR4 Cr4; > BOOLEAN Enable5LevelPaging; > > Index5 = ((UINTN)RShiftU64 (Address, 48)) & PAGING_PAE_INDEX_MASK; > @@ -140,8 +178,7 @@ GetPageTableEntry ( > Index2 = ((UINTN)Address >> 21) & PAGING_PAE_INDEX_MASK; > Index1 = ((UINTN)Address >> 12) & PAGING_PAE_INDEX_MASK; > > - Cr4.UintN = AsmReadCr4 (); > - Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1); > + Enable5LevelPaging = Is5LevelPageTableBase(); > > if (sizeof(UINTN) == sizeof(UINT64)) { > if (Enable5LevelPaging) { > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index 810985df20..6f2f4adb7d 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -387,6 +387,8 @@ SmmInitPageTable ( > SetSubEntriesNum (Pml4Entry, 3); > PTEntry = Pml4Entry; > > + SetPageTableType(m5LevelPagingNeeded); > + > if (m5LevelPagingNeeded) { > // > // Fill PML5 entry > This approach is wrong, in my opinion. Here's my understanding. (1) The SMM page tables are allocated / initialized on the following call path. PiCpuSmmEntry() [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c] InitializeMpServiceData() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c] SmmInitPageTable() [UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c] (2) When CET is enabled, then PiCpuSmmEntry() must further modify the just-allocated / just-initialized page tables. (And not the page tables that are currently in use.) The CET-related modifications are that (a) the shadow stack pages must be marked EFI_MEMORY_RO, and (b) if the stack guard feature is enabled, in addition to CET, then the shadow stacks' guard pages must be marked as absent (EFI_MEMORY_RP). These CET-specific page attribute modifications could have been done with the existing utility function SmmSetMemoryAttributes(). However, the SmmSetMemoryAttributes() function manipulates the *currently effective* page tables, through the following long chain of function calls: SmmSetMemoryAttributes() [UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c] SmmSetMemoryAttributesEx() [UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c] ConvertMemoryPageAttributes() [UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c] GetPageTableEntry() [UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c] GetPageTableBase() [UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c] AsmReadCr3() Commit 3eb69b081c68, which introduced CET support, didn't want to generalize this whole list of functions to an externally-provided CR3 (meaning an additional parameter for every function affected). Instead, it decided to add an override to the *innermost* function (namely GetPageTableBase()), via the new global variable "mInternalGr3". For the above-described page table updates (a) and (b) *only*, commit 3eb69b081c68 introduced two helper functions, SetShadowStack() and SetNotPresentPage(), respectively. These would temporarily set the override for GetPageTableBase(), and call SmmSetMemoryAttributes(). The result would be: PiCpuSmmEntry() [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c] InitializeMpServiceData() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c] SmmInitPageTable() [UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c] SetShadowStack() [UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c] // // SET OVERRIDE // SmmSetMemoryAttributes() [UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c] // // UNSET OVERRIDE // SetNotPresentPage() [UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c] // // SET OVERRIDE // SmmSetMemoryAttributes() [UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c] // // UNSET OVERRIDE // Unfortunately however, the ConvertMemoryPageAttributes() function doesn't only call GetPageTableEntry(); it also calls ConvertPageEntryAttribute(). And the latter function also had to be updated, dependent on the "mInternalGr3" override. So we can see that this override was a bad idea, because it rendered some functions *implicitly* stateful (dependent on global data, which is hard to track down). While it was more comfortable than implementing a bunch of parameter passing, it introduced hidden state. Importantly, GetPageTableBase() is reached via a *whole lot* of other call paths (too many to enumerate here). They now all depend on "mInternalGr3", without the source code explicitly showing it! (3) When 5-level paging was introduced in commit 4eee0cc7cc0d, SmmInitPageTable() was modified to take advantage of 5-level paging, but the information returned by SmmInitPageTable() was not extended with that property, beyond the just-allocated Cr3. The depth of the page tables created by SmmInitPageTable() could now be 5 levels, but that fact was not explicitly propagated to InitializeMpServiceData() and PiCpuSmmEntry(). It was again only stored in a new global variable, "m5LevelPagingSupport". Consequently, this information was not forwarded by PiCpuSmmEntry() either, down to the page table override within SmmSetMemoryAttributes()! And so GetPageTableEntry() would assume a page tables depth based on the live CR4 value, rather than what was set up by SmmInitPageTable(). (4) In commit 86ad762fa7a5, "m5LevelPagingSupport" was replaced with "m5LevelPagingNeeded", but it would still be set / determined in SmmInitPageTable(). Therefore the data flow issue was not affected by commit 86ad762fa7a5. For the current discussion, we can consider it a simple rename. (5) The above explains why the current patch / approach is wrong. The current patch introduces yet another global variable ("mInternalIs5LevelPaging"), for propagating "m5LevelPagingNeeded" out of the X64-specific implementation of SmmInitPageTable(). This increases confusion. Ultimately, every site that depends on "mInternalGr3" *and* consumes "Cr4.Bits.LA57" currently, should choose between the existent "m5LevelPagingNeeded" variable vs. the actual CR4 register (dependent on "mInternalGr3"). But the patch only modifies one such dependent site, near the GetPageTableBase() function call in GetPageTableEntry(). There are other dependent sites: - The ConvertPageEntryAttribute() function depends on "mInternalGr3". It doesn't depend on CR4 though, so this function indeed needs no update. - All other "mInternalGr3" dependencies go through GetPageTableBase(): - The SetPageTableAttributes() function in "UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c": This is IA32-specific, so 5-level paging makes no sense. OK; no functional update needed. - The SetPageTableAttributes() function in "UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c": This function consume "Cr4.Bits.LA57". But the patch does not touch this code location -- which is a sanity bug. The patch exploits the fact that, whenever this function is reached: SmiRendezvous() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c] BSPHandler() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c] PerformRemainingTasks() [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c] SetPageTableAttributes() [UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c] then the override is never in place, *in practice* (instead, IIUC, we're running on the new page tables, so CR4 actually reflects their depth). Exploiting that knowledge is wrong, because it further obscures the connection between the page table base, and the depth of the page tables. (We fetch CR4 directly, but go through GetPageTableBase() rather than grabbing CR3???) These two properties are inseparable. Allowing one property (the new Cr3) to propagate out of SmmInitPageTable() without being accompanied by the page tables depth is where it all went wrong in the first place. The gist of it is that every site that calls GetPageTableBase() must show *via syntax* that it has a dependency on *both* properties of the page tables (base, and depth). The patch is not wrong in the functional sense, but it's a disaster from the maintainability perspective. It makes a hack (built upon global variables) worse. I propose: - Change the prototype of GetPageTableBase() like this, in "UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h": VOID GetPageTable ( OUT UINTN *Base, OUT BOOLEAN *FiveLevel OPTIONAL ); - Make "mInternalCr3" an extern variable. - Make the implementation of GetPageTable() architecture-specific, in "UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c" and "UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c". IA32: VOID GetPageTable ( OUT UINTN *Base, OUT BOOLEAN *FiveLevels OPTIONAL ) { *Base = ((mInternalCr3 == 0) ? (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64) : mInternalCr3); if (FiveLevels != NULL) { *FiveLevels = FALSE; } } X64: VOID GetPageTable ( OUT UINTN *Base, OUT BOOLEAN *FiveLevels OPTIONAL ) { IA32_CR4 Cr4; if (mInternalCr3 == 0) { *Base = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64; if (FiveLevels != NULL) { Cr4.UintN = AsmReadCr4 (); *FiveLevels = (BOOLEAN)(Cr4.Bits.LA57 == 1); } return; } *Base = mInternalCr3; if (FiveLevels != NULL) { *FiveLevels = m5LevelPagingNeeded; } } - Update every GetPageTableBase() call site. In IA32 code, *or* when FiveLevels does not matter, pass NULL for FiveLevels. I feel that this proposal demonstrates better encapsulation; it makes it much harder for a caller to use such a page tables depth that does not match the page tables base. Additionally, the CR3/CR4 accesses, vs. the global variable (override) accesses, are tightly coupled in the appropriate branches. Anyway, if the UefiCpuPkg maintainers disagree with my proposal, I could tolerate the v4 patch set as well, now that I've spent several hours writing up this explanation myself. (It should have been the job of the patch author, really.) Thanis, Laszlo