[PATCH v6 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

[PATCH v6 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

[Sheng Wei]

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.

Correct the Cr3 typo

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>

Sheng Wei (2):
  UefiCpuPkg/PiSmmCpuDxeSmm: Correct the Cr3 typo
  UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table
    address

 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           | 24 ++++++++++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         | 12 ++++---
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 33 +++++-------------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c            | 40 +++++++++++++++++++---
 4 files changed, 73 insertions(+), 36 deletions(-)

-- 
2.16.2.windows.1

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

[Sheng Wei]

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;
-- 
2.16.2.windows.1

Re: [edk2-devel] [PATCH v6 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Correct the Cr3 typo

[Laszlo Ersek]

Wei,

can you please explain why you didn't pick up my R-b, which I have given
*twice* for this patch?

https://edk2.groups.io/g/devel/message/66872
https://edk2.groups.io/g/devel/message/67006

Do you understand what it means to "pick up an R-b"?

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-28

Laszlo

On 11/10/20 03:24, 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;
> 

Re: [edk2-devel] [PATCH v6 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Correct the Cr3 typo

[Sheng Wei]

Hi Laszlo,
Thank you.
I will add the R-b part.
BR
Sheng Wei

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: 2020年11月11日 3:05
> To: devel@edk2.groups.io; Sheng, W <w.sheng@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [edk2-devel] [PATCH v6 1/2] UefiCpuPkg/PiSmmCpuDxeSmm:
> Correct the Cr3 typo
> 
> Wei,
> 
> can you please explain why you didn't pick up my R-b, which I have given
> *twice* for this patch?
> 
> https://edk2.groups.io/g/devel/message/66872
> https://edk2.groups.io/g/devel/message/67006
> 
> Do you understand what it means to "pick up an R-b"?
> 
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo%27s-unkempt-git-
> guide-for-edk2-contributors-and-maintainers#contrib-28
> 
> Laszlo
> 
> On 11/10/20 03:24, 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;
> >

[PATCH v6 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address

[Sheng Wei]

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;
+
 /**
   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);
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;
 
   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);
 
   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;
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;
+
 /**
   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;
 
-  Cr4.UintN = AsmReadCr4 ();
-  Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
+  GetPageTable ((UINTN *)&PageTableBase, &Enable5LevelPaging);
 
   //
   // 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);
-- 
2.16.2.windows.1

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

[Laszlo Ersek]

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

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

[Sheng Wei]

Hi Laszlo,
Thank you for the review comments
I will update the patch.
BR
Sheng Wei

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: 2020年11月11日 3:25
> To: devel@edk2.groups.io; Sheng, W <w.sheng@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [edk2-devel] [PATCH v6 2/2] UefiCpuPkg/PiSmmCpuDxeSmm:
> Reflect page table depth with page table address
> 
> 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

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

[Laszlo Ersek]

On 11/10/20 03:24, Sheng Wei wrote:

> @@ -1114,11 +1145,10 @@ SetPageTableAttributes (
>    BOOLEAN               IsSplitted;
>    BOOLEAN               PageTableSplitted;
>    BOOLEAN               CetEnabled;
> -  IA32_CR4              Cr4;
>    BOOLEAN               Enable5LevelPaging;
> +  UINT64                *PageTableBase = NULL;
>
> -  Cr4.UintN = AsmReadCr4 ();
> -  Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> +  GetPageTable ((UINTN *)&PageTableBase, &Enable5LevelPaging);
>
>    //
>    // 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);
>

(14) Are you sure that we never need to re-read the CR3 register inside
the outermost loop?

I would feel safer if you didn't place the GetPageTable() call at the
top of the function (replacing the AsmReadCr4() call). Instead, I
suggest placing GetPageTable() near the top of the outermost loop (the
one that is repeated as long as we split pages).

Here's what I suggest, for "UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c":

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 810985df20ae..52b8eac9cdaf 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -1112,16 +1112,13 @@ SetPageTableAttributes (
>    UINT64                *L4PageTable;
>    UINT64                *L5PageTable;
>    BOOLEAN               IsSplitted;
>    BOOLEAN               PageTableSplitted;
>    BOOLEAN               CetEnabled;
> -  IA32_CR4              Cr4;
> +  UINTN                 PageTableBase;
>    BOOLEAN               Enable5LevelPaging;
>
> -  Cr4.UintN = AsmReadCr4 ();
> -  Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> -
>    //
>    // Don't mark page table memory as read-only if
>    //  - no restriction on access to non-SMRAM memory; or
>    //  - SMM heap guard feature enabled; or
>    //      BIT2: SMM page guard enabled
> @@ -1161,24 +1158,27 @@ SetPageTableAttributes (
>
>    do {
>      DEBUG ((DEBUG_INFO, "Start...\n"));
>      PageTableSplitted = FALSE;
>      L5PageTable = NULL;
> +
> +    GetPageTableBase (&PageTableBase, &Enable5LevelPaging);
> +
>      if (Enable5LevelPaging) {
> -      L5PageTable = (UINT64 *)GetPageTableBase ();
> -      SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L5PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
> +      L5PageTable = (UINT64 *)PageTableBase;
> +      SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)PageTableBase, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
>        PageTableSplitted = (PageTableSplitted || IsSplitted);
>      }
>
>      for (Index5 = 0; Index5 < (Enable5LevelPaging ? SIZE_4KB/sizeof(UINT64) : 1); Index5++) {
>        if (Enable5LevelPaging) {
>          L4PageTable = (UINT64 *)(UINTN)(L5PageTable[Index5] & ~mAddressEncMask & PAGING_4K_ADDRESS_MASK_64);
>          if (L4PageTable == NULL) {
>            continue;
>          }
>        } else {
> -        L4PageTable = (UINT64 *)GetPageTableBase ();
> +        L4PageTable = (UINT64 *)PageTableBase;
>        }
>        SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L4PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted);
>        PageTableSplitted = (PageTableSplitted || IsSplitted);
>
>        for (Index4 = 0; Index4 < SIZE_4KB/sizeof(UINT64); Index4++) {

This way, we preserve the pre-patch behavior that the page table base is
re-read *exactly once* per outermost loop iteration.

My proposal matches the IA32 version more closely as well -- in the IA32
version too, GetPageTableBase() is called near the top of the outermost
loop.

It is true that we re-read "Enable5LevelPaging" as well once per
outermost iteration, but (IMO) that's fine. It's a consequence of
binding "PageTableBase" and "Enable5LevelPaging" together (which is a
good thing), and re-reading "Enable5LevelPaging" once per outermost
iteration is not costly.

Thanks
Laszlo