[PATCH 0/2] Enable page table write protection

[PATCH 0/2] Enable page table write protection

[Jian J Wang]

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):
  UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
  MdeModulePkg/DxeIpl: Mark page table as read-only

 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166 +++++++++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
 UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65 ++++++++-
 3 files changed, 241 insertions(+), 4 deletions(-)

-- 
2.14.1.windows.1

[PATCH 1/2] UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table

[Jian J Wang]

The CPU driver will always set CR0.WP if paging is enabled.

GetCurrentPagingContext():
  if ((AsmReadCr0 () & BIT31) != 0) {
    PagingContext->ContextData.X64.PageTableBase = (AsmReadCr3 () &
                                        PAGING_4K_ADDRESS_MASK_64);
    if ((AsmReadCr0 () & BIT16) == 0) {
      AsmWriteCr0 (AsmReadCr0 () | BIT16);
      SyncMemoryPageAttributesAp (SyncCpuEnableWriteProtection);
    }
  } else {

Before this patch, there's no driver to set page attribute of memory
used for page table to be "read-only". CR0.WP will not prevent the page
attributes from updating.

Since this patch, the pages used for page table will be set as "read-only"
to protect them from corruption caused by buffer overflow issue. In this
situation, CR0.WP must be cleared before updating page table. CR0.WP must
be set again afterwards.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 65 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index 9658ed74c5..dd0debb448 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -561,6 +561,43 @@ SplitPage (
   }
 }
 
