> v4 changes:
>MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 22 ++++++++++-----------
> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  2 +-
> UefiCpuPkg/CpuDxe/CpuPageTable.c                 | 25 +++++++++++-------------
> UefiCpuPkg/CpuDxe/CpuPageTable.h                 |  2 +-
> 4 files changed, 23 insertions(+), 28 deletions(-)
>
>diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>index 4ef3521224..038aa0d127 100644
>--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
>@@ -126,12 +126,13 @@ EnableExecuteDisableBit (
>   Initialize a buffer pool for page table use only.
> 
>   To reduce the potential split operation on page table, the pages reserved for
>-  page table should be allocated in the times of 512 (= SIZE_2MB) and at the
>-  boundary of SIZE_2MB. So the page pool is always initialized with number of
>-  pages greater than or equal to the given PoolPages.
>+  page table should be allocated in the times of PAGE_TABLE_POOL_UNIT_PAGES and
>+  at the boundary of PAGE_TABLE_POOL_ALIGNMENT. So the page pool is always
>+  initialized with number of pages greater than or equal to the given PoolPages.
> 
>   Once the pages in the pool are used up, this method should be called again to
>-  reserve at least another 512 pages. But usually this won't happen in practice.
>+  reserve at least another PAGE_TABLE_POOL_UNIT_PAGES. But usually this won't
>+  happen in practice.
> 
>   @param PoolPages  The least page number of the pool to be created.
> 
>@@ -146,15 +147,12 @@ InitializePageTablePool (
>   VOID          *Buffer;
> 
>   //
>-  // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES (512).
>+  // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
>+  // header.
>   //
>-  if (PoolPages <= PAGE_TABLE_POOL_UNIT_PAGES) {
>-    PoolPages = PAGE_TABLE_POOL_UNIT_PAGES;
>-  } else {
>-    PoolPages = ((PoolPages + PAGE_TABLE_POOL_UNIT_PAGES) %
>-                 PAGE_TABLE_POOL_UNIT_PAGES) * PAGE_TABLE_POOL_UNIT_PAGES;
>-  }
>-
>+  PoolPages += 1;   // Add one page for header.
>+  PoolPages = ((PoolPages - 1) / PAGE_TABLE_POOL_UNIT_PAGES + 1) *
>+              PAGE_TABLE_POOL_UNIT_PAGES;
>   Buffer = AllocateAlignedPages (PoolPages, PAGE_TABLE_POOL_ALIGNMENT);
>   if (Buffer == NULL) {
>     DEBUG ((DEBUG_ERROR, "ERROR: Out of aligned pages\r\n"));
>diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
>index 9f14ac6007..b8cf43104e 100644
>--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
>+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
>@@ -169,7 +169,7 @@ typedef union {
> 
> #define PAGE_TABLE_POOL_ALIGNMENT   BASE_2MB
> #define PAGE_TABLE_POOL_UNIT_SIZE   SIZE_2MB
>-#define PAGE_TABLE_POOL_UNIT_PAGES  EFI_SIZE_TO_PAGES (SIZE_2MB)
>+#define PAGE_TABLE_POOL_UNIT_PAGES  EFI_SIZE_TO_PAGES (PAGE_TABLE_POOL_UNIT_SIZE)
> #define PAGE_TABLE_POOL_ALIGN_MASK  \
>   (~(EFI_PHYSICAL_ADDRESS)(PAGE_TABLE_POOL_ALIGNMENT - 1))
> 
>diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>index 57ec76378a..a9c9bc9d5e 100644
>--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>@@ -1000,13 +1000,13 @@ RefreshGcdMemoryAttributesFromPaging (
>   Initialize a buffer pool for page table use only.
> 
>   To reduce the potential split operation on page table, the pages reserved for
>-  page table should be allocated in the times of 512 (= SIZE_2MB) and at the
>-  boundary of SIZE_2MB. So the page pool is always initialized with number of
>-  pages greater than or equal to the given PoolPages.
>+  page table should be allocated in the times of PAGE_TABLE_POOL_UNIT_PAGES and
>+  at the boundary of PAGE_TABLE_POOL_ALIGNMENT. So the page pool is always
>+  initialized with number of pages greater than or equal to the given PoolPages.
> 
>   Once the pages in the pool are used up, this method should be called again to
>-  reserve at least another 512 pages. Usually this won't happen often in
>-  practice.
>+  reserve at least another PAGE_TABLE_POOL_UNIT_PAGES. Usually this won't happen
>+  often in practice.
> 
>   @param[in] PoolPages      The least page number of the pool to be created.
> 
>@@ -1022,15 +1022,12 @@ InitializePageTablePool (
>   BOOLEAN                   IsModified;
> 
>   //
>-  // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES (512).
>+  // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
>+  // header.
>   //
>-  if (PoolPages <= PAGE_TABLE_POOL_UNIT_PAGES) {
>-    PoolPages = PAGE_TABLE_POOL_UNIT_PAGES;
>-  } else {
>-    PoolPages = ((PoolPages + PAGE_TABLE_POOL_UNIT_PAGES) %
>-                 PAGE_TABLE_POOL_UNIT_PAGES) * PAGE_TABLE_POOL_UNIT_PAGES;
>-  }
>-
>+  PoolPages += 1;   // Add one page for header.
>+  PoolPages = ((PoolPages - 1) / PAGE_TABLE_POOL_UNIT_PAGES + 1) *
>+              PAGE_TABLE_POOL_UNIT_PAGES;
>   Buffer = AllocateAlignedPages (PoolPages, PAGE_TABLE_POOL_ALIGNMENT);
>   if (Buffer == NULL) {
>     DEBUG ((DEBUG_ERROR, "ERROR: Out of aligned pages\r\n"));
>@@ -1139,7 +1136,7 @@ InitializePageTableLib (
>       (CurrentPagingContext.ContextData.Ia32.Attributes &
>        PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE) != 0) {
>     DisableReadOnlyPageWriteProtect ();
>-    InitializePageTablePool (PAGE_TABLE_POOL_UNIT_SIZE);
>+    InitializePageTablePool (1);
>     EnableReadOnlyPageWriteProtect ();
>   }
> 
>diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.h b/UefiCpuPkg/CpuDxe/CpuPageTable.h
>index 2557ffaecf..48da9cdde7 100644
>--- a/UefiCpuPkg/CpuDxe/CpuPageTable.h
>+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.h
>@@ -52,7 +52,7 @@ typedef struct {
> 
> #define PAGE_TABLE_POOL_ALIGNMENT   BASE_2MB
> #define PAGE_TABLE_POOL_UNIT_SIZE   SIZE_2MB
>-#define PAGE_TABLE_POOL_UNIT_PAGES  EFI_SIZE_TO_PAGES (SIZE_2MB)
>+#define PAGE_TABLE_POOL_UNIT_PAGES  EFI_SIZE_TO_PAGES (PAGE_TABLE_POOL_UNIT_SIZE)
> #define PAGE_TABLE_POOL_ALIGN_MASK  \
>   (~(EFI_PHYSICAL_ADDRESS)(PAGE_TABLE_POOL_ALIGNMENT - 1))
   
> v3 changes:
>  a. According to code review comments, remove the public definitions of
>     page table pool. Now the DxeIpl and CpuDxe will have their own page
>     table pool but in the same mechanism. Related PCDs, GUDI and headers
>     are also removed.
>  b. Apply protection to all page tables, including new ones added in
>     CpuDxe driver.
>  c. Code/comments cleanup.

> v2 changes:
>  a. Enable protection on any newly added page table after DxeIpl.
>  b. Introduce page table pool concept to make page table allocation
>     and protection easier and error free.

Write Protect feature (CR0.WP) is always enabled in driver UefiCpuPkg/CpuDxe.
But the memory pages used for page table are not set as read-only in the driver
DxeIplPeim, after the paging is setup. This might jeopardize the page table
integrity if there's buffer overflow occured in other part of system.

This patch series will change this situation by clearing R/W bit in page attribute
of the pages used as page table.

Validation works include booting Windows (10/server 2016) and Linux (Fedora/Ubuntu)
on OVMF and Intel real platform.

Jian J Wang (2):
  MdeModulePkg/DxeIpl: Mark page table as read-only
  UefiCpuPkg/CpuDxe: Enable protection for newly added page table

 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            |  34 +++
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |   8 +-
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 299 ++++++++++++++++++++++-
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  26 ++
 UefiCpuPkg/CpuDxe/CpuDxe.c                       |  17 +-
 UefiCpuPkg/CpuDxe/CpuDxe.h                       |   2 +
 UefiCpuPkg/CpuDxe/CpuPageTable.c                 | 223 ++++++++++++++++-
 UefiCpuPkg/CpuDxe/CpuPageTable.h                 |  34 +++
 8 files changed, 630 insertions(+), 13 deletions(-)

-- 
2.15.1.windows.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel