[edk2-devel] [PATCH v6 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address
Laszlo Ersek
lersek at redhat.com
Tue Nov 10 19:25:23 UTC 2020
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 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/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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67248): https://edk2.groups.io/g/devel/message/67248
Mute This Topic: https://groups.io/mt/78152032/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