[edk2-devel] [PATCH v4 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Return level paging type for Internal CR3
Sheng Wei
w.sheng at intel.com
Fri Nov 6 04:20:42 UTC 2020
Hi Laszlo,
Thank you for the detail analysis and propose.
I think it is much better than patch v4.
Please ignore my patch v5, I will update a new patch.
Thank you.
BR
Sheng Wei
> -----Original Message-----
> From: Laszlo Ersek <lersek at redhat.com>
> Sent: 2020年11月5日 11:04
> To: devel at edk2.groups.io; Sheng, W <w.sheng at intel.com>
> Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>; Kumar,
> Rahul1 <rahul1.kumar at intel.com>; Yao, Jiewen <jiewen.yao at intel.com>; Bret
> Barkelew <Bret.Barkelew at microsoft.com>
> Subject: Re: [edk2-devel] [PATCH v4 2/2] UefiCpuPkg/PiSmmCpuDxeSmm:
> Return level paging type for Internal CR3
>
> 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 <w.sheng at intel.com>
> > Cc: Eric Dong <eric.dong at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Laszlo Ersek <lersek at redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar at intel.com>
> > Cc: Jiewen Yao <jiewen.yao at intel.com>
> > ---
> > 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67075): https://edk2.groups.io/g/devel/message/67075
Mute This Topic: https://groups.io/mt/78044188/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list