Re: [PATCH v4 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Correct the Cr3 typo

Re: [PATCH v4 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Correct the Cr3 typo

[Laszlo Ersek]

On 11/03/20 05:30, Sheng Wei wrote:
> Change the variable name from mInternalGr3 to mInternalCr3.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3015
> 
> Signed-off-by: Sheng Wei <w.sheng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index ebfc46ad45..d67f036aea 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -32,7 +32,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
>    {Page1G,  SIZE_1GB, PAGING_1G_ADDRESS_MASK_64},
>  };
>  
> -UINTN  mInternalGr3;
> +UINTN  mInternalCr3;
>  
>  /**
>    Set the internal page table base address.
> @@ -46,7 +46,7 @@ SetPageTableBase (
>    IN UINTN   Cr3
>    )
>  {
> -  mInternalGr3 = Cr3;
> +  mInternalCr3 = Cr3;
>  }
>  
>  /**
> @@ -59,8 +59,8 @@ GetPageTableBase (
>    VOID
>    )
>  {
> -  if (mInternalGr3 != 0) {
> -    return mInternalGr3;
> +  if (mInternalCr3 != 0) {
> +    return mInternalCr3;
>    }
>    return (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
>  }
> @@ -252,7 +252,7 @@ ConvertPageEntryAttribute (
>    if ((Attributes & EFI_MEMORY_RO) != 0) {
>      if (IsSet) {
>        NewPageEntry &= ~(UINT64)IA32_PG_RW;
> -      if (mInternalGr3 != 0) {
> +      if (mInternalCr3 != 0) {
>          // Environment setup
>          // ReadOnly page need set Dirty bit for shadow stack
>          NewPageEntry |= IA32_PG_D;
> 

You should have picked up my

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

from <https://edk2.groups.io/g/devel/message/66872>.

Laszlo

Re: [edk2-devel] [PATCH v4 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Return level paging type for Internal CR3

[Laszlo Ersek]

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@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Jiewen Yao <jiewen.yao@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