[Patch V6 00/14] Use CpuPageTableLib to create and update smm page table

[Patch V6 00/14] Use CpuPageTableLib to create and update smm page table

[duntan]

In the V6 patch set:
In 'MdeModulePkg: Remove other attribute protection in UnsetGuardPage', add code to apply EFI_MEMORY_XP to the guarded page to be freed in UnsetGuardPage() if it happens after SmmReadyToLock. 

Only resend the patches in MdeModulePkg and OvmfPkg for further review. Other patches in UefiCpuPkg has been reviewed-by Ray.

Dun Tan (14):
  OvmfPkg:Remove code that apply AddressEncMask to non-leaf entry
  MdeModulePkg: Remove other attribute protection in UnsetGuardPage
  UefiCpuPkg: Use CpuPageTableLib to convert SMM paging attribute.
  UefiCpuPkg: Add DEBUG_CODE for special case when clear RP
  UefiCpuPkg/PiSmmCpuDxeSmm: Avoid setting non-present range to RO/NX
  UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP
  UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR0.WP before modify page table
  UefiCpuPkg: Extern mSmmShadowStackSize in PiSmmCpuDxeSmm.h
  UefiCpuPkg: Add GenSmmPageTable() to create smm page table
  UefiCpuPkg: Use GenSmmPageTable() to create Smm S3 page table
  UefiCpuPkg: Sort mSmmCpuSmramRanges in FindSmramInfo
  UefiCpuPkg: Sort mProtectionMemRange when ReadyToLock
  UefiCpuPkg: Refinement to smm runtime InitPaging() code
  UefiCpuPkg/PiSmmCpuDxeSmm: Remove unnecessary function

 MdeModulePkg/Core/PiSmmCore/HeapGuard.c                        |  16 +++++++++++++++-
 OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c |   6 +++---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c                       |   5 +++--
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c                  |   3 +--
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c                |   2 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c                          | 132 ------------------------------------------------------------------------------------------------------------------------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c                     |  40 ++++++++++++++++++++++++++++++++++++++--
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h                     | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf                   |   1 +
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c             | 795 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c                         | 324 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c                        | 229 ++++++++++++++++++++++++++++++++-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c                   |   3 +--
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c                 |  20 ++++----------------
 14 files changed, 682 insertions(+), 1015 deletions(-)

-- 
2.31.1.windows.1

[Patch V6 01/14] OvmfPkg:Remove code that apply AddressEncMask to non-leaf entry

[duntan]

Remove code that apply AddressEncMask to non-leaf entry when split
smm page table by MemEncryptSevLib. In FvbServicesSmm driver, it
calls MemEncryptSevClearMmioPageEncMask to clear AddressEncMask
bit in page table for a specific range. In AMD SEV feature, this
AddressEncMask bit in page table is used to indicate if the memory
is guest private memory or shared memory. But all memory used by
page table are treated as encrypted regardless of encryption bit.
So remove the EncMask bit for smm non-leaf page table entry
doesn't impact AMD SEV feature.
If page split happens in the AddressEncMask bit clear process,
there will be some new non-leaf entries with AddressEncMask
applied in smm page table. When ReadyToLock, code in PiSmmCpuDxe
module will use CpuPageTableLib to modify smm page table. So
remove code to apply AddressEncMask for new non-leaf entries
since CpuPageTableLib doesn't consume the EncMask PCD.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
---
 OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
