Re: [edk2-devel] [PATCH v6 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

["Laszlo Ersek" <lersek@redhat.com>]

On 11/10/20 03:24, Sheng Wei wrote:
> When trying to get page table base, if mInternalCr3 is zero, it will use
>  the page table from CR3, and reflect the page table depth by CR4 LA57 bit.
> If mInternalCr3 is non zero, it will use the page table from mInternalCr3
>  and reflect the page table depth of mInternalCr3 at same time.
> In the case of X64, we use m5LevelPagingNeeded to reflect the depth of
>  the page table. And in the case of IA32, it will not the page table depth
>  information.
> 
> This patch is a bug fix when enable CET feature with 5 level paging.
> The SMM page tables are allocated / initialized in PiCpuSmmEntry().
> When CET is enabled, PiCpuSmmEntry() must further modify the attribute of
>  shadow stack pages. This page table is not set to CR3 in PiCpuSmmEntry().
>  So the page table base address is set to mInternalCr3 for modifty the
>  page table attribute. It could not use CR4 LA57 bit to reflect the
>  page table depth for mInternalCr3.
> So we create a architecture-specific implementation GetPageTable() with
>  2 output parameters. One parameter is used to output the page table
>  address. Another parameter is used to reflect if it is 5 level paging
>  or not.
> 
> 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/Ia32/PageTbl.c           | 24 ++++++++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         | 12 ++++---
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 27 +++------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c            | 40 +++++++++++++++++++---
>  4 files changed, 70 insertions(+), 33 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index 2483f2ea84..f5d64392bd 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -10,6 +10,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  #include "PiSmmCpuDxeSmm.h"
>  
> +extern UINTN mInternalCr3;
> +

(1) This extern declaration (and its counterpart in the X64 source file)
both belong in the "PiSmmCpuDxeSmm.h" header.

>  /**
>    Disable CET.
>  **/
> @@ -28,6 +30,26 @@ EnableCet (
>    VOID
>    );
>  
> +/**
> +  Get page table base address and the depth of the page table.
> +
> +  @param[out] Base        Page table base address.
> +  @param[out] FiveLevels  TRUE means 5 level paging. FALSE means 4 level paging.
> +**/
> +VOID
> +GetPageTable (
> +  OUT UINTN   *Base,
> +  OUT BOOLEAN *FiveLevels OPTIONAL
> +  )
> +{
> +  *Base = ((mInternalCr3 == 0) ?
> +            (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64) :
> +            mInternalCr3);
> +  if (FiveLevels != NULL) {
> +    *FiveLevels = FALSE;
> +  }
> +}
> +
>  /**
>    Create PageTable for SMM use.
>  
> @@ -268,7 +290,7 @@ SetPageTableAttributes (
>      DEBUG ((DEBUG_INFO, "Start...\n"));
>      PageTableSplitted = FALSE;
>  
> -    L3PageTable = (UINT64 *)GetPageTableBase ();
> +    GetPageTable ((UINTN *)&L3PageTable, NULL);
>  
>      SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L3PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
>      PageTableSplitted = (PageTableSplitted || IsSplitted);

(2) Please introduce a helper variable instead;

  UINTN PageTableBase;

and write

  GetPageTable (&PageTableBase, NULL);
  L3PageTable = (UINT64 *)PageTableBase;

This is much cleaner.

There is a big conceptual difference between converting an integer to a
pointer, and reinterpreting the bit pattern of an integer as a pointer.

(3) Furthermore, if you look at the same context, you will see that we
have the expression a bit later:

  (EFI_PHYSICAL_ADDRESS)(UINTN)L3PageTable

and now you can simplify that to

  (EFI_PHYSICAL_ADDRESS)PageTableBase


> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 7fb3a2d9e4..59bc764140 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -942,13 +942,15 @@ SetPageTableAttributes (
>    );
>  
>  /**
> -  Return page table base.
> +  Get page table base address and the depth of the page table.
>  
> -  @return page table base.
> +  @param[out] Base        Page table base address.
> +  @param[out] FiveLevels  TRUE means 5 level paging. FALSE means 4 level paging.
>  **/
> -UINTN
> -GetPageTableBase (
> -  VOID
> +VOID
> +GetPageTable (
> +  OUT UINTN   *Base,
> +  OUT BOOLEAN *FiveLevels OPTIONAL
>    );
>  
>  /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index d67f036aea..fe71b77295 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -49,22 +49,6 @@ SetPageTableBase (
>    mInternalCr3 = Cr3;
>  }
>  
> -/**
> -  Return page table base.
> -
> -  @return page table base.
> -**/
> -UINTN
> -GetPageTableBase (
> -  VOID
> -  )
> -{
> -  if (mInternalCr3 != 0) {
> -    return mInternalCr3;
> -  }
> -  return (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
> -}
> -
>  /**
>    Return length according to page attributes.
>  
> @@ -131,8 +115,8 @@ GetPageTableEntry (
>    UINT64                *L3PageTable;
>    UINT64                *L4PageTable;
>    UINT64                *L5PageTable;
> -  IA32_CR4              Cr4;
>    BOOLEAN               Enable5LevelPaging;
> +  UINT64                *PageTableBase = NULL;

(4) Initialization of local variables (that are not static) is forbidden
by the edk2 coding style.

(5) The type of this variable should be UINTN, not (UINT64*).

>  
>    Index5 = ((UINTN)RShiftU64 (Address, 48)) & PAGING_PAE_INDEX_MASK;
>    Index4 = ((UINTN)RShiftU64 (Address, 39)) & PAGING_PAE_INDEX_MASK;
> @@ -140,12 +124,11 @@ 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);
> +  GetPageTable ((UINTN *)&PageTableBase, &Enable5LevelPaging);

(6) Please adjust the first argument according to (5) -- drop the
(UINTN*) cast.

>  
>    if (sizeof(UINTN) == sizeof(UINT64)) {
>      if (Enable5LevelPaging) {
> -      L5PageTable = (UINT64 *)GetPageTableBase ();
> +      L5PageTable = PageTableBase;
>        if (L5PageTable[Index5] == 0) {
>          *PageAttribute = PageNone;
>          return NULL;
> @@ -153,7 +136,7 @@ GetPageTableEntry (
>  
>        L4PageTable = (UINT64 *)(UINTN)(L5PageTable[Index5] & ~mAddressEncMask & PAGING_4K_ADDRESS_MASK_64);
>      } else {
> -      L4PageTable = (UINT64 *)GetPageTableBase ();
> +      L4PageTable = PageTableBase;
>      }
>      if (L4PageTable[Index4] == 0) {
>        *PageAttribute = PageNone;
> @@ -162,7 +145,7 @@ GetPageTableEntry (
>  
>      L3PageTable = (UINT64 *)(UINTN)(L4PageTable[Index4] & ~mAddressEncMask & PAGING_4K_ADDRESS_MASK_64);
>    } else {
> -    L3PageTable = (UINT64 *)GetPageTableBase ();
> +    L3PageTable = PageTableBase;
>    }
>    if (L3PageTable[Index3] == 0) {
>      *PageAttribute = PageNone;

(7) And so all three of these should be (UINT64 *)PageTableBase.


> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 810985df20..51469d3b88 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -19,6 +19,8 @@ BOOLEAN                             mCpuSmmRestrictedMemoryAccess;
>  BOOLEAN                             m5LevelPagingNeeded;
>  X86_ASSEMBLY_PATCH_LABEL            gPatch5LevelPagingNeeded;
>  
> +extern UINTN mInternalCr3;

(8) Belongs in "PiSmmCpuDxeSmm.h".

> +
>  /**
>    Disable CET.
>  **/
> @@ -104,6 +106,35 @@ Is5LevelPagingNeeded (
>    }
>  }
>  
> +/**
> +  Get page table base address and the depth of the page table.
> +
> +  @param[out] Base        Page table base address.
> +  @param[out] FiveLevels  TRUE means 5 level paging. FALSE means 4 level paging.
> +**/
> +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;
> +  }
> +}
> +
>  /**
>    Set sub-entries number in entry.
>  
> @@ -1114,11 +1145,10 @@ SetPageTableAttributes (
>    BOOLEAN               IsSplitted;
>    BOOLEAN               PageTableSplitted;
>    BOOLEAN               CetEnabled;
> -  IA32_CR4              Cr4;
>    BOOLEAN               Enable5LevelPaging;
> +  UINT64                *PageTableBase = NULL;

(9) Initialization of local variables (that are not static) is forbidden
by the edk2 coding style.

(10) The type of this variable should be UINTN, not (UINT64*).


>  
> -  Cr4.UintN = AsmReadCr4 ();
> -  Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> +  GetPageTable ((UINTN *)&PageTableBase, &Enable5LevelPaging);

(11) Please drop the (UINTN*) cast.

>  
>    //
>    // Don't mark page table memory as read-only if
> @@ -1164,7 +1194,7 @@ SetPageTableAttributes (
>      PageTableSplitted = FALSE;
>      L5PageTable = NULL;
>      if (Enable5LevelPaging) {
> -      L5PageTable = (UINT64 *)GetPageTableBase ();
> +      L5PageTable = PageTableBase;
>        SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L5PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
>        PageTableSplitted = (PageTableSplitted || IsSplitted);
>      }
> @@ -1176,7 +1206,7 @@ SetPageTableAttributes (
>            continue;
>          }
>        } else {
> -        L4PageTable = (UINT64 *)GetPageTableBase ();
> +        L4PageTable = PageTableBase;
>        }
>        SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L4PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
>        PageTableSplitted = (PageTableSplitted || IsSplitted);
> 

(12) Please add the necessary (UINT64*)PageTableBase casts (two of them).

(13) You can simplify

  (EFI_PHYSICAL_ADDRESS)(UINTN)L5PageTable

to

  (EFI_PHYSICAL_ADDRESS)PageTableBase

Thanks
Laszlo