+/**
+ Check the WP status in CR0 register. This bit is used to lock or unlock write
+ access to pages marked as read-only.
+
+  @retval TRUE    Write protection is enabled.
+  @retval FALSE   Write protection is disabled.
+**/
+BOOLEAN
+IsReadOnlyPageWriteProtected (
+  VOID
+  )
+{
+  return ((AsmReadCr0 () & BIT16) != 0);
+}
+
+/**
+ Disable Write Protect on pages marked as read-only.  
+**/
+VOID
+DisableReadOnlyPageWriteProtect (
+  VOID
+  )
+{
+  AsmWriteCr0 (AsmReadCr0() & ~BIT16);
+}
+
+/**
+ Enable Write Protect on pages marked as read-only.
+**/
+VOID
+EnableReadOnlyPageWriteProtect (
+  VOID
+  )
+{
+  AsmWriteCr0 (AsmReadCr0() | BIT16);
+}
+
 /**
   This function modifies the page attributes for the memory region specified by BaseAddress and
   Length from their current attributes to the attributes specified by Attributes.
@@ -609,6 +646,7 @@ ConvertMemoryPageAttributes (
   PAGE_ATTRIBUTE                    SplitAttribute;
   RETURN_STATUS                     Status;
   BOOLEAN                           IsEntryModified;
+  BOOLEAN                           IsWpEnabled;
 
   if ((BaseAddress & (SIZE_4KB - 1)) != 0) {
     DEBUG ((DEBUG_ERROR, "BaseAddress(0x%lx) is not aligned!\n", BaseAddress));
@@ -666,13 +704,23 @@ ConvertMemoryPageAttributes (
     *IsModified = FALSE;
   }
 
+  //
+  // Make sure that the page table is changeable.
+  //
+  IsWpEnabled = IsReadOnlyPageWriteProtected ();
+  if (IsWpEnabled) {
+    DisableReadOnlyPageWriteProtect ();
+  }
+
   //
   // Below logic is to check 2M/4K page to make sure we donot waist memory.
   //
+  Status = EFI_SUCCESS;
   while (Length != 0) {
     PageEntry = GetPageTableEntry (&CurrentPagingContext, BaseAddress, &PageAttribute);
     if (PageEntry == NULL) {
-      return RETURN_UNSUPPORTED;
+      Status = RETURN_UNSUPPORTED;
+      goto Done;
     }
     PageEntryLength = PageAttributeToLength (PageAttribute);
     SplitAttribute = NeedSplitPage (BaseAddress, Length, PageEntry, PageAttribute);
@@ -690,11 +738,13 @@ ConvertMemoryPageAttributes (
       Length -= PageEntryLength;
     } else {
       if (AllocatePagesFunc == NULL) {
-        return RETURN_UNSUPPORTED;
+        Status = RETURN_UNSUPPORTED;
+        goto Done;
       }
       Status = SplitPage (PageEntry, PageAttribute, SplitAttribute, AllocatePagesFunc);
       if (RETURN_ERROR (Status)) {
-        return RETURN_UNSUPPORTED;
+        Status = RETURN_UNSUPPORTED;
+        goto Done;
       }
       if (IsSplitted != NULL) {
         *IsSplitted = TRUE;
@@ -709,7 +759,14 @@ ConvertMemoryPageAttributes (
     }
   }
 
-  return RETURN_SUCCESS;
+Done:
+  //
+  // Restore page table write protection, if any.
+  //
+  if (IsWpEnabled) {
+    EnableReadOnlyPageWriteProtect ();
+  }
+  return Status;
 }
 
 /**
-- 
2.14.1.windows.1

[PATCH 2/2] MdeModulePkg/DxeIpl: Mark page table as read-only

[Jian J Wang]

This patch will set the memory pages used for page table as read-only
memory after the paging is setup. CR0.WP must set to let it take into
effect.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166 +++++++++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
 2 files changed, 180 insertions(+)

diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index 29b6205e88..7a859606c6 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -234,6 +234,166 @@ Split1GPageTo2M (
   }
 }
 
+/**
+  Set one page (4KB) of memory to be read-only.
+
+  @param[in] PageTableBase    Base address of page table (CR3).
+  @param[in] Address          Start address of a page to be set as read-only.
+
+**/
+VOID
+SetPageReadOnly (
+  IN  UINTN                             PageTableBase,
+  IN  PHYSICAL_ADDRESS                  Address
+  )
+{
+  UINTN                 Index;
+  UINTN                 Index1;
+  UINTN                 Index2;
+  UINTN                 Index3;
+  UINTN                 Index4;
+  UINT64                *L1PageTable;
+  UINT64                *L2PageTable;
+  UINT64                *L3PageTable;
+  UINT64                *L4PageTable;
+  UINT64                AddressEncMask;
+  PHYSICAL_ADDRESS      PhysicalAddress;
+
+  ASSERT (PageTableBase != 0);
+
+  Index4 = ((UINTN)RShiftU64 (Address, PAGING_L4_ADDRESS_SHIFT)) &
+           PAGING_PAE_INDEX_MASK;
+  ASSERT (Index4 < PAGING_PML4E_NUMBER);
+
+  Index3 = ((UINTN)Address >> PAGING_L3_ADDRESS_SHIFT) & PAGING_PAE_INDEX_MASK;
+  Index2 = ((UINTN)Address >> PAGING_L2_ADDRESS_SHIFT) & PAGING_PAE_INDEX_MASK;
+  Index1 = ((UINTN)Address >> PAGING_L1_ADDRESS_SHIFT) & PAGING_PAE_INDEX_MASK;
+
+  //
+  // Make sure AddressEncMask is contained to smallest supported address field.
+  //
+  AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) &
+                   PAGING_1G_ADDRESS_MASK_64;
+
+  L4PageTable = (UINT64 *)(UINTN)PageTableBase;
+  L3PageTable = (UINT64 *)(UINTN)(L4PageTable[Index4] & ~AddressEncMask &
+                                  PAGING_4K_ADDRESS_MASK_64);
+  if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
+    // 1G page. Split to 2M.
+    L2PageTable = AllocatePages (1);
+    ASSERT (L2PageTable != NULL);
+
+    PhysicalAddress = L3PageTable[Index3] & PAGING_1G_ADDRESS_MASK_64;
+    for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
+      L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
+                           IA32_PG_PS | IA32_PG_P | IA32_PG_RW;
+      PhysicalAddress += SIZE_2MB;
+    }
+
+    L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable | AddressEncMask |
+                                           IA32_PG_P | IA32_PG_RW;
+    SetPageReadOnly (PageTableBase, (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
+  }
+
+  L2PageTable = (UINT64 *)(UINTN)(L3PageTable[Index3] & ~AddressEncMask &
+                                  PAGING_4K_ADDRESS_MASK_64);
+  if ((L2PageTable[Index2] & IA32_PG_PS) != 0) {
+    // 2M page. Split to 4K.
+    L1PageTable = AllocatePages (1);
+    ASSERT (L1PageTable != NULL);
+
+    PhysicalAddress = L2PageTable[Index2] & PAGING_2M_ADDRESS_MASK_64;
+    for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
+      L1PageTable[Index] = PhysicalAddress  | AddressEncMask |
+                           IA32_PG_P | IA32_PG_RW;
+      PhysicalAddress += SIZE_4KB;
+    }
+
+    L2PageTable[Index2] = (UINT64)(UINTN)L1PageTable | AddressEncMask |
+                                         IA32_PG_P | IA32_PG_RW;
+    SetPageReadOnly (PageTableBase, (EFI_PHYSICAL_ADDRESS)(UINTN)L1PageTable);
+  }
+
+  // 4k
+  L1PageTable = (UINT64 *)(UINTN)(L2PageTable[Index2] & ~AddressEncMask &
+                                  PAGING_4K_ADDRESS_MASK_64);
+  L1PageTable[Index1] &= ~IA32_PG_RW;
+}
+
+/**
+  Prevent the memory pages used for page table from been overwritten.
+
+  @param[in] PageTableBase    Base address of page table (CR3).
+
+**/
+VOID
+EnablePageTableProtection (
+  IN UINTN      PageTableBase
+  )
+{
+  UINTN                 Index2;
+  UINTN                 Index3;
+  UINTN                 Index4;
+  UINT64                *L1PageTable;
+  UINT64                *L2PageTable;
+  UINT64                *L3PageTable;
+  UINT64                *L4PageTable;
+  UINT64                AddressEncMask;
+
+  //
+  // Disable write protection, because we need to mark page table to be write 
+  // protected.
+  //
+  AsmWriteCr0 (AsmReadCr0() & ~CR0_WP);
+
+  AddressEncMask = PcdGet64 (PcdPteMemoryEncryptionAddressOrMask) &
+                   PAGING_1G_ADDRESS_MASK_64;
+  L4PageTable = (UINT64 *)PageTableBase;
+  SetPageReadOnly (PageTableBase, (EFI_PHYSICAL_ADDRESS)(UINTN)L4PageTable);
+
+  for (Index4 = 0; Index4 < PAGING_PML4E_NUMBER; Index4++) {
+    L3PageTable = (UINT64 *)(UINTN)(L4PageTable[Index4] & ~AddressEncMask &
+                                    PAGING_4K_ADDRESS_MASK_64);
+    if (L3PageTable == NULL) {
+      continue;
+    }
+    SetPageReadOnly (PageTableBase, (EFI_PHYSICAL_ADDRESS)(UINTN)L3PageTable);
+
+    for (Index3 = 0; Index3 < EFI_PAGE_SIZE/sizeof(UINT64); Index3++) {
+      if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
+        // 1G
+        continue;
+      }
+
+      L2PageTable = (UINT64 *)(UINTN)(L3PageTable[Index3] & ~AddressEncMask &
+                                      PAGING_4K_ADDRESS_MASK_64);
+      if (L2PageTable == NULL) {
+        continue;
+      }
+      SetPageReadOnly (PageTableBase, (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
+
+      for (Index2 = 0; Index2 < EFI_PAGE_SIZE/sizeof(UINT64); Index2++) {
+        if ((L2PageTable[Index2] & IA32_PG_PS) != 0) {
+          // 2M
+          continue;
+        }
+
+        L1PageTable = (UINT64 *)(UINTN)(L2PageTable[Index2] & ~AddressEncMask &
+                                        PAGING_4K_ADDRESS_MASK_64);
+        if (L1PageTable == NULL) {
+          continue;
+        }
+        SetPageReadOnly (PageTableBase, (EFI_PHYSICAL_ADDRESS)(UINTN)L1PageTable);
+      }
+    }
+  }
+
+  //
+  // Enable write protection, after page table updated.
+  //
+  AsmWriteCr0 (AsmReadCr0() | CR0_WP);
+}
+
 /**
   Allocates and fills in the Page Directory and Page Table Entries to
   establish a 1:1 Virtual to Physical mapping.
@@ -430,6 +590,12 @@ CreateIdentityMappingPageTables (
       );
   }
 
+  //
+  // Protect the page table by marking the memory used for page table to be
+  // read-only.
+  //
+  EnablePageTableProtection ((UINTN)PageMap);
+
   if (PcdGetBool (PcdSetNxForStack)) {
     EnableExecuteDisableBit ();
   }
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
index 7c9bb49e3e..6d1961b6f8 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
@@ -148,11 +148,25 @@ typedef union {
 
 #pragma pack()
 
+#define CR0_WP                      BIT16
+
 #define IA32_PG_P                   BIT0
 #define IA32_PG_RW                  BIT1
+#define IA32_PG_PS                  BIT7
+
+#define PAGING_PAE_INDEX_MASK       0x1FF
 
+#define PAGING_4K_ADDRESS_MASK_64   0x000FFFFFFFFFF000ull
+#define PAGING_2M_ADDRESS_MASK_64   0x000FFFFFFFE00000ull
 #define PAGING_1G_ADDRESS_MASK_64   0x000FFFFFC0000000ull
 
+#define PAGING_L1_ADDRESS_SHIFT     12
+#define PAGING_L2_ADDRESS_SHIFT     21
+#define PAGING_L3_ADDRESS_SHIFT     30
+#define PAGING_L4_ADDRESS_SHIFT     39
+
+#define PAGING_PML4E_NUMBER         4
+
 /**
   Enable Execute Disable Bit.
 
-- 
2.14.1.windows.1

Re: [PATCH 0/2] Enable page table write protection

[Yao, Jiewen]

Thanks.

May I know if this is validated in uefi shell, that all page table is readonly?

I did not find the code to set new allocated split page to be readonly. Can you give me a hand on that?

thank you!
Yao, Jiewen


> 在 2017年11月29日，下午4:47，Jian J Wang <jian.j.wang@intel.com> 写道：
> 
> 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):
> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
> MdeModulePkg/DxeIpl: Mark page table as read-only
> 
> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166 +++++++++++++++++++++++
> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65 ++++++++-
> 3 files changed, 241 insertions(+), 4 deletions(-)
> 
> -- 
> 2.14.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Wang, Jian J]

Yes, I validated them manually with JTAG debug tool. 

 if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
   // 1G page. Split to 2M.
   L2PageTable = AllocatePages (1);
   ASSERT (L2PageTable != NULL);
    PhysicalAddress = L3PageTable[Index3] & PAGING_1G_ADDRESS_MASK_64;
    for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
     L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
                          IA32_PG_PS | IA32_PG_P | IA32_PG_RW;
     PhysicalAddress += SIZE_2MB;
   }
   L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable | AddressEncMask |
                                          IA32_PG_P | IA32_PG_RW;
   SetPageReadOnly (PageTableBase, (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
 }

The newly allocated page table is set in the SetPageReadOnly() itself recursively, like
above code in which L2PageTable is allocated and then set it to be read-only after
initializing the table content.

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, November 29, 2017 5:16 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> 
> Thanks.
> 
> May I know if this is validated in uefi shell, that all page table is readonly?
> 
> I did not find the code to set new allocated split page to be readonly. Can you
> give me a hand on that?
> 
> thank you!
> Yao, Jiewen
> 
> 
> > 在 2017年11月29日，下午4:47，Jian J Wang <jian.j.wang@intel.com> 写
> 道：
> >
> > 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):
> > UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
> > MdeModulePkg/DxeIpl: Mark page table as read-only
> >
> > MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
> +++++++++++++++++++++++
> > MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
> > UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65 ++++++++-
> > 3 files changed, 241 insertions(+), 4 deletions(-)
> >
> > --
> > 2.14.1.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Yao, Jiewen]

Is this code in CPU driver?

thank you!
Yao, Jiewen


> 在 2017年11月29日，下午6:24，Wang, Jian J <jian.j.wang@intel.com> 写道：
> 
> Yes, I validated them manually with JTAG debug tool. 
> 
> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
>   // 1G page. Split to 2M.
>   L2PageTable = AllocatePages (1);
>   ASSERT (L2PageTable != NULL);
>    PhysicalAddress = L3PageTable[Index3] & PAGING_1G_ADDRESS_MASK_64;
>    for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
>     L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
>                          IA32_PG_PS | IA32_PG_P | IA32_PG_RW;
>     PhysicalAddress += SIZE_2MB;
>   }
>   L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable | AddressEncMask |
>                                          IA32_PG_P | IA32_PG_RW;
>   SetPageReadOnly (PageTableBase, (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
> }
> 
> The newly allocated page table is set in the SetPageReadOnly() itself recursively, like
> above code in which L2PageTable is allocated and then set it to be read-only after
> initializing the table content.
> 
>> -----Original Message-----
>> From: Yao, Jiewen
>> Sent: Wednesday, November 29, 2017 5:16 PM
>> To: Wang, Jian J <jian.j.wang@intel.com>
>> Cc: edk2-devel@lists.01.org
>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
>> 
>> Thanks.
>> 
>> May I know if this is validated in uefi shell, that all page table is readonly?
>> 
>> I did not find the code to set new allocated split page to be readonly. Can you
>> give me a hand on that?
>> 
>> thank you!
>> Yao, Jiewen
>> 
>> 
>>> 在 2017年11月29日，下午4:47，Jian J Wang <jian.j.wang@intel.com> 写
>> 道：
>>> 
>>> 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):
>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
>>> MdeModulePkg/DxeIpl: Mark page table as read-only
>>> 
>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
>> +++++++++++++++++++++++
>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
>>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65 ++++++++-
>>> 3 files changed, 241 insertions(+), 4 deletions(-)
>>> 
>>> --
>>> 2.14.1.windows.1
>>> 
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Wang, Jian J]

It's in the DxeIplPeim.

By the way, there's an issue in this patch. I forgot to protect page table for 32-bit mode.
So this patch works only for 64-bit mode. I'll add it in v2 patch. 

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, November 29, 2017 6:56 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> 
> Is this code in CPU driver?
> 
> thank you!
> Yao, Jiewen
> 
> 
> > 在 2017年11月29日，下午6:24，Wang, Jian J <jian.j.wang@intel.com> 写
> 道：
> >
> > Yes, I validated them manually with JTAG debug tool.
> >
> > if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
> >   // 1G page. Split to 2M.
> >   L2PageTable = AllocatePages (1);
> >   ASSERT (L2PageTable != NULL);
> >    PhysicalAddress = L3PageTable[Index3] & PAGING_1G_ADDRESS_MASK_64;
> >    for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
> >     L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
> >                          IA32_PG_PS | IA32_PG_P | IA32_PG_RW;
> >     PhysicalAddress += SIZE_2MB;
> >   }
> >   L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable | AddressEncMask |
> >                                          IA32_PG_P | IA32_PG_RW;
> >   SetPageReadOnly (PageTableBase,
> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
> > }
> >
> > The newly allocated page table is set in the SetPageReadOnly() itself
> recursively, like
> > above code in which L2PageTable is allocated and then set it to be read-only
> after
> > initializing the table content.
> >
> >> -----Original Message-----
> >> From: Yao, Jiewen
> >> Sent: Wednesday, November 29, 2017 5:16 PM
> >> To: Wang, Jian J <jian.j.wang@intel.com>
> >> Cc: edk2-devel@lists.01.org
> >> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> >>
> >> Thanks.
> >>
> >> May I know if this is validated in uefi shell, that all page table is readonly?
> >>
> >> I did not find the code to set new allocated split page to be readonly. Can you
> >> give me a hand on that?
> >>
> >> thank you!
> >> Yao, Jiewen
> >>
> >>
> >>> 在 2017年11月29日，下午4:47，Jian J Wang <jian.j.wang@intel.com>
> 写
> >> 道：
> >>>
> >>> 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):
> >>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
> >>> MdeModulePkg/DxeIpl: Mark page table as read-only
> >>>
> >>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
> >> +++++++++++++++++++++++
> >>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
> >>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65 ++++++++-
> >>> 3 files changed, 241 insertions(+), 4 deletions(-)
> >>>
> >>> --
> >>> 2.14.1.windows.1
> >>>
> >>> _______________________________________________
> >>> edk2-devel mailing list
> >>> edk2-devel@lists.01.org
> >>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Yao, Jiewen]

I do think we need update CPU driver to protect new allocated split page.

If you verified in shell env, I think it will exposed, please add a test to trigger page split in CPU driver.

I recommend to write some unit test to parse page table in shell.

thank you!
Yao, Jiewen


> 在 2017年11月29日，下午8:15，Wang, Jian J <jian.j.wang@intel.com> 写道：
> 
> It's in the DxeIplPeim.
> 
> By the way, there's an issue in this patch. I forgot to protect page table for 32-bit mode.
> So this patch works only for 64-bit mode. I'll add it in v2 patch. 
> 
>> -----Original Message-----
>> From: Yao, Jiewen
>> Sent: Wednesday, November 29, 2017 6:56 PM
>> To: Wang, Jian J <jian.j.wang@intel.com>
>> Cc: edk2-devel@lists.01.org
>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
>> 
>> Is this code in CPU driver?
>> 
>> thank you!
>> Yao, Jiewen
>> 
>> 
>>> 在 2017年11月29日，下午6:24，Wang, Jian J <jian.j.wang@intel.com> 写
>> 道：
>>> 
>>> Yes, I validated them manually with JTAG debug tool.
>>> 
>>> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
>>>  // 1G page. Split to 2M.
>>>  L2PageTable = AllocatePages (1);
>>>  ASSERT (L2PageTable != NULL);
>>>   PhysicalAddress = L3PageTable[Index3] & PAGING_1G_ADDRESS_MASK_64;
>>>   for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
>>>    L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
>>>                         IA32_PG_PS | IA32_PG_P | IA32_PG_RW;
>>>    PhysicalAddress += SIZE_2MB;
>>>  }
>>>  L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable | AddressEncMask |
>>>                                         IA32_PG_P | IA32_PG_RW;
>>>  SetPageReadOnly (PageTableBase,
>> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
>>> }
>>> 
>>> The newly allocated page table is set in the SetPageReadOnly() itself
>> recursively, like
>>> above code in which L2PageTable is allocated and then set it to be read-only
>> after
>>> initializing the table content.
>>> 
>>>> -----Original Message-----
>>>> From: Yao, Jiewen
>>>> Sent: Wednesday, November 29, 2017 5:16 PM
>>>> To: Wang, Jian J <jian.j.wang@intel.com>
>>>> Cc: edk2-devel@lists.01.org
>>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
>>>> 
>>>> Thanks.
>>>> 
>>>> May I know if this is validated in uefi shell, that all page table is readonly?
>>>> 
>>>> I did not find the code to set new allocated split page to be readonly. Can you
>>>> give me a hand on that?
>>>> 
>>>> thank you!
>>>> Yao, Jiewen
>>>> 
>>>> 
>>>>> 在 2017年11月29日，下午4:47，Jian J Wang <jian.j.wang@intel.com>
>> 写
>>>> 道：
>>>>> 
>>>>> 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):
>>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
>>>>> MdeModulePkg/DxeIpl: Mark page table as read-only
>>>>> 
>>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
>>>> +++++++++++++++++++++++
>>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
>>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65 ++++++++-
>>>>> 3 files changed, 241 insertions(+), 4 deletions(-)
>>>>> 
>>>>> --
>>>>> 2.14.1.windows.1
>>>>> 
>>>>> _______________________________________________
>>>>> edk2-devel mailing list
>>>>> edk2-devel@lists.01.org
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Wang, Jian J]

I did think of protecting new page table in cpu driver. But I think there's risks
to do it, which is that there might be a chance that, whenever you're trying to
mark one page used as page table to be read-only, you need to split its page
table in advance, and again and again, until all page tables are for 4KB pages.

Although this is a rare case, or the page table "split" will just happen for a few
times, it will slow down the boot process a little bit anyway. So I though  it's
safer not to protect new page tables created after DxeIpl.

Maybe it's just over worrying about it. Maybe we just need to add a PCD to 
turn on/off it just in case. Do you have any ideas in mind?

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, November 29, 2017 9:35 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> 
> I do think we need update CPU driver to protect new allocated split page.
> 
> If you verified in shell env, I think it will exposed, please add a test to trigger
> page split in CPU driver.
> 
> I recommend to write some unit test to parse page table in shell.
> 
> thank you!
> Yao, Jiewen
> 
> 
> > 在 2017年11月29日，下午8:15，Wang, Jian J <jian.j.wang@intel.com> 写
> 道：
> >
> > It's in the DxeIplPeim.
> >
> > By the way, there's an issue in this patch. I forgot to protect page table for 32-
> bit mode.
> > So this patch works only for 64-bit mode. I'll add it in v2 patch.
> >
> >> -----Original Message-----
> >> From: Yao, Jiewen
> >> Sent: Wednesday, November 29, 2017 6:56 PM
> >> To: Wang, Jian J <jian.j.wang@intel.com>
> >> Cc: edk2-devel@lists.01.org
> >> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> >>
> >> Is this code in CPU driver?
> >>
> >> thank you!
> >> Yao, Jiewen
> >>
> >>
> >>> 在 2017年11月29日，下午6:24，Wang, Jian J <jian.j.wang@intel.com>
> 写
> >> 道：
> >>>
> >>> Yes, I validated them manually with JTAG debug tool.
> >>>
> >>> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
> >>>  // 1G page. Split to 2M.
> >>>  L2PageTable = AllocatePages (1);
> >>>  ASSERT (L2PageTable != NULL);
> >>>   PhysicalAddress = L3PageTable[Index3] & PAGING_1G_ADDRESS_MASK_64;
> >>>   for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
> >>>    L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
> >>>                         IA32_PG_PS | IA32_PG_P | IA32_PG_RW;
> >>>    PhysicalAddress += SIZE_2MB;
> >>>  }
> >>>  L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable | AddressEncMask |
> >>>                                         IA32_PG_P | IA32_PG_RW;
> >>>  SetPageReadOnly (PageTableBase,
> >> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
> >>> }
> >>>
> >>> The newly allocated page table is set in the SetPageReadOnly() itself
> >> recursively, like
> >>> above code in which L2PageTable is allocated and then set it to be read-only
> >> after
> >>> initializing the table content.
> >>>
> >>>> -----Original Message-----
> >>>> From: Yao, Jiewen
> >>>> Sent: Wednesday, November 29, 2017 5:16 PM
> >>>> To: Wang, Jian J <jian.j.wang@intel.com>
> >>>> Cc: edk2-devel@lists.01.org
> >>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> >>>>
> >>>> Thanks.
> >>>>
> >>>> May I know if this is validated in uefi shell, that all page table is readonly?
> >>>>
> >>>> I did not find the code to set new allocated split page to be readonly. Can
> you
> >>>> give me a hand on that?
> >>>>
> >>>> thank you!
> >>>> Yao, Jiewen
> >>>>
> >>>>
> >>>>> 在 2017年11月29日，下午4:47，Jian J Wang <jian.j.wang@intel.com>
> >> 写
> >>>> 道：
> >>>>>
> >>>>> 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):
> >>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
> >>>>> MdeModulePkg/DxeIpl: Mark page table as read-only
> >>>>>
> >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
> >>>> +++++++++++++++++++++++
> >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
> >>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65 ++++++++-
> >>>>> 3 files changed, 241 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> --
> >>>>> 2.14.1.windows.1
> >>>>>
> >>>>> _______________________________________________
> >>>>> edk2-devel mailing list
> >>>>> edk2-devel@lists.01.org
> >>>>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Yao, Jiewen]

-- whenever you're trying to mark one page used as page table to be read-only, you need to split its page table in advance
[Jiewen] Sorry, I do not quite understand the problem statement.
To set a page to be ReadOnly, you just flip the R/W bit in the page entry.
The step I expect is:
1) You split page table.
2) Fill the data structure in the new entry.
3) Flip R/W bit (<== This is the new step).
4) FlushPageTable.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, November 30, 2017 8:44 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> 
> I did think of protecting new page table in cpu driver. But I think there's risks
> to do it, which is that there might be a chance that, whenever you're trying to
> mark one page used as page table to be read-only, you need to split its page
> table in advance, and again and again, until all page tables are for 4KB pages.
> 
> Although this is a rare case, or the page table "split" will just happen for a few
> times, it will slow down the boot process a little bit anyway. So I though  it's
> safer not to protect new page tables created after DxeIpl.
> 
> Maybe it's just over worrying about it. Maybe we just need to add a PCD to
> turn on/off it just in case. Do you have any ideas in mind?
> 
> > -----Original Message-----
> > From: Yao, Jiewen
> > Sent: Wednesday, November 29, 2017 9:35 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>
> > Cc: edk2-devel@lists.01.org
> > Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> >
> > I do think we need update CPU driver to protect new allocated split page.
> >
> > If you verified in shell env, I think it will exposed, please add a test to trigger
> > page split in CPU driver.
> >
> > I recommend to write some unit test to parse page table in shell.
> >
> > thank you!
> > Yao, Jiewen
> >
> >
> > > 在 2017年11月29日，下午8:15，Wang, Jian J <jian.j.wang@intel.com>
> 写
> > 道：
> > >
> > > It's in the DxeIplPeim.
> > >
> > > By the way, there's an issue in this patch. I forgot to protect page table for
> 32-
> > bit mode.
> > > So this patch works only for 64-bit mode. I'll add it in v2 patch.
> > >
> > >> -----Original Message-----
> > >> From: Yao, Jiewen
> > >> Sent: Wednesday, November 29, 2017 6:56 PM
> > >> To: Wang, Jian J <jian.j.wang@intel.com>
> > >> Cc: edk2-devel@lists.01.org
> > >> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > >>
> > >> Is this code in CPU driver?
> > >>
> > >> thank you!
> > >> Yao, Jiewen
> > >>
> > >>
> > >>> 在 2017年11月29日，下午6:24，Wang, Jian J <jian.j.wang@intel.com>
> > 写
> > >> 道：
> > >>>
> > >>> Yes, I validated them manually with JTAG debug tool.
> > >>>
> > >>> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
> > >>>  // 1G page. Split to 2M.
> > >>>  L2PageTable = AllocatePages (1);
> > >>>  ASSERT (L2PageTable != NULL);
> > >>>   PhysicalAddress = L3PageTable[Index3] &
> PAGING_1G_ADDRESS_MASK_64;
> > >>>   for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
> > >>>    L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
> > >>>                         IA32_PG_PS | IA32_PG_P | IA32_PG_RW;
> > >>>    PhysicalAddress += SIZE_2MB;
> > >>>  }
> > >>>  L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable |
> AddressEncMask |
> > >>>                                         IA32_PG_P |
> IA32_PG_RW;
> > >>>  SetPageReadOnly (PageTableBase,
> > >> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
> > >>> }
> > >>>
> > >>> The newly allocated page table is set in the SetPageReadOnly() itself
> > >> recursively, like
> > >>> above code in which L2PageTable is allocated and then set it to be
> read-only
> > >> after
> > >>> initializing the table content.
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Yao, Jiewen
> > >>>> Sent: Wednesday, November 29, 2017 5:16 PM
> > >>>> To: Wang, Jian J <jian.j.wang@intel.com>
> > >>>> Cc: edk2-devel@lists.01.org
> > >>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > >>>>
> > >>>> Thanks.
> > >>>>
> > >>>> May I know if this is validated in uefi shell, that all page table is readonly?
> > >>>>
> > >>>> I did not find the code to set new allocated split page to be readonly. Can
> > you
> > >>>> give me a hand on that?
> > >>>>
> > >>>> thank you!
> > >>>> Yao, Jiewen
> > >>>>
> > >>>>
> > >>>>> 在 2017年11月29日，下午4:47，Jian J Wang
> <jian.j.wang@intel.com>
> > >> 写
> > >>>> 道：
> > >>>>>
> > >>>>> 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):
> > >>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
> > >>>>> MdeModulePkg/DxeIpl: Mark page table as read-only
> > >>>>>
> > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
> > >>>> +++++++++++++++++++++++
> > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
> > >>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65
> ++++++++-
> > >>>>> 3 files changed, 241 insertions(+), 4 deletions(-)
> > >>>>>
> > >>>>> --
> > >>>>> 2.14.1.windows.1
> > >>>>>
> > >>>>> _______________________________________________
> > >>>>> edk2-devel mailing list
> > >>>>> edk2-devel@lists.01.org
> > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Wang, Jian J]

When you split page tables, you need to allocate new pages for new page table.
Since you have new page tables added, you need to mark them to be read-only
as well, right? When you do this, the page table for the memory newly allocated
might still needs to be split. This is the worst case but there's still chance of it,
right? We cannot guarantee, theoretically, the page table for the newly allocated
page tables is in the same as just split ones. So, in worst case, once we want to
mark new page table to be read-only, we need to split the page table managing
those memory, and if we need to do split, we need to allocate more new pages.
This might fall into a recursive situation until all the smallest page table are created.
In practice it's almost impossible to let it happen but the chances are we will fall into 
such recursive situation more than one level.

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Thursday, November 30, 2017 8:52 AM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> 
> -- whenever you're trying to mark one page used as page table to be read-only,
> you need to split its page table in advance
> [Jiewen] Sorry, I do not quite understand the problem statement.
> To set a page to be ReadOnly, you just flip the R/W bit in the page entry.
> The step I expect is:
> 1) You split page table.
> 2) Fill the data structure in the new entry.
> 3) Flip R/W bit (<== This is the new step).
> 4) FlushPageTable.
> 
> Thank you
> Yao Jiewen
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Thursday, November 30, 2017 8:44 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>
> > Cc: edk2-devel@lists.01.org
> > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> >
> > I did think of protecting new page table in cpu driver. But I think there's risks
> > to do it, which is that there might be a chance that, whenever you're trying to
> > mark one page used as page table to be read-only, you need to split its page
> > table in advance, and again and again, until all page tables are for 4KB pages.
> >
> > Although this is a rare case, or the page table "split" will just happen for a few
> > times, it will slow down the boot process a little bit anyway. So I though  it's
> > safer not to protect new page tables created after DxeIpl.
> >
> > Maybe it's just over worrying about it. Maybe we just need to add a PCD to
> > turn on/off it just in case. Do you have any ideas in mind?
> >
> > > -----Original Message-----
> > > From: Yao, Jiewen
> > > Sent: Wednesday, November 29, 2017 9:35 PM
> > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > Cc: edk2-devel@lists.01.org
> > > Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > >
> > > I do think we need update CPU driver to protect new allocated split page.
> > >
> > > If you verified in shell env, I think it will exposed, please add a test to trigger
> > > page split in CPU driver.
> > >
> > > I recommend to write some unit test to parse page table in shell.
> > >
> > > thank you!
> > > Yao, Jiewen
> > >
> > >
> > > > 在 2017年11月29日，下午8:15，Wang, Jian J <jian.j.wang@intel.com>
> > 写
> > > 道：
> > > >
> > > > It's in the DxeIplPeim.
> > > >
> > > > By the way, there's an issue in this patch. I forgot to protect page table for
> > 32-
> > > bit mode.
> > > > So this patch works only for 64-bit mode. I'll add it in v2 patch.
> > > >
> > > >> -----Original Message-----
> > > >> From: Yao, Jiewen
> > > >> Sent: Wednesday, November 29, 2017 6:56 PM
> > > >> To: Wang, Jian J <jian.j.wang@intel.com>
> > > >> Cc: edk2-devel@lists.01.org
> > > >> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > >>
> > > >> Is this code in CPU driver?
> > > >>
> > > >> thank you!
> > > >> Yao, Jiewen
> > > >>
> > > >>
> > > >>> 在 2017年11月29日，下午6:24，Wang, Jian J
> <jian.j.wang@intel.com>
> > > 写
> > > >> 道：
> > > >>>
> > > >>> Yes, I validated them manually with JTAG debug tool.
> > > >>>
> > > >>> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
> > > >>>  // 1G page. Split to 2M.
> > > >>>  L2PageTable = AllocatePages (1);
> > > >>>  ASSERT (L2PageTable != NULL);
> > > >>>   PhysicalAddress = L3PageTable[Index3] &
> > PAGING_1G_ADDRESS_MASK_64;
> > > >>>   for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
> > > >>>    L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
> > > >>>                         IA32_PG_PS | IA32_PG_P | IA32_PG_RW;
> > > >>>    PhysicalAddress += SIZE_2MB;
> > > >>>  }
> > > >>>  L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable |
> > AddressEncMask |
> > > >>>                                         IA32_PG_P |
> > IA32_PG_RW;
> > > >>>  SetPageReadOnly (PageTableBase,
> > > >> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
> > > >>> }
> > > >>>
> > > >>> The newly allocated page table is set in the SetPageReadOnly() itself
> > > >> recursively, like
> > > >>> above code in which L2PageTable is allocated and then set it to be
> > read-only
> > > >> after
> > > >>> initializing the table content.
> > > >>>
> > > >>>> -----Original Message-----
> > > >>>> From: Yao, Jiewen
> > > >>>> Sent: Wednesday, November 29, 2017 5:16 PM
> > > >>>> To: Wang, Jian J <jian.j.wang@intel.com>
> > > >>>> Cc: edk2-devel@lists.01.org
> > > >>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > >>>>
> > > >>>> Thanks.
> > > >>>>
> > > >>>> May I know if this is validated in uefi shell, that all page table is
> readonly?
> > > >>>>
> > > >>>> I did not find the code to set new allocated split page to be readonly.
> Can
> > > you
> > > >>>> give me a hand on that?
> > > >>>>
> > > >>>> thank you!
> > > >>>> Yao, Jiewen
> > > >>>>
> > > >>>>
> > > >>>>> 在 2017年11月29日，下午4:47，Jian J Wang
> > <jian.j.wang@intel.com>
> > > >> 写
> > > >>>> 道：
> > > >>>>>
> > > >>>>> 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):
> > > >>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
> > > >>>>> MdeModulePkg/DxeIpl: Mark page table as read-only
> > > >>>>>
> > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
> > > >>>> +++++++++++++++++++++++
> > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
> > > >>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65
> > ++++++++-
> > > >>>>> 3 files changed, 241 insertions(+), 4 deletions(-)
> > > >>>>>
> > > >>>>> --
> > > >>>>> 2.14.1.windows.1
> > > >>>>>
> > > >>>>> _______________________________________________
> > > >>>>> edk2-devel mailing list
> > > >>>>> edk2-devel@lists.01.org
> > > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Yao, Jiewen]

Can you just allocate 1 more page for split?
If new one need split, you can just use the additional page.
If not, you can free it later.

Thank you
Yao Jiewen


> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, November 30, 2017 9:17 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> 
> When you split page tables, you need to allocate new pages for new page table.
> Since you have new page tables added, you need to mark them to be read-only
> as well, right? When you do this, the page table for the memory newly allocated
> might still needs to be split. This is the worst case but there's still chance of it,
> right? We cannot guarantee, theoretically, the page table for the newly allocated
> page tables is in the same as just split ones. So, in worst case, once we want to
> mark new page table to be read-only, we need to split the page table managing
> those memory, and if we need to do split, we need to allocate more new pages.
> This might fall into a recursive situation until all the smallest page table are
> created.
> In practice it's almost impossible to let it happen but the chances are we will fall
> into
> such recursive situation more than one level.
> 
> > -----Original Message-----
> > From: Yao, Jiewen
> > Sent: Thursday, November 30, 2017 8:52 AM
> > To: Wang, Jian J <jian.j.wang@intel.com>
> > Cc: edk2-devel@lists.01.org
> > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> >
> > -- whenever you're trying to mark one page used as page table to be read-only,
> > you need to split its page table in advance
> > [Jiewen] Sorry, I do not quite understand the problem statement.
> > To set a page to be ReadOnly, you just flip the R/W bit in the page entry.
> > The step I expect is:
> > 1) You split page table.
> > 2) Fill the data structure in the new entry.
> > 3) Flip R/W bit (<== This is the new step).
> > 4) FlushPageTable.
> >
> > Thank you
> > Yao Jiewen
> >
> > > -----Original Message-----
> > > From: Wang, Jian J
> > > Sent: Thursday, November 30, 2017 8:44 AM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > Cc: edk2-devel@lists.01.org
> > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > >
> > > I did think of protecting new page table in cpu driver. But I think there's risks
> > > to do it, which is that there might be a chance that, whenever you're trying to
> > > mark one page used as page table to be read-only, you need to split its page
> > > table in advance, and again and again, until all page tables are for 4KB pages.
> > >
> > > Although this is a rare case, or the page table "split" will just happen for a few
> > > times, it will slow down the boot process a little bit anyway. So I though  it's
> > > safer not to protect new page tables created after DxeIpl.
> > >
> > > Maybe it's just over worrying about it. Maybe we just need to add a PCD to
> > > turn on/off it just in case. Do you have any ideas in mind?
> > >
> > > > -----Original Message-----
> > > > From: Yao, Jiewen
> > > > Sent: Wednesday, November 29, 2017 9:35 PM
> > > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > > Cc: edk2-devel@lists.01.org
> > > > Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > >
> > > > I do think we need update CPU driver to protect new allocated split page.
> > > >
> > > > If you verified in shell env, I think it will exposed, please add a test to trigger
> > > > page split in CPU driver.
> > > >
> > > > I recommend to write some unit test to parse page table in shell.
> > > >
> > > > thank you!
> > > > Yao, Jiewen
> > > >
> > > >
> > > > > 在 2017年11月29日，下午8:15，Wang, Jian J <jian.j.wang@intel.com>
> > > 写
> > > > 道：
> > > > >
> > > > > It's in the DxeIplPeim.
> > > > >
> > > > > By the way, there's an issue in this patch. I forgot to protect page table for
> > > 32-
> > > > bit mode.
> > > > > So this patch works only for 64-bit mode. I'll add it in v2 patch.
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: Yao, Jiewen
> > > > >> Sent: Wednesday, November 29, 2017 6:56 PM
> > > > >> To: Wang, Jian J <jian.j.wang@intel.com>
> > > > >> Cc: edk2-devel@lists.01.org
> > > > >> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > >>
> > > > >> Is this code in CPU driver?
> > > > >>
> > > > >> thank you!
> > > > >> Yao, Jiewen
> > > > >>
> > > > >>
> > > > >>> 在 2017年11月29日，下午6:24，Wang, Jian J
> > <jian.j.wang@intel.com>
> > > > 写
> > > > >> 道：
> > > > >>>
> > > > >>> Yes, I validated them manually with JTAG debug tool.
> > > > >>>
> > > > >>> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
> > > > >>>  // 1G page. Split to 2M.
> > > > >>>  L2PageTable = AllocatePages (1);
> > > > >>>  ASSERT (L2PageTable != NULL);
> > > > >>>   PhysicalAddress = L3PageTable[Index3] &
> > > PAGING_1G_ADDRESS_MASK_64;
> > > > >>>   for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
> > > > >>>    L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
> > > > >>>                         IA32_PG_PS | IA32_PG_P |
> IA32_PG_RW;
> > > > >>>    PhysicalAddress += SIZE_2MB;
> > > > >>>  }
> > > > >>>  L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable |
> > > AddressEncMask |
> > > > >>>                                         IA32_PG_P |
> > > IA32_PG_RW;
> > > > >>>  SetPageReadOnly (PageTableBase,
> > > > >> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
> > > > >>> }
> > > > >>>
> > > > >>> The newly allocated page table is set in the SetPageReadOnly() itself
> > > > >> recursively, like
> > > > >>> above code in which L2PageTable is allocated and then set it to be
> > > read-only
> > > > >> after
> > > > >>> initializing the table content.
> > > > >>>
> > > > >>>> -----Original Message-----
> > > > >>>> From: Yao, Jiewen
> > > > >>>> Sent: Wednesday, November 29, 2017 5:16 PM
> > > > >>>> To: Wang, Jian J <jian.j.wang@intel.com>
> > > > >>>> Cc: edk2-devel@lists.01.org
> > > > >>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > >>>>
> > > > >>>> Thanks.
> > > > >>>>
> > > > >>>> May I know if this is validated in uefi shell, that all page table is
> > readonly?
> > > > >>>>
> > > > >>>> I did not find the code to set new allocated split page to be readonly.
> > Can
> > > > you
> > > > >>>> give me a hand on that?
> > > > >>>>
> > > > >>>> thank you!
> > > > >>>> Yao, Jiewen
> > > > >>>>
> > > > >>>>
> > > > >>>>> 在 2017年11月29日，下午4:47，Jian J Wang
> > > <jian.j.wang@intel.com>
> > > > >> 写
> > > > >>>> 道：
> > > > >>>>>
> > > > >>>>> 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):
> > > > >>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
> > > > >>>>> MdeModulePkg/DxeIpl: Mark page table as read-only
> > > > >>>>>
> > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
> > > > >>>> +++++++++++++++++++++++
> > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
> > > > >>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65
> > > ++++++++-
> > > > >>>>> 3 files changed, 241 insertions(+), 4 deletions(-)
> > > > >>>>>
> > > > >>>>> --
> > > > >>>>> 2.14.1.windows.1
> > > > >>>>>
> > > > >>>>> _______________________________________________
> > > > >>>>> edk2-devel mailing list
> > > > >>>>> edk2-devel@lists.01.org
> > > > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Yao, Jiewen]

Clarify my word below:

Can you just predict and pre-allocate additional pages for future split in worst case?
If new one need split, you can just use the additional pages.
If not, you can free them later.

For example, you can allocate 2M aligned memory for new pages, for make sure they are in same page directory.

Thank you
Yao Jiewen

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao,
> Jiewen
> Sent: Thursday, November 30, 2017 9:36 AM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> 
> Can you just allocate 1 more page for split?
> If new one need split, you can just use the additional page.
> If not, you can free it later.
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Thursday, November 30, 2017 9:17 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>
> > Cc: edk2-devel@lists.01.org
> > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> >
> > When you split page tables, you need to allocate new pages for new page
> table.
> > Since you have new page tables added, you need to mark them to be read-only
> > as well, right? When you do this, the page table for the memory newly
> allocated
> > might still needs to be split. This is the worst case but there's still chance of it,
> > right? We cannot guarantee, theoretically, the page table for the newly
> allocated
> > page tables is in the same as just split ones. So, in worst case, once we want to
> > mark new page table to be read-only, we need to split the page table managing
> > those memory, and if we need to do split, we need to allocate more new pages.
> > This might fall into a recursive situation until all the smallest page table are
> > created.
> > In practice it's almost impossible to let it happen but the chances are we will fall
> > into
> > such recursive situation more than one level.
> >
> > > -----Original Message-----
> > > From: Yao, Jiewen
> > > Sent: Thursday, November 30, 2017 8:52 AM
> > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > Cc: edk2-devel@lists.01.org
> > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > >
> > > -- whenever you're trying to mark one page used as page table to be
> read-only,
> > > you need to split its page table in advance
> > > [Jiewen] Sorry, I do not quite understand the problem statement.
> > > To set a page to be ReadOnly, you just flip the R/W bit in the page entry.
> > > The step I expect is:
> > > 1) You split page table.
> > > 2) Fill the data structure in the new entry.
> > > 3) Flip R/W bit (<== This is the new step).
> > > 4) FlushPageTable.
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > > > -----Original Message-----
> > > > From: Wang, Jian J
> > > > Sent: Thursday, November 30, 2017 8:44 AM
> > > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > > Cc: edk2-devel@lists.01.org
> > > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > > >
> > > > I did think of protecting new page table in cpu driver. But I think there's
> risks
> > > > to do it, which is that there might be a chance that, whenever you're trying
> to
> > > > mark one page used as page table to be read-only, you need to split its page
> > > > table in advance, and again and again, until all page tables are for 4KB
> pages.
> > > >
> > > > Although this is a rare case, or the page table "split" will just happen for a
> few
> > > > times, it will slow down the boot process a little bit anyway. So I though
> it's
> > > > safer not to protect new page tables created after DxeIpl.
> > > >
> > > > Maybe it's just over worrying about it. Maybe we just need to add a PCD to
> > > > turn on/off it just in case. Do you have any ideas in mind?
> > > >
> > > > > -----Original Message-----
> > > > > From: Yao, Jiewen
> > > > > Sent: Wednesday, November 29, 2017 9:35 PM
> > > > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > Cc: edk2-devel@lists.01.org
> > > > > Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > >
> > > > > I do think we need update CPU driver to protect new allocated split page.
> > > > >
> > > > > If you verified in shell env, I think it will exposed, please add a test to
> trigger
> > > > > page split in CPU driver.
> > > > >
> > > > > I recommend to write some unit test to parse page table in shell.
> > > > >
> > > > > thank you!
> > > > > Yao, Jiewen
> > > > >
> > > > >
> > > > > > 在 2017年11月29日，下午8:15，Wang, Jian J
> <jian.j.wang@intel.com>
> > > > 写
> > > > > 道：
> > > > > >
> > > > > > It's in the DxeIplPeim.
> > > > > >
> > > > > > By the way, there's an issue in this patch. I forgot to protect page table
> for
> > > > 32-
> > > > > bit mode.
> > > > > > So this patch works only for 64-bit mode. I'll add it in v2 patch.
> > > > > >
> > > > > >> -----Original Message-----
> > > > > >> From: Yao, Jiewen
> > > > > >> Sent: Wednesday, November 29, 2017 6:56 PM
> > > > > >> To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > >> Cc: edk2-devel@lists.01.org
> > > > > >> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > > >>
> > > > > >> Is this code in CPU driver?
> > > > > >>
> > > > > >> thank you!
> > > > > >> Yao, Jiewen
> > > > > >>
> > > > > >>
> > > > > >>> 在 2017年11月29日，下午6:24，Wang, Jian J
> > > <jian.j.wang@intel.com>
> > > > > 写
> > > > > >> 道：
> > > > > >>>
> > > > > >>> Yes, I validated them manually with JTAG debug tool.
> > > > > >>>
> > > > > >>> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
> > > > > >>>  // 1G page. Split to 2M.
> > > > > >>>  L2PageTable = AllocatePages (1);
> > > > > >>>  ASSERT (L2PageTable != NULL);
> > > > > >>>   PhysicalAddress = L3PageTable[Index3] &
> > > > PAGING_1G_ADDRESS_MASK_64;
> > > > > >>>   for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
> > > > > >>>    L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
> > > > > >>>                         IA32_PG_PS | IA32_PG_P |
> > IA32_PG_RW;
> > > > > >>>    PhysicalAddress += SIZE_2MB;
> > > > > >>>  }
> > > > > >>>  L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable |
> > > > AddressEncMask |
> > > > > >>>                                         IA32_PG_P |
> > > > IA32_PG_RW;
> > > > > >>>  SetPageReadOnly (PageTableBase,
> > > > > >> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
> > > > > >>> }
> > > > > >>>
> > > > > >>> The newly allocated page table is set in the SetPageReadOnly() itself
> > > > > >> recursively, like
> > > > > >>> above code in which L2PageTable is allocated and then set it to be
> > > > read-only
> > > > > >> after
> > > > > >>> initializing the table content.
> > > > > >>>
> > > > > >>>> -----Original Message-----
> > > > > >>>> From: Yao, Jiewen
> > > > > >>>> Sent: Wednesday, November 29, 2017 5:16 PM
> > > > > >>>> To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > >>>> Cc: edk2-devel@lists.01.org
> > > > > >>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > > >>>>
> > > > > >>>> Thanks.
> > > > > >>>>
> > > > > >>>> May I know if this is validated in uefi shell, that all page table is
> > > readonly?
> > > > > >>>>
> > > > > >>>> I did not find the code to set new allocated split page to be
> readonly.
> > > Can
> > > > > you
> > > > > >>>> give me a hand on that?
> > > > > >>>>
> > > > > >>>> thank you!
> > > > > >>>> Yao, Jiewen
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>> 在 2017年11月29日，下午4:47，Jian J Wang
> > > > <jian.j.wang@intel.com>
> > > > > >> 写
> > > > > >>>> 道：
> > > > > >>>>>
> > > > > >>>>> 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):
> > > > > >>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
> > > > > >>>>> MdeModulePkg/DxeIpl: Mark page table as read-only
> > > > > >>>>>
> > > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
> > > > > >>>> +++++++++++++++++++++++
> > > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14
> ++
> > > > > >>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65
> > > > ++++++++-
> > > > > >>>>> 3 files changed, 241 insertions(+), 4 deletions(-)
> > > > > >>>>>
> > > > > >>>>> --
> > > > > >>>>> 2.14.1.windows.1
> > > > > >>>>>
> > > > > >>>>> _______________________________________________
> > > > > >>>>> edk2-devel mailing list
> > > > > >>>>> edk2-devel@lists.01.org
> > > > > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Wang, Jian J]

Good idea. I think it will highly reduce the possibility but not solve it still.
The dilemma here is: do we need to take care of it? This issue exists in theory
but has not yet been encountered in practice.

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Thursday, November 30, 2017 9:36 AM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> 
> Can you just allocate 1 more page for split?
> If new one need split, you can just use the additional page.
> If not, you can free it later.
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Thursday, November 30, 2017 9:17 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>
> > Cc: edk2-devel@lists.01.org
> > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> >
> > When you split page tables, you need to allocate new pages for new page
> table.
> > Since you have new page tables added, you need to mark them to be read-only
> > as well, right? When you do this, the page table for the memory newly
> allocated
> > might still needs to be split. This is the worst case but there's still chance of it,
> > right? We cannot guarantee, theoretically, the page table for the newly
> allocated
> > page tables is in the same as just split ones. So, in worst case, once we want to
> > mark new page table to be read-only, we need to split the page table
> managing
> > those memory, and if we need to do split, we need to allocate more new
> pages.
> > This might fall into a recursive situation until all the smallest page table are
> > created.
> > In practice it's almost impossible to let it happen but the chances are we will
> fall
> > into
> > such recursive situation more than one level.
> >
> > > -----Original Message-----
> > > From: Yao, Jiewen
> > > Sent: Thursday, November 30, 2017 8:52 AM
> > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > Cc: edk2-devel@lists.01.org
> > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > >
> > > -- whenever you're trying to mark one page used as page table to be read-
> only,
> > > you need to split its page table in advance
> > > [Jiewen] Sorry, I do not quite understand the problem statement.
> > > To set a page to be ReadOnly, you just flip the R/W bit in the page entry.
> > > The step I expect is:
> > > 1) You split page table.
> > > 2) Fill the data structure in the new entry.
> > > 3) Flip R/W bit (<== This is the new step).
> > > 4) FlushPageTable.
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > > > -----Original Message-----
> > > > From: Wang, Jian J
> > > > Sent: Thursday, November 30, 2017 8:44 AM
> > > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > > Cc: edk2-devel@lists.01.org
> > > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > > >
> > > > I did think of protecting new page table in cpu driver. But I think there's
> risks
> > > > to do it, which is that there might be a chance that, whenever you're trying
> to
> > > > mark one page used as page table to be read-only, you need to split its
> page
> > > > table in advance, and again and again, until all page tables are for 4KB
> pages.
> > > >
> > > > Although this is a rare case, or the page table "split" will just happen for a
> few
> > > > times, it will slow down the boot process a little bit anyway. So I though
> it's
> > > > safer not to protect new page tables created after DxeIpl.
> > > >
> > > > Maybe it's just over worrying about it. Maybe we just need to add a PCD to
> > > > turn on/off it just in case. Do you have any ideas in mind?
> > > >
> > > > > -----Original Message-----
> > > > > From: Yao, Jiewen
> > > > > Sent: Wednesday, November 29, 2017 9:35 PM
> > > > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > Cc: edk2-devel@lists.01.org
> > > > > Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > >
> > > > > I do think we need update CPU driver to protect new allocated split page.
> > > > >
> > > > > If you verified in shell env, I think it will exposed, please add a test to
> trigger
> > > > > page split in CPU driver.
> > > > >
> > > > > I recommend to write some unit test to parse page table in shell.
> > > > >
> > > > > thank you!
> > > > > Yao, Jiewen
> > > > >
> > > > >
> > > > > > 在 2017年11月29日，下午8:15，Wang, Jian J
> <jian.j.wang@intel.com>
> > > > 写
> > > > > 道：
> > > > > >
> > > > > > It's in the DxeIplPeim.
> > > > > >
> > > > > > By the way, there's an issue in this patch. I forgot to protect page table
> for
> > > > 32-
> > > > > bit mode.
> > > > > > So this patch works only for 64-bit mode. I'll add it in v2 patch.
> > > > > >
> > > > > >> -----Original Message-----
> > > > > >> From: Yao, Jiewen
> > > > > >> Sent: Wednesday, November 29, 2017 6:56 PM
> > > > > >> To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > >> Cc: edk2-devel@lists.01.org
> > > > > >> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > > >>
> > > > > >> Is this code in CPU driver?
> > > > > >>
> > > > > >> thank you!
> > > > > >> Yao, Jiewen
> > > > > >>
> > > > > >>
> > > > > >>> 在 2017年11月29日，下午6:24，Wang, Jian J
> > > <jian.j.wang@intel.com>
> > > > > 写
> > > > > >> 道：
> > > > > >>>
> > > > > >>> Yes, I validated them manually with JTAG debug tool.
> > > > > >>>
> > > > > >>> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
> > > > > >>>  // 1G page. Split to 2M.
> > > > > >>>  L2PageTable = AllocatePages (1);
> > > > > >>>  ASSERT (L2PageTable != NULL);
> > > > > >>>   PhysicalAddress = L3PageTable[Index3] &
> > > > PAGING_1G_ADDRESS_MASK_64;
> > > > > >>>   for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
> > > > > >>>    L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
> > > > > >>>                         IA32_PG_PS | IA32_PG_P |
> > IA32_PG_RW;
> > > > > >>>    PhysicalAddress += SIZE_2MB;
> > > > > >>>  }
> > > > > >>>  L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable |
> > > > AddressEncMask |
> > > > > >>>                                         IA32_PG_P |
> > > > IA32_PG_RW;
> > > > > >>>  SetPageReadOnly (PageTableBase,
> > > > > >> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
> > > > > >>> }
> > > > > >>>
> > > > > >>> The newly allocated page table is set in the SetPageReadOnly() itself
> > > > > >> recursively, like
> > > > > >>> above code in which L2PageTable is allocated and then set it to be
> > > > read-only
> > > > > >> after
> > > > > >>> initializing the table content.
> > > > > >>>
> > > > > >>>> -----Original Message-----
> > > > > >>>> From: Yao, Jiewen
> > > > > >>>> Sent: Wednesday, November 29, 2017 5:16 PM
> > > > > >>>> To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > >>>> Cc: edk2-devel@lists.01.org
> > > > > >>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > > >>>>
> > > > > >>>> Thanks.
> > > > > >>>>
> > > > > >>>> May I know if this is validated in uefi shell, that all page table is
> > > readonly?
> > > > > >>>>
> > > > > >>>> I did not find the code to set new allocated split page to be
> readonly.
> > > Can
> > > > > you
> > > > > >>>> give me a hand on that?
> > > > > >>>>
> > > > > >>>> thank you!
> > > > > >>>> Yao, Jiewen
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>> 在 2017年11月29日，下午4:47，Jian J Wang
> > > > <jian.j.wang@intel.com>
> > > > > >> 写
> > > > > >>>> 道：
> > > > > >>>>>
> > > > > >>>>> 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):
> > > > > >>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
> > > > > >>>>> MdeModulePkg/DxeIpl: Mark page table as read-only
> > > > > >>>>>
> > > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
> > > > > >>>> +++++++++++++++++++++++
> > > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
> > > > > >>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65
> > > > ++++++++-
> > > > > >>>>> 3 files changed, 241 insertions(+), 4 deletions(-)
> > > > > >>>>>
> > > > > >>>>> --
> > > > > >>>>> 2.14.1.windows.1
> > > > > >>>>>
> > > > > >>>>> _______________________________________________
> > > > > >>>>> edk2-devel mailing list
> > > > > >>>>> edk2-devel@lists.01.org
> > > > > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Yao, Jiewen]

Clarify my word below:

Can you just predict and pre-allocate additional pages for future split in worst case?
If new one need split, you can just use the additional pages.
If not, you can free them later.

For example, you can allocate 2M aligned memory for new pages, for make sure they are in same page directory.

I think it can resolve the problem.
The key is to pre-calculate how many pages are involved in the worst case.

Thank you
Yao Jiewen


> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, November 30, 2017 9:47 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> 
> Good idea. I think it will highly reduce the possibility but not solve it still.
> The dilemma here is: do we need to take care of it? This issue exists in theory
> but has not yet been encountered in practice.
> 
> > -----Original Message-----
> > From: Yao, Jiewen
> > Sent: Thursday, November 30, 2017 9:36 AM
> > To: Wang, Jian J <jian.j.wang@intel.com>
> > Cc: edk2-devel@lists.01.org
> > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> >
> > Can you just allocate 1 more page for split?
> > If new one need split, you can just use the additional page.
> > If not, you can free it later.
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Wang, Jian J
> > > Sent: Thursday, November 30, 2017 9:17 AM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > Cc: edk2-devel@lists.01.org
> > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > >
> > > When you split page tables, you need to allocate new pages for new page
> > table.
> > > Since you have new page tables added, you need to mark them to be
> read-only
> > > as well, right? When you do this, the page table for the memory newly
> > allocated
> > > might still needs to be split. This is the worst case but there's still chance of it,
> > > right? We cannot guarantee, theoretically, the page table for the newly
> > allocated
> > > page tables is in the same as just split ones. So, in worst case, once we want
> to
> > > mark new page table to be read-only, we need to split the page table
> > managing
> > > those memory, and if we need to do split, we need to allocate more new
> > pages.
> > > This might fall into a recursive situation until all the smallest page table are
> > > created.
> > > In practice it's almost impossible to let it happen but the chances are we will
> > fall
> > > into
> > > such recursive situation more than one level.
> > >
> > > > -----Original Message-----
> > > > From: Yao, Jiewen
> > > > Sent: Thursday, November 30, 2017 8:52 AM
> > > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > > Cc: edk2-devel@lists.01.org
> > > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > > >
> > > > -- whenever you're trying to mark one page used as page table to be read-
> > only,
> > > > you need to split its page table in advance
> > > > [Jiewen] Sorry, I do not quite understand the problem statement.
> > > > To set a page to be ReadOnly, you just flip the R/W bit in the page entry.
> > > > The step I expect is:
> > > > 1) You split page table.
> > > > 2) Fill the data structure in the new entry.
> > > > 3) Flip R/W bit (<== This is the new step).
> > > > 4) FlushPageTable.
> > > >
> > > > Thank you
> > > > Yao Jiewen
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Jian J
> > > > > Sent: Thursday, November 30, 2017 8:44 AM
> > > > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > > > Cc: edk2-devel@lists.01.org
> > > > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > > > >
> > > > > I did think of protecting new page table in cpu driver. But I think there's
> > risks
> > > > > to do it, which is that there might be a chance that, whenever you're
> trying
> > to
> > > > > mark one page used as page table to be read-only, you need to split its
> > page
> > > > > table in advance, and again and again, until all page tables are for 4KB
> > pages.
> > > > >
> > > > > Although this is a rare case, or the page table "split" will just happen for a
> > few
> > > > > times, it will slow down the boot process a little bit anyway. So I though
> > it's
> > > > > safer not to protect new page tables created after DxeIpl.
> > > > >
> > > > > Maybe it's just over worrying about it. Maybe we just need to add a PCD
> to
> > > > > turn on/off it just in case. Do you have any ideas in mind?
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Yao, Jiewen
> > > > > > Sent: Wednesday, November 29, 2017 9:35 PM
> > > > > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > > Cc: edk2-devel@lists.01.org
> > > > > > Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > > >
> > > > > > I do think we need update CPU driver to protect new allocated split
> page.
> > > > > >
> > > > > > If you verified in shell env, I think it will exposed, please add a test to
> > trigger
> > > > > > page split in CPU driver.
> > > > > >
> > > > > > I recommend to write some unit test to parse page table in shell.
> > > > > >
> > > > > > thank you!
> > > > > > Yao, Jiewen
> > > > > >
> > > > > >
> > > > > > > 在 2017年11月29日，下午8:15，Wang, Jian J
> > <jian.j.wang@intel.com>
> > > > > 写
> > > > > > 道：
> > > > > > >
> > > > > > > It's in the DxeIplPeim.
> > > > > > >
> > > > > > > By the way, there's an issue in this patch. I forgot to protect page table
> > for
> > > > > 32-
> > > > > > bit mode.
> > > > > > > So this patch works only for 64-bit mode. I'll add it in v2 patch.
> > > > > > >
> > > > > > >> -----Original Message-----
> > > > > > >> From: Yao, Jiewen
> > > > > > >> Sent: Wednesday, November 29, 2017 6:56 PM
> > > > > > >> To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > > >> Cc: edk2-devel@lists.01.org
> > > > > > >> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > > > >>
> > > > > > >> Is this code in CPU driver?
> > > > > > >>
> > > > > > >> thank you!
> > > > > > >> Yao, Jiewen
> > > > > > >>
> > > > > > >>
> > > > > > >>> 在 2017年11月29日，下午6:24，Wang, Jian J
> > > > <jian.j.wang@intel.com>
> > > > > > 写
> > > > > > >> 道：
> > > > > > >>>
> > > > > > >>> Yes, I validated them manually with JTAG debug tool.
> > > > > > >>>
> > > > > > >>> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
> > > > > > >>>  // 1G page. Split to 2M.
> > > > > > >>>  L2PageTable = AllocatePages (1);
> > > > > > >>>  ASSERT (L2PageTable != NULL);
> > > > > > >>>   PhysicalAddress = L3PageTable[Index3] &
> > > > > PAGING_1G_ADDRESS_MASK_64;
> > > > > > >>>   for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index)
> {
> > > > > > >>>    L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
> > > > > > >>>                         IA32_PG_PS | IA32_PG_P |
> > > IA32_PG_RW;
> > > > > > >>>    PhysicalAddress += SIZE_2MB;
> > > > > > >>>  }
> > > > > > >>>  L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable |
> > > > > AddressEncMask |
> > > > > > >>>                                         IA32_PG_P |
> > > > > IA32_PG_RW;
> > > > > > >>>  SetPageReadOnly (PageTableBase,
> > > > > > >> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
> > > > > > >>> }
> > > > > > >>>
> > > > > > >>> The newly allocated page table is set in the SetPageReadOnly() itself
> > > > > > >> recursively, like
> > > > > > >>> above code in which L2PageTable is allocated and then set it to be
> > > > > read-only
> > > > > > >> after
> > > > > > >>> initializing the table content.
> > > > > > >>>
> > > > > > >>>> -----Original Message-----
> > > > > > >>>> From: Yao, Jiewen
> > > > > > >>>> Sent: Wednesday, November 29, 2017 5:16 PM
> > > > > > >>>> To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > > >>>> Cc: edk2-devel@lists.01.org
> > > > > > >>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write
> protection
> > > > > > >>>>
> > > > > > >>>> Thanks.
> > > > > > >>>>
> > > > > > >>>> May I know if this is validated in uefi shell, that all page table is
> > > > readonly?
> > > > > > >>>>
> > > > > > >>>> I did not find the code to set new allocated split page to be
> > readonly.
> > > > Can
> > > > > > you
> > > > > > >>>> give me a hand on that?
> > > > > > >>>>
> > > > > > >>>> thank you!
> > > > > > >>>> Yao, Jiewen
> > > > > > >>>>
> > > > > > >>>>
> > > > > > >>>>> 在 2017年11月29日，下午4:47，Jian J Wang
> > > > > <jian.j.wang@intel.com>
> > > > > > >> 写
> > > > > > >>>> 道：
> > > > > > >>>>>
> > > > > > >>>>> 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):
> > > > > > >>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
> > > > > > >>>>> MdeModulePkg/DxeIpl: Mark page table as read-only
> > > > > > >>>>>
> > > > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
> > > > > > >>>> +++++++++++++++++++++++
> > > > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14
> ++
> > > > > > >>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65
> > > > > ++++++++-
> > > > > > >>>>> 3 files changed, 241 insertions(+), 4 deletions(-)
> > > > > > >>>>>
> > > > > > >>>>> --
> > > > > > >>>>> 2.14.1.windows.1
> > > > > > >>>>>
> > > > > > >>>>> _______________________________________________
> > > > > > >>>>> edk2-devel mailing list
> > > > > > >>>>> edk2-devel@lists.01.org
> > > > > > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Wang, Jian J]

That's a better idea. Maybe we don't to free them but just reserve it for future uses.
The reserved 2MB page table can manage 1GB memory. I think in BIOS time typical
memory usage I observed is far less than that.

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Thursday, November 30, 2017 9:59 AM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> 
> Clarify my word below:
> 
> Can you just predict and pre-allocate additional pages for future split in worst
> case?
> If new one need split, you can just use the additional pages.
> If not, you can free them later.
> 
> For example, you can allocate 2M aligned memory for new pages, for make sure
> they are in same page directory.
> 
> I think it can resolve the problem.
> The key is to pre-calculate how many pages are involved in the worst case.
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Thursday, November 30, 2017 9:47 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>
> > Cc: edk2-devel@lists.01.org
> > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> >
> > Good idea. I think it will highly reduce the possibility but not solve it still.
> > The dilemma here is: do we need to take care of it? This issue exists in theory
> > but has not yet been encountered in practice.
> >
> > > -----Original Message-----
> > > From: Yao, Jiewen
> > > Sent: Thursday, November 30, 2017 9:36 AM
> > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > Cc: edk2-devel@lists.01.org
> > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > >
> > > Can you just allocate 1 more page for split?
> > > If new one need split, you can just use the additional page.
> > > If not, you can free it later.
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wang, Jian J
> > > > Sent: Thursday, November 30, 2017 9:17 AM
> > > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > > Cc: edk2-devel@lists.01.org
> > > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > > >
> > > > When you split page tables, you need to allocate new pages for new page
> > > table.
> > > > Since you have new page tables added, you need to mark them to be
> > read-only
> > > > as well, right? When you do this, the page table for the memory newly
> > > allocated
> > > > might still needs to be split. This is the worst case but there's still chance of
> it,
> > > > right? We cannot guarantee, theoretically, the page table for the newly
> > > allocated
> > > > page tables is in the same as just split ones. So, in worst case, once we
> want
> > to
> > > > mark new page table to be read-only, we need to split the page table
> > > managing
> > > > those memory, and if we need to do split, we need to allocate more new
> > > pages.
> > > > This might fall into a recursive situation until all the smallest page table are
> > > > created.
> > > > In practice it's almost impossible to let it happen but the chances are we
> will
> > > fall
> > > > into
> > > > such recursive situation more than one level.
> > > >
> > > > > -----Original Message-----
> > > > > From: Yao, Jiewen
> > > > > Sent: Thursday, November 30, 2017 8:52 AM
> > > > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > Cc: edk2-devel@lists.01.org
> > > > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > > > >
> > > > > -- whenever you're trying to mark one page used as page table to be
> read-
> > > only,
> > > > > you need to split its page table in advance
> > > > > [Jiewen] Sorry, I do not quite understand the problem statement.
> > > > > To set a page to be ReadOnly, you just flip the R/W bit in the page entry.
> > > > > The step I expect is:
> > > > > 1) You split page table.
> > > > > 2) Fill the data structure in the new entry.
> > > > > 3) Flip R/W bit (<== This is the new step).
> > > > > 4) FlushPageTable.
> > > > >
> > > > > Thank you
> > > > > Yao Jiewen
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Wang, Jian J
> > > > > > Sent: Thursday, November 30, 2017 8:44 AM
> > > > > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > > > > Cc: edk2-devel@lists.01.org
> > > > > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > > > > >
> > > > > > I did think of protecting new page table in cpu driver. But I think there's
> > > risks
> > > > > > to do it, which is that there might be a chance that, whenever you're
> > trying
> > > to
> > > > > > mark one page used as page table to be read-only, you need to split its
> > > page
> > > > > > table in advance, and again and again, until all page tables are for 4KB
> > > pages.
> > > > > >
> > > > > > Although this is a rare case, or the page table "split" will just happen for
> a
> > > few
> > > > > > times, it will slow down the boot process a little bit anyway. So I though
> > > it's
> > > > > > safer not to protect new page tables created after DxeIpl.
> > > > > >
> > > > > > Maybe it's just over worrying about it. Maybe we just need to add a
> PCD
> > to
> > > > > > turn on/off it just in case. Do you have any ideas in mind?
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Yao, Jiewen
> > > > > > > Sent: Wednesday, November 29, 2017 9:35 PM
> > > > > > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > > > Cc: edk2-devel@lists.01.org
> > > > > > > Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > > > >
> > > > > > > I do think we need update CPU driver to protect new allocated split
> > page.
> > > > > > >
> > > > > > > If you verified in shell env, I think it will exposed, please add a test to
> > > trigger
> > > > > > > page split in CPU driver.
> > > > > > >
> > > > > > > I recommend to write some unit test to parse page table in shell.
> > > > > > >
> > > > > > > thank you!
> > > > > > > Yao, Jiewen
> > > > > > >
> > > > > > >
> > > > > > > > 在 2017年11月29日，下午8:15，Wang, Jian J
> > > <jian.j.wang@intel.com>
> > > > > > 写
> > > > > > > 道：
> > > > > > > >
> > > > > > > > It's in the DxeIplPeim.
> > > > > > > >
> > > > > > > > By the way, there's an issue in this patch. I forgot to protect page
> table
> > > for
> > > > > > 32-
> > > > > > > bit mode.
> > > > > > > > So this patch works only for 64-bit mode. I'll add it in v2 patch.
> > > > > > > >
> > > > > > > >> -----Original Message-----
> > > > > > > >> From: Yao, Jiewen
> > > > > > > >> Sent: Wednesday, November 29, 2017 6:56 PM
> > > > > > > >> To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > > > >> Cc: edk2-devel@lists.01.org
> > > > > > > >> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > > > > >>
> > > > > > > >> Is this code in CPU driver?
> > > > > > > >>
> > > > > > > >> thank you!
> > > > > > > >> Yao, Jiewen
> > > > > > > >>
> > > > > > > >>
> > > > > > > >>> 在 2017年11月29日，下午6:24，Wang, Jian J
> > > > > <jian.j.wang@intel.com>
> > > > > > > 写
> > > > > > > >> 道：
> > > > > > > >>>
> > > > > > > >>> Yes, I validated them manually with JTAG debug tool.
> > > > > > > >>>
> > > > > > > >>> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
> > > > > > > >>>  // 1G page. Split to 2M.
> > > > > > > >>>  L2PageTable = AllocatePages (1);
> > > > > > > >>>  ASSERT (L2PageTable != NULL);
> > > > > > > >>>   PhysicalAddress = L3PageTable[Index3] &
> > > > > > PAGING_1G_ADDRESS_MASK_64;
> > > > > > > >>>   for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index)
> > {
> > > > > > > >>>    L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
> > > > > > > >>>                         IA32_PG_PS | IA32_PG_P |
> > > > IA32_PG_RW;
> > > > > > > >>>    PhysicalAddress += SIZE_2MB;
> > > > > > > >>>  }
> > > > > > > >>>  L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable |
> > > > > > AddressEncMask |
> > > > > > > >>>                                         IA32_PG_P |
> > > > > > IA32_PG_RW;
> > > > > > > >>>  SetPageReadOnly (PageTableBase,
> > > > > > > >> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
> > > > > > > >>> }
> > > > > > > >>>
> > > > > > > >>> The newly allocated page table is set in the SetPageReadOnly()
> itself
> > > > > > > >> recursively, like
> > > > > > > >>> above code in which L2PageTable is allocated and then set it to
> be
> > > > > > read-only
> > > > > > > >> after
> > > > > > > >>> initializing the table content.
> > > > > > > >>>
> > > > > > > >>>> -----Original Message-----
> > > > > > > >>>> From: Yao, Jiewen
> > > > > > > >>>> Sent: Wednesday, November 29, 2017 5:16 PM
> > > > > > > >>>> To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > > > >>>> Cc: edk2-devel@lists.01.org
> > > > > > > >>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write
> > protection
> > > > > > > >>>>
> > > > > > > >>>> Thanks.
> > > > > > > >>>>
> > > > > > > >>>> May I know if this is validated in uefi shell, that all page table is
> > > > > readonly?
> > > > > > > >>>>
> > > > > > > >>>> I did not find the code to set new allocated split page to be
> > > readonly.
> > > > > Can
> > > > > > > you
> > > > > > > >>>> give me a hand on that?
> > > > > > > >>>>
> > > > > > > >>>> thank you!
> > > > > > > >>>> Yao, Jiewen
> > > > > > > >>>>
> > > > > > > >>>>
> > > > > > > >>>>> 在 2017年11月29日，下午4:47，Jian J Wang
> > > > > > <jian.j.wang@intel.com>
> > > > > > > >> 写
> > > > > > > >>>> 道：
> > > > > > > >>>>>
> > > > > > > >>>>> 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):
> > > > > > > >>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page
> table
> > > > > > > >>>>> MdeModulePkg/DxeIpl: Mark page table as read-only
> > > > > > > >>>>>
> > > > > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
> > > > > > > >>>> +++++++++++++++++++++++
> > > > > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14
> > ++
> > > > > > > >>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65
> > > > > > ++++++++-
> > > > > > > >>>>> 3 files changed, 241 insertions(+), 4 deletions(-)
> > > > > > > >>>>>
> > > > > > > >>>>> --
> > > > > > > >>>>> 2.14.1.windows.1
> > > > > > > >>>>>
> > > > > > > >>>>> _______________________________________________
> > > > > > > >>>>> edk2-devel mailing list
> > > > > > > >>>>> edk2-devel@lists.01.org
> > > > > > > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Wang, Jian J]

I also noticed that, in the cpu driver, there's a potential recursive calling of 
CpuArchProtocol->SetMemoryAttributes() which is caused by page table allocation:

    (cpu)SetMemoryAttributes() -> (cpu) AssignMemoryPageAttributes () ->
                                                        -> (cpu) ConvertMemoryPageAttributes() ->
                                                        -> (cpu) SplitPage() ->
                                                        -> (cpu) AllocatePagesFunc() ->
                                                        -> (core) CoreAllocatePages() ->
                                                        -> (core)ApplyMemoryProtectionPolicy() -> 
-> (cpu)SetMemoryAttributes()

This has caused a problem in protecting page tables. Maybe your idea can help
to solve it too.


> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, November 30, 2017 10:02 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> 
> That's a better idea. Maybe we don't to free them but just reserve it for future
> uses.
> The reserved 2MB page table can manage 1GB memory. I think in BIOS time
> typical
> memory usage I observed is far less than that.
> 
> > -----Original Message-----
> > From: Yao, Jiewen
> > Sent: Thursday, November 30, 2017 9:59 AM
> > To: Wang, Jian J <jian.j.wang@intel.com>
> > Cc: edk2-devel@lists.01.org
> > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> >
> > Clarify my word below:
> >
> > Can you just predict and pre-allocate additional pages for future split in worst
> > case?
> > If new one need split, you can just use the additional pages.
> > If not, you can free them later.
> >
> > For example, you can allocate 2M aligned memory for new pages, for make
> sure
> > they are in same page directory.
> >
> > I think it can resolve the problem.
> > The key is to pre-calculate how many pages are involved in the worst case.
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Wang, Jian J
> > > Sent: Thursday, November 30, 2017 9:47 AM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > Cc: edk2-devel@lists.01.org
> > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > >
> > > Good idea. I think it will highly reduce the possibility but not solve it still.
> > > The dilemma here is: do we need to take care of it? This issue exists in theory
> > > but has not yet been encountered in practice.
> > >
> > > > -----Original Message-----
> > > > From: Yao, Jiewen
> > > > Sent: Thursday, November 30, 2017 9:36 AM
> > > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > > Cc: edk2-devel@lists.01.org
> > > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > > >
> > > > Can you just allocate 1 more page for split?
> > > > If new one need split, you can just use the additional page.
> > > > If not, you can free it later.
> > > >
> > > > Thank you
> > > > Yao Jiewen
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Jian J
> > > > > Sent: Thursday, November 30, 2017 9:17 AM
> > > > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > > > Cc: edk2-devel@lists.01.org
> > > > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > > > >
> > > > > When you split page tables, you need to allocate new pages for new
> page
> > > > table.
> > > > > Since you have new page tables added, you need to mark them to be
> > > read-only
> > > > > as well, right? When you do this, the page table for the memory newly
> > > > allocated
> > > > > might still needs to be split. This is the worst case but there's still chance
> of
> > it,
> > > > > right? We cannot guarantee, theoretically, the page table for the newly
> > > > allocated
> > > > > page tables is in the same as just split ones. So, in worst case, once we
> > want
> > > to
> > > > > mark new page table to be read-only, we need to split the page table
> > > > managing
> > > > > those memory, and if we need to do split, we need to allocate more new
> > > > pages.
> > > > > This might fall into a recursive situation until all the smallest page table
> are
> > > > > created.
> > > > > In practice it's almost impossible to let it happen but the chances are we
> > will
> > > > fall
> > > > > into
> > > > > such recursive situation more than one level.
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Yao, Jiewen
> > > > > > Sent: Thursday, November 30, 2017 8:52 AM
> > > > > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > > Cc: edk2-devel@lists.01.org
> > > > > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > > > > >
> > > > > > -- whenever you're trying to mark one page used as page table to be
> > read-
> > > > only,
> > > > > > you need to split its page table in advance
> > > > > > [Jiewen] Sorry, I do not quite understand the problem statement.
> > > > > > To set a page to be ReadOnly, you just flip the R/W bit in the page entry.
> > > > > > The step I expect is:
> > > > > > 1) You split page table.
> > > > > > 2) Fill the data structure in the new entry.
> > > > > > 3) Flip R/W bit (<== This is the new step).
> > > > > > 4) FlushPageTable.
> > > > > >
> > > > > > Thank you
> > > > > > Yao Jiewen
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Wang, Jian J
> > > > > > > Sent: Thursday, November 30, 2017 8:44 AM
> > > > > > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > > > > > Cc: edk2-devel@lists.01.org
> > > > > > > Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> > > > > > >
> > > > > > > I did think of protecting new page table in cpu driver. But I think
> there's
> > > > risks
> > > > > > > to do it, which is that there might be a chance that, whenever you're
> > > trying
> > > > to
> > > > > > > mark one page used as page table to be read-only, you need to split
> its
> > > > page
> > > > > > > table in advance, and again and again, until all page tables are for 4KB
> > > > pages.
> > > > > > >
> > > > > > > Although this is a rare case, or the page table "split" will just happen
> for
> > a
> > > > few
> > > > > > > times, it will slow down the boot process a little bit anyway. So I
> though
> > > > it's
> > > > > > > safer not to protect new page tables created after DxeIpl.
> > > > > > >
> > > > > > > Maybe it's just over worrying about it. Maybe we just need to add a
> > PCD
> > > to
> > > > > > > turn on/off it just in case. Do you have any ideas in mind?
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Yao, Jiewen
> > > > > > > > Sent: Wednesday, November 29, 2017 9:35 PM
> > > > > > > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > > > > Cc: edk2-devel@lists.01.org
> > > > > > > > Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> > > > > > > >
> > > > > > > > I do think we need update CPU driver to protect new allocated split
> > > page.
> > > > > > > >
> > > > > > > > If you verified in shell env, I think it will exposed, please add a test
> to
> > > > trigger
> > > > > > > > page split in CPU driver.
> > > > > > > >
> > > > > > > > I recommend to write some unit test to parse page table in shell.
> > > > > > > >
> > > > > > > > thank you!
> > > > > > > > Yao, Jiewen
> > > > > > > >
> > > > > > > >
> > > > > > > > > 在 2017年11月29日，下午8:15，Wang, Jian J
> > > > <jian.j.wang@intel.com>
> > > > > > > 写
> > > > > > > > 道：
> > > > > > > > >
> > > > > > > > > It's in the DxeIplPeim.
> > > > > > > > >
> > > > > > > > > By the way, there's an issue in this patch. I forgot to protect page
> > table
> > > > for
> > > > > > > 32-
> > > > > > > > bit mode.
> > > > > > > > > So this patch works only for 64-bit mode. I'll add it in v2 patch.
> > > > > > > > >
> > > > > > > > >> -----Original Message-----
> > > > > > > > >> From: Yao, Jiewen
> > > > > > > > >> Sent: Wednesday, November 29, 2017 6:56 PM
> > > > > > > > >> To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > > > > >> Cc: edk2-devel@lists.01.org
> > > > > > > > >> Subject: Re: [edk2] [PATCH 0/2] Enable page table write
> protection
> > > > > > > > >>
> > > > > > > > >> Is this code in CPU driver?
> > > > > > > > >>
> > > > > > > > >> thank you!
> > > > > > > > >> Yao, Jiewen
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > > >>> 在 2017年11月29日，下午6:24，Wang, Jian J
> > > > > > <jian.j.wang@intel.com>
> > > > > > > > 写
> > > > > > > > >> 道：
> > > > > > > > >>>
> > > > > > > > >>> Yes, I validated them manually with JTAG debug tool.
> > > > > > > > >>>
> > > > > > > > >>> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
> > > > > > > > >>>  // 1G page. Split to 2M.
> > > > > > > > >>>  L2PageTable = AllocatePages (1);
> > > > > > > > >>>  ASSERT (L2PageTable != NULL);
> > > > > > > > >>>   PhysicalAddress = L3PageTable[Index3] &
> > > > > > > PAGING_1G_ADDRESS_MASK_64;
> > > > > > > > >>>   for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64);
> ++Index)
> > > {
> > > > > > > > >>>    L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
> > > > > > > > >>>                         IA32_PG_PS | IA32_PG_P |
> > > > > IA32_PG_RW;
> > > > > > > > >>>    PhysicalAddress += SIZE_2MB;
> > > > > > > > >>>  }
> > > > > > > > >>>  L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable |
> > > > > > > AddressEncMask |
> > > > > > > > >>>                                         IA32_PG_P |
> > > > > > > IA32_PG_RW;
> > > > > > > > >>>  SetPageReadOnly (PageTableBase,
> > > > > > > > >> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
> > > > > > > > >>> }
> > > > > > > > >>>
> > > > > > > > >>> The newly allocated page table is set in the SetPageReadOnly()
> > itself
> > > > > > > > >> recursively, like
> > > > > > > > >>> above code in which L2PageTable is allocated and then set it to
> > be
> > > > > > > read-only
> > > > > > > > >> after
> > > > > > > > >>> initializing the table content.
> > > > > > > > >>>
> > > > > > > > >>>> -----Original Message-----
> > > > > > > > >>>> From: Yao, Jiewen
> > > > > > > > >>>> Sent: Wednesday, November 29, 2017 5:16 PM
> > > > > > > > >>>> To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > > > > >>>> Cc: edk2-devel@lists.01.org
> > > > > > > > >>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write
> > > protection
> > > > > > > > >>>>
> > > > > > > > >>>> Thanks.
> > > > > > > > >>>>
> > > > > > > > >>>> May I know if this is validated in uefi shell, that all page table is
> > > > > > readonly?
> > > > > > > > >>>>
> > > > > > > > >>>> I did not find the code to set new allocated split page to be
> > > > readonly.
> > > > > > Can
> > > > > > > > you
> > > > > > > > >>>> give me a hand on that?
> > > > > > > > >>>>
> > > > > > > > >>>> thank you!
> > > > > > > > >>>> Yao, Jiewen
> > > > > > > > >>>>
> > > > > > > > >>>>
> > > > > > > > >>>>> 在 2017年11月29日，下午4:47，Jian J Wang
> > > > > > > <jian.j.wang@intel.com>
> > > > > > > > >> 写
> > > > > > > > >>>> 道：
> > > > > > > > >>>>>
> > > > > > > > >>>>> 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):
> > > > > > > > >>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page
> > table
> > > > > > > > >>>>> MdeModulePkg/DxeIpl: Mark page table as read-only
> > > > > > > > >>>>>
> > > > > > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c |
> 166
> > > > > > > > >>>> +++++++++++++++++++++++
> > > > > > > > >>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |
> 14
> > > ++
> > > > > > > > >>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65
> > > > > > > ++++++++-
> > > > > > > > >>>>> 3 files changed, 241 insertions(+), 4 deletions(-)
> > > > > > > > >>>>>
> > > > > > > > >>>>> --
> > > > > > > > >>>>> 2.14.1.windows.1
> > > > > > > > >>>>>
> > > > > > > > >>>>> _______________________________________________
> > > > > > > > >>>>> edk2-devel mailing list
> > > > > > > > >>>>> edk2-devel@lists.01.org
> > > > > > > > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Andrew Fish]

> On Nov 29, 2017, at 5:16 PM, Wang, Jian J <jian.j.wang@intel.com> wrote:
> 
> When you split page tables, you need to allocate new pages for new page table.
> Since you have new page tables added, you need to mark them to be read-only
> as well, right? When you do this, the page table for the memory newly allocated
> might still needs to be split. This is the worst case but there's still chance of it,
> right? We cannot guarantee, theoretically, the page table for the newly allocated
> page tables is in the same as just split ones. So, in worst case, once we want to
> mark new page table to be read-only, we need to split the page table managing
> those memory, and if we need to do split, we need to allocate more new pages.
> This might fall into a recursive situation until all the smallest page table are created.
> In practice it's almost impossible to let it happen but the chances are we will fall into 
> such recursive situation more than one level.
> 

Why not just have the page table update code turn off the write protect when it is modifying tables, and then turn it back on? Or am I missing something?

Thanks,

Andrew Fish

>> -----Original Message-----
>> From: Yao, Jiewen
>> Sent: Thursday, November 30, 2017 8:52 AM
>> To: Wang, Jian J <jian.j.wang@intel.com>
>> Cc: edk2-devel@lists.01.org
>> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
>> 
>> -- whenever you're trying to mark one page used as page table to be read-only,
>> you need to split its page table in advance
>> [Jiewen] Sorry, I do not quite understand the problem statement.
>> To set a page to be ReadOnly, you just flip the R/W bit in the page entry.
>> The step I expect is:
>> 1) You split page table.
>> 2) Fill the data structure in the new entry.
>> 3) Flip R/W bit (<== This is the new step).
>> 4) FlushPageTable.
>> 
>> Thank you
>> Yao Jiewen
>> 
>>> -----Original Message-----
>>> From: Wang, Jian J
>>> Sent: Thursday, November 30, 2017 8:44 AM
>>> To: Yao, Jiewen <jiewen.yao@intel.com>
>>> Cc: edk2-devel@lists.01.org
>>> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
>>> 
>>> I did think of protecting new page table in cpu driver. But I think there's risks
>>> to do it, which is that there might be a chance that, whenever you're trying to
>>> mark one page used as page table to be read-only, you need to split its page
>>> table in advance, and again and again, until all page tables are for 4KB pages.
>>> 
>>> Although this is a rare case, or the page table "split" will just happen for a few
>>> times, it will slow down the boot process a little bit anyway. So I though  it's
>>> safer not to protect new page tables created after DxeIpl.
>>> 
>>> Maybe it's just over worrying about it. Maybe we just need to add a PCD to
>>> turn on/off it just in case. Do you have any ideas in mind?
>>> 
>>>> -----Original Message-----
>>>> From: Yao, Jiewen
>>>> Sent: Wednesday, November 29, 2017 9:35 PM
>>>> To: Wang, Jian J <jian.j.wang@intel.com>
>>>> Cc: edk2-devel@lists.01.org
>>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
>>>> 
>>>> I do think we need update CPU driver to protect new allocated split page.
>>>> 
>>>> If you verified in shell env, I think it will exposed, please add a test to trigger
>>>> page split in CPU driver.
>>>> 
>>>> I recommend to write some unit test to parse page table in shell.
>>>> 
>>>> thank you!
>>>> Yao, Jiewen
>>>> 
>>>> 
>>>>> 在 2017年11月29日，下午8:15，Wang, Jian J <jian.j.wang@intel.com>
>>> 写
>>>> 道：
>>>>> 
>>>>> It's in the DxeIplPeim.
>>>>> 
>>>>> By the way, there's an issue in this patch. I forgot to protect page table for
>>> 32-
>>>> bit mode.
>>>>> So this patch works only for 64-bit mode. I'll add it in v2 patch.
>>>>> 
>>>>>> -----Original Message-----
>>>>>> From: Yao, Jiewen
>>>>>> Sent: Wednesday, November 29, 2017 6:56 PM
>>>>>> To: Wang, Jian J <jian.j.wang@intel.com>
>>>>>> Cc: edk2-devel@lists.01.org
>>>>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
>>>>>> 
>>>>>> Is this code in CPU driver?
>>>>>> 
>>>>>> thank you!
>>>>>> Yao, Jiewen
>>>>>> 
>>>>>> 
>>>>>>> 在 2017年11月29日，下午6:24，Wang, Jian J
>> <jian.j.wang@intel.com>
>>>> 写
>>>>>> 道：
>>>>>>> 
>>>>>>> Yes, I validated them manually with JTAG debug tool.
>>>>>>> 
>>>>>>> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
>>>>>>> // 1G page. Split to 2M.
>>>>>>> L2PageTable = AllocatePages (1);
>>>>>>> ASSERT (L2PageTable != NULL);
>>>>>>>  PhysicalAddress = L3PageTable[Index3] &
>>> PAGING_1G_ADDRESS_MASK_64;
>>>>>>>  for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
>>>>>>>   L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
>>>>>>>                        IA32_PG_PS | IA32_PG_P | IA32_PG_RW;
>>>>>>>   PhysicalAddress += SIZE_2MB;
>>>>>>> }
>>>>>>> L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable |
>>> AddressEncMask |
>>>>>>>                                        IA32_PG_P |
>>> IA32_PG_RW;
>>>>>>> SetPageReadOnly (PageTableBase,
>>>>>> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
>>>>>>> }
>>>>>>> 
>>>>>>> The newly allocated page table is set in the SetPageReadOnly() itself
>>>>>> recursively, like
>>>>>>> above code in which L2PageTable is allocated and then set it to be
>>> read-only
>>>>>> after
>>>>>>> initializing the table content.
>>>>>>> 
>>>>>>>> -----Original Message-----
>>>>>>>> From: Yao, Jiewen
>>>>>>>> Sent: Wednesday, November 29, 2017 5:16 PM
>>>>>>>> To: Wang, Jian J <jian.j.wang@intel.com>
>>>>>>>> Cc: edk2-devel@lists.01.org
>>>>>>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
>>>>>>>> 
>>>>>>>> Thanks.
>>>>>>>> 
>>>>>>>> May I know if this is validated in uefi shell, that all page table is
>> readonly?
>>>>>>>> 
>>>>>>>> I did not find the code to set new allocated split page to be readonly.
>> Can
>>>> you
>>>>>>>> give me a hand on that?
>>>>>>>> 
>>>>>>>> thank you!
>>>>>>>> Yao, Jiewen
>>>>>>>> 
>>>>>>>> 
>>>>>>>>> 在 2017年11月29日，下午4:47，Jian J Wang
>>> <jian.j.wang@intel.com>
>>>>>> 写
>>>>>>>> 道：
>>>>>>>>> 
>>>>>>>>> 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):
>>>>>>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
>>>>>>>>> MdeModulePkg/DxeIpl: Mark page table as read-only
>>>>>>>>> 
>>>>>>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
>>>>>>>> +++++++++++++++++++++++
>>>>>>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
>>>>>>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65
>>> ++++++++-
>>>>>>>>> 3 files changed, 241 insertions(+), 4 deletions(-)
>>>>>>>>> 
>>>>>>>>> --
>>>>>>>>> 2.14.1.windows.1
>>>>>>>>> 
>>>>>>>>> _______________________________________________
>>>>>>>>> edk2-devel mailing list
>>>>>>>>> edk2-devel@lists.01.org
>>>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Wang, Jian J]

Hi Andrew,

I did turn on/off write protection when I update page attributes. What we're discussing
is the worst case in which protecting the newly added page table will fall into a very deep
recursive loop. This is possible in theory but not sure it will really happen in practice.

> -----Original Message-----
> From: afish@apple.com [mailto:afish@apple.com]
> Sent: Thursday, November 30, 2017 9:38 AM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> 
> 
> 
> > On Nov 29, 2017, at 5:16 PM, Wang, Jian J <jian.j.wang@intel.com> wrote:
> >
> > When you split page tables, you need to allocate new pages for new page
> table.
> > Since you have new page tables added, you need to mark them to be read-only
> > as well, right? When you do this, the page table for the memory newly
> allocated
> > might still needs to be split. This is the worst case but there's still chance of it,
> > right? We cannot guarantee, theoretically, the page table for the newly
> allocated
> > page tables is in the same as just split ones. So, in worst case, once we want to
> > mark new page table to be read-only, we need to split the page table
> managing
> > those memory, and if we need to do split, we need to allocate more new
> pages.
> > This might fall into a recursive situation until all the smallest page table are
> created.
> > In practice it's almost impossible to let it happen but the chances are we will
> fall into
> > such recursive situation more than one level.
> >
> 
> Why not just have the page table update code turn off the write protect when it
> is modifying tables, and then turn it back on? Or am I missing something?
> 
> Thanks,
> 
> Andrew Fish
> 
> >> -----Original Message-----
> >> From: Yao, Jiewen
> >> Sent: Thursday, November 30, 2017 8:52 AM
> >> To: Wang, Jian J <jian.j.wang@intel.com>
> >> Cc: edk2-devel@lists.01.org
> >> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> >>
> >> -- whenever you're trying to mark one page used as page table to be read-
> only,
> >> you need to split its page table in advance
> >> [Jiewen] Sorry, I do not quite understand the problem statement.
> >> To set a page to be ReadOnly, you just flip the R/W bit in the page entry.
> >> The step I expect is:
> >> 1) You split page table.
> >> 2) Fill the data structure in the new entry.
> >> 3) Flip R/W bit (<== This is the new step).
> >> 4) FlushPageTable.
> >>
> >> Thank you
> >> Yao Jiewen
> >>
> >>> -----Original Message-----
> >>> From: Wang, Jian J
> >>> Sent: Thursday, November 30, 2017 8:44 AM
> >>> To: Yao, Jiewen <jiewen.yao@intel.com>
> >>> Cc: edk2-devel@lists.01.org
> >>> Subject: RE: [edk2] [PATCH 0/2] Enable page table write protection
> >>>
> >>> I did think of protecting new page table in cpu driver. But I think there's risks
> >>> to do it, which is that there might be a chance that, whenever you're trying
> to
> >>> mark one page used as page table to be read-only, you need to split its page
> >>> table in advance, and again and again, until all page tables are for 4KB pages.
> >>>
> >>> Although this is a rare case, or the page table "split" will just happen for a
> few
> >>> times, it will slow down the boot process a little bit anyway. So I though  it's
> >>> safer not to protect new page tables created after DxeIpl.
> >>>
> >>> Maybe it's just over worrying about it. Maybe we just need to add a PCD to
> >>> turn on/off it just in case. Do you have any ideas in mind?
> >>>
> >>>> -----Original Message-----
> >>>> From: Yao, Jiewen
> >>>> Sent: Wednesday, November 29, 2017 9:35 PM
> >>>> To: Wang, Jian J <jian.j.wang@intel.com>
> >>>> Cc: edk2-devel@lists.01.org
> >>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> >>>>
> >>>> I do think we need update CPU driver to protect new allocated split page.
> >>>>
> >>>> If you verified in shell env, I think it will exposed, please add a test to
> trigger
> >>>> page split in CPU driver.
> >>>>
> >>>> I recommend to write some unit test to parse page table in shell.
> >>>>
> >>>> thank you!
> >>>> Yao, Jiewen
> >>>>
> >>>>
> >>>>> 在 2017年11月29日，下午8:15，Wang, Jian J
> <jian.j.wang@intel.com>
> >>> 写
> >>>> 道：
> >>>>>
> >>>>> It's in the DxeIplPeim.
> >>>>>
> >>>>> By the way, there's an issue in this patch. I forgot to protect page table
> for
> >>> 32-
> >>>> bit mode.
> >>>>> So this patch works only for 64-bit mode. I'll add it in v2 patch.
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Yao, Jiewen
> >>>>>> Sent: Wednesday, November 29, 2017 6:56 PM
> >>>>>> To: Wang, Jian J <jian.j.wang@intel.com>
> >>>>>> Cc: edk2-devel@lists.01.org
> >>>>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> >>>>>>
> >>>>>> Is this code in CPU driver?
> >>>>>>
> >>>>>> thank you!
> >>>>>> Yao, Jiewen
> >>>>>>
> >>>>>>
> >>>>>>> 在 2017年11月29日，下午6:24，Wang, Jian J
> >> <jian.j.wang@intel.com>
> >>>> 写
> >>>>>> 道：
> >>>>>>>
> >>>>>>> Yes, I validated them manually with JTAG debug tool.
> >>>>>>>
> >>>>>>> if ((L3PageTable[Index3] & IA32_PG_PS) != 0) {
> >>>>>>> // 1G page. Split to 2M.
> >>>>>>> L2PageTable = AllocatePages (1);
> >>>>>>> ASSERT (L2PageTable != NULL);
> >>>>>>>  PhysicalAddress = L3PageTable[Index3] &
> >>> PAGING_1G_ADDRESS_MASK_64;
> >>>>>>>  for (Index = 0; Index < EFI_PAGE_SIZE/sizeof (UINT64); ++Index) {
> >>>>>>>   L2PageTable[Index] = PhysicalAddress  | AddressEncMask |
> >>>>>>>                        IA32_PG_PS | IA32_PG_P | IA32_PG_RW;
> >>>>>>>   PhysicalAddress += SIZE_2MB;
> >>>>>>> }
> >>>>>>> L3PageTable[Index3] = (UINT64) (UINTN) L2PageTable |
> >>> AddressEncMask |
> >>>>>>>                                        IA32_PG_P |
> >>> IA32_PG_RW;
> >>>>>>> SetPageReadOnly (PageTableBase,
> >>>>>> (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable);
> >>>>>>> }
> >>>>>>>
> >>>>>>> The newly allocated page table is set in the SetPageReadOnly() itself
> >>>>>> recursively, like
> >>>>>>> above code in which L2PageTable is allocated and then set it to be
> >>> read-only
> >>>>>> after
> >>>>>>> initializing the table content.
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Yao, Jiewen
> >>>>>>>> Sent: Wednesday, November 29, 2017 5:16 PM
> >>>>>>>> To: Wang, Jian J <jian.j.wang@intel.com>
> >>>>>>>> Cc: edk2-devel@lists.01.org
> >>>>>>>> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> >>>>>>>>
> >>>>>>>> Thanks.
> >>>>>>>>
> >>>>>>>> May I know if this is validated in uefi shell, that all page table is
> >> readonly?
> >>>>>>>>
> >>>>>>>> I did not find the code to set new allocated split page to be readonly.
> >> Can
> >>>> you
> >>>>>>>> give me a hand on that?
> >>>>>>>>
> >>>>>>>> thank you!
> >>>>>>>> Yao, Jiewen
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> 在 2017年11月29日，下午4:47，Jian J Wang
> >>> <jian.j.wang@intel.com>
> >>>>>> 写
> >>>>>>>> 道：
> >>>>>>>>>
> >>>>>>>>> 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):
> >>>>>>>>> UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
> >>>>>>>>> MdeModulePkg/DxeIpl: Mark page table as read-only
> >>>>>>>>>
> >>>>>>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
> >>>>>>>> +++++++++++++++++++++++
> >>>>>>>>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
> >>>>>>>>> UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65
> >>> ++++++++-
> >>>>>>>>> 3 files changed, 241 insertions(+), 4 deletions(-)
> >>>>>>>>>
> >>>>>>>>> --
> >>>>>>>>> 2.14.1.windows.1
> >>>>>>>>>
> >>>>>>>>> _______________________________________________
> >>>>>>>>> edk2-devel mailing list
> >>>>>>>>> edk2-devel@lists.01.org
> >>>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] Enable page table write protection

[Laszlo Ersek]

Hi Jian,

On 11/29/17 09:46, Jian J Wang wrote:
> 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):
>   UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
>   MdeModulePkg/DxeIpl: Mark page table as read-only
> 
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166 +++++++++++++++++++++++
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
>  UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65 ++++++++-
>  3 files changed, 241 insertions(+), 4 deletions(-)
> 

thanks for the CC -- I'd like to test this feature.

Is it OK if I wait 1-2 days first, to see if there is review feedback
that requires a v2?

Thanks!
Laszlo

Re: [PATCH 0/2] Enable page table write protection

[Wang, Jian J]

There's already issue found. So there must be v2. Please wait for it.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, November 29, 2017 8:39 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Enable page table write protection
> 
> Hi Jian,
> 
> On 11/29/17 09:46, Jian J Wang wrote:
> > 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):
> >   UefiCpuPkg/CpuDxe: Check CR0.WP before changing page table
> >   MdeModulePkg/DxeIpl: Mark page table as read-only
> >
> >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 166
> +++++++++++++++++++++++
> >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  14 ++
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c                 |  65 ++++++++-
> >  3 files changed, 241 insertions(+), 4 deletions(-)
> >
> 
> thanks for the CC -- I'd like to test this feature.
> 
> Is it OK if I wait 1-2 days first, to see if there is review feedback
> that requires a v2?
> 
> Thanks!
> Laszlo