index cf2441b551..aba2e8c081 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
@@ -233,7 +233,7 @@ Split2MPageTo4K (
   // Fill in 2M page entry.
   //
   *PageEntry2M = ((UINT64)(UINTN)PageTableEntry1 |
-                  IA32_PG_P | IA32_PG_RW | AddressEncMask);
+                  IA32_PG_P | IA32_PG_RW);
 }
 
 /**
@@ -352,7 +352,7 @@ SetPageTablePoolReadOnly (
         PhysicalAddress += LevelSize[Level - 1];
       }
 
-      PageTable[Index] = (UINT64)(UINTN)NewPageTable | AddressEncMask |
+      PageTable[Index] = (UINT64)(UINTN)NewPageTable |
                          IA32_PG_P | IA32_PG_RW;
       PageTable = NewPageTable;
     }
@@ -440,7 +440,7 @@ Split1GPageTo2M (
   // Fill in 1G page entry.
   //
   *PageEntry1G = ((UINT64)(UINTN)PageDirectoryEntry |
-                  IA32_PG_P | IA32_PG_RW | AddressEncMask);
+                  IA32_PG_P | IA32_PG_RW);
 
   PhysicalAddress2M = PhysicalAddress;
   for (IndexOfPageDirectoryEntries = 0;
-- 
2.31.1.windows.1

[Patch V6 02/14] MdeModulePkg: Remove other attribute protection in UnsetGuardPage

[duntan]

In UnsetGuardPage(), before SmmReadyToLock, remove NX and RO
memory attribute protection for guarded page since
EfiConventionalMemory in SMRAM is RW and executable before
SmmReadyToLock. If UnsetGuardPage() happens after SmmReadyToLock,
then apply EFI_MEMORY_XP to the guarded page to make sure
EfiConventionalMemory in SMRAM is NX since EfiConventionalMemory
in SMRAM is marked as NX in PiSmmCpuDxe driver when SmmReadyToLock.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
---
 MdeModulePkg/Core/PiSmmCore/HeapGuard.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
index 8f3bab6fee..25310122ca 100644
--- a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
+++ b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
@@ -553,9 +553,23 @@ UnsetGuardPage (
                                          mSmmMemoryAttribute,
                                          BaseAddress,
                                          EFI_PAGE_SIZE,
-                                         EFI_MEMORY_RP
+                                         EFI_MEMORY_RP|EFI_MEMORY_RO|EFI_MEMORY_XP
                                          );
     ASSERT_EFI_ERROR (Status);
+
+    if (gST == NULL) {
+      //
+      // Make sure EfiConventionalMemory is NX after SmmReadyToLock
+      //
+      Status = mSmmMemoryAttribute->SetMemoryAttributes (
+                                      mSmmMemoryAttribute,
+                                      BaseAddress,
+                                      EFI_PAGE_SIZE,
+                                      EFI_MEMORY_XP
+                                      );
+      ASSERT_EFI_ERROR (Status);
+    }
+
     mOnGuarding = FALSE;
   }
 }
-- 
2.31.1.windows.1

Re: [edk2-devel] [Patch V6 01/14] OvmfPkg:Remove code that apply AddressEncMask to non-leaf entry

[duntan]

Hi all,

Could you please help to review this patch?

Thanks,
Dun

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan
Sent: Friday, June 9, 2023 5:16 PM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Tom Lendacky <thomas.lendacky@amd.com>; Ni, Ray <ray.ni@intel.com>
Subject: [edk2-devel] [Patch V6 01/14] OvmfPkg:Remove code that apply AddressEncMask to non-leaf entry

Remove code that apply AddressEncMask to non-leaf entry when split smm page table by MemEncryptSevLib. In FvbServicesSmm driver, it calls MemEncryptSevClearMmioPageEncMask to clear AddressEncMask bit in page table for a specific range. In AMD SEV feature, this AddressEncMask bit in page table is used to indicate if the memory is guest private memory or shared memory. But all memory used by page table are treated as encrypted regardless of encryption bit.
So remove the EncMask bit for smm non-leaf page table entry doesn't impact AMD SEV feature.
If page split happens in the AddressEncMask bit clear process, there will be some new non-leaf entries with AddressEncMask applied in smm page table. When ReadyToLock, code in PiSmmCpuDxe module will use CpuPageTableLib to modify smm page table. So remove code to apply AddressEncMask for new non-leaf entries since CpuPageTableLib doesn't consume the EncMask PCD.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
---
 OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
index cf2441b551..aba2e8c081 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
@@ -233,7 +233,7 @@ Split2MPageTo4K (
   // Fill in 2M page entry.
   //
   *PageEntry2M = ((UINT64)(UINTN)PageTableEntry1 |
-                  IA32_PG_P | IA32_PG_RW | AddressEncMask);
+                  IA32_PG_P | IA32_PG_RW);
 }
 
 /**
@@ -352,7 +352,7 @@ SetPageTablePoolReadOnly (
         PhysicalAddress += LevelSize[Level - 1];
       }
 
-      PageTable[Index] = (UINT64)(UINTN)NewPageTable | AddressEncMask |
+      PageTable[Index] = (UINT64)(UINTN)NewPageTable |
                          IA32_PG_P | IA32_PG_RW;
       PageTable = NewPageTable;
     }
@@ -440,7 +440,7 @@ Split1GPageTo2M (
   // Fill in 1G page entry.
   //
   *PageEntry1G = ((UINT64)(UINTN)PageDirectoryEntry |
-                  IA32_PG_P | IA32_PG_RW | AddressEncMask);
+                  IA32_PG_P | IA32_PG_RW);
 
   PhysicalAddress2M = PhysicalAddress;
   for (IndexOfPageDirectoryEntries = 0;
--
2.31.1.windows.1

Re: [edk2-devel] [Patch V6 02/14] MdeModulePkg: Remove other attribute protection in UnsetGuardPage

[duntan]

Hi all,

Could you please help to review this patch?

Thanks,
Dun

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan
Sent: Friday, June 9, 2023 5:16 PM
To: devel@edk2.groups.io
Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: [edk2-devel] [Patch V6 02/14] MdeModulePkg: Remove other attribute protection in UnsetGuardPage

In UnsetGuardPage(), before SmmReadyToLock, remove NX and RO memory attribute protection for guarded page since EfiConventionalMemory in SMRAM is RW and executable before SmmReadyToLock. If UnsetGuardPage() happens after SmmReadyToLock, then apply EFI_MEMORY_XP to the guarded page to make sure EfiConventionalMemory in SMRAM is NX since EfiConventionalMemory in SMRAM is marked as NX in PiSmmCpuDxe driver when SmmReadyToLock.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
---
 MdeModulePkg/Core/PiSmmCore/HeapGuard.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
index 8f3bab6fee..25310122ca 100644
--- a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
+++ b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
@@ -553,9 +553,23 @@ UnsetGuardPage (
                                          mSmmMemoryAttribute,
                                          BaseAddress,
                                          EFI_PAGE_SIZE,
-                                         EFI_MEMORY_RP
+                                         
+ EFI_MEMORY_RP|EFI_MEMORY_RO|EFI_MEMORY_XP
                                          );
     ASSERT_EFI_ERROR (Status);
+
+    if (gST == NULL) {
+      //
+      // Make sure EfiConventionalMemory is NX after SmmReadyToLock
+      //
+      Status = mSmmMemoryAttribute->SetMemoryAttributes (
+                                      mSmmMemoryAttribute,
+                                      BaseAddress,
+                                      EFI_PAGE_SIZE,
+                                      EFI_MEMORY_XP
+                                      );
+      ASSERT_EFI_ERROR (Status);
+    }
+
     mOnGuarding = FALSE;
   }
 }
--
2.31.1.windows.1

Re: [edk2-devel] [Patch V6 02/14] MdeModulePkg: Remove other attribute protection in UnsetGuardPage

[Wang, Jian J]

Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

Regards,
Jian



> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan
> Sent: Friday, June 09, 2023 5:16 PM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>
> Subject: [edk2-devel] [Patch V6 02/14] MdeModulePkg: Remove other attribute
> protection in UnsetGuardPage
> 
> In UnsetGuardPage(), before SmmReadyToLock, remove NX and RO
> memory attribute protection for guarded page since
> EfiConventionalMemory in SMRAM is RW and executable before
> SmmReadyToLock. If UnsetGuardPage() happens after SmmReadyToLock,
> then apply EFI_MEMORY_XP to the guarded page to make sure
> EfiConventionalMemory in SMRAM is NX since EfiConventionalMemory
> in SMRAM is marked as NX in PiSmmCpuDxe driver when SmmReadyToLock.
> 
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> ---
>  MdeModulePkg/Core/PiSmmCore/HeapGuard.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
> b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
> index 8f3bab6fee..25310122ca 100644
> --- a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
> +++ b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
> @@ -553,9 +553,23 @@ UnsetGuardPage (
>                                           mSmmMemoryAttribute,
>                                           BaseAddress,
>                                           EFI_PAGE_SIZE,
> -                                         EFI_MEMORY_RP
> +                                         EFI_MEMORY_RP|EFI_MEMORY_RO|EFI_MEMORY_XP
>                                           );
>      ASSERT_EFI_ERROR (Status);
> +
> +    if (gST == NULL) {
> +      //
> +      // Make sure EfiConventionalMemory is NX after SmmReadyToLock
> +      //
> +      Status = mSmmMemoryAttribute->SetMemoryAttributes (
> +                                      mSmmMemoryAttribute,
> +                                      BaseAddress,
> +                                      EFI_PAGE_SIZE,
> +                                      EFI_MEMORY_XP
> +                                      );
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +
>      mOnGuarding = FALSE;
>    }
>  }
> --
> 2.31.1.windows.1
> 
> 
> 
> 
>