[edk2] [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection

Jian J Wang posted 6 patches 7 years, 2 months ago
There is a newer version of this series
[edk2] [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
Posted by Jian J Wang 7 years, 2 months ago
> According to Jiewen's feedback, change the page split condition
> for NULL pointer detection to exclude IsExecuteDisableBitAvailable()
> (Ia32/DxeLoadFunc.c)

NULL pointer detection is done by making use of paging mechanism of CPU.
During page table setup, if enabled, the first 4-K page (0-4095) will be
marked as NOT PRESENT. Any code which unintentionally access memory between
0-4095 will trigger a Page Fault exception which warns users that there's
potential illegal code in BIOS.

This also means that legacy code which has to access memory between 0-4095
should be cautious to temporarily disable this feature before the access
and re-enable it afterwards; or disalbe this feature at all.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ayellet Wolman <ayellet.wolman@intel.com>
Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            | 25 +++++++++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c           | 65 ++++++++++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  | 11 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 31 ++++++++---
 6 files changed, 126 insertions(+), 9 deletions(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
index 72d2532f50..1654bcd2dc 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
@@ -240,4 +240,29 @@ Decompress (
   OUT       UINTN                   *OutputSize
   );
 
+/**
+   Clear legacy memory located at the first 4K-page.
+
+   This function traverses the whole HOB list to check if memory from 0 to 4095
+   exists and has not been allocated, and then clear it if so.
+
+   @param HoStart         The start of HobList passed to DxeCore.
+
+**/
+VOID
+ClearLegacyMemory (
+  IN  VOID *HobStart
+  );
+
+/**
+   Return configure status of NULL pointer detection feature
+
+   @return TRUE   NULL pointer detection feature is enabled
+   @return FALSE  NULL pointer detection feature is disabled
+**/
+BOOLEAN
+IsNullDetectionEnabled (
+  VOID
+  );
+
 #endif
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index c54afe4aa6..9d0e76a293 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -115,6 +115,7 @@
 [Pcd.IA32,Pcd.X64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable                      ## SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask    ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
 
 [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ## SOMETIMES_CONSUMES
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
index 50b5440d15..0a71b1f3de 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
@@ -825,3 +825,68 @@ UpdateStackHob (
     Hob.Raw = GET_NEXT_HOB (Hob);
   }
 }
+
+/**
+   Clear legacy memory located at the first 4K-page, if available.
+
+   This function traverses the whole HOB list to check if memory from 0 to 4095
+   exists and has not been allocated, and then clear it if so.
+
+   @param HoStart                   The start of HobList passed to DxeCore.
+
+**/
+VOID
+ClearLegacyMemory (
+  IN  VOID *HobStart
+  )
+{
+  EFI_PEI_HOB_POINTERS          RscHob;
+  EFI_PEI_HOB_POINTERS          MemHob;
+  BOOLEAN                       DoClear;
+
+  RscHob.Raw = HobStart;
+  MemHob.Raw = HobStart;
+  DoClear = FALSE;
+
+  //
+  // Check if page 0 exists and free
+  //
+  while ((RscHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
+                                   RscHob.Raw)) != NULL) {
+    if (RscHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY &&
+        RscHob.ResourceDescriptor->PhysicalStart == 0) {
+      DoClear = TRUE;
+      //
+      // Make sure memory at 0-4095 has not been allocated.
+      //
+      while ((MemHob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION,
+                                       MemHob.Raw)) != NULL) {
+        if (MemHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress
+            < EFI_PAGE_SIZE) {
+          DoClear = FALSE;
+          break;
+        }
+        MemHob.Raw = GET_NEXT_HOB (MemHob);
+      }
+      break;
+    }
+    RscHob.Raw = GET_NEXT_HOB (RscHob);
+  }
+
+  if (DoClear) {
+    DEBUG ((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
+    SetMem (NULL, EFI_PAGE_SIZE, 0);
+  }
+
+  return;
+}
+
+BOOLEAN
+IsNullDetectionEnabled (
+  VOID
+  )
+{
+  return (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0) ?
+          TRUE : FALSE);
+}
+
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index 1957326caf..7a8421dd16 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -123,7 +123,9 @@ Create4GPageTablesIa32Pae (
     PageDirectoryPointerEntry->Bits.Present = 1;
 
     for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress += SIZE_2MB) {
-      if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase)) {
+      if ((IsNullDetectionEnabled () && PhysicalAddress == 0)
+          || ((PhysicalAddress < StackBase + StackSize)
+              && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
         //
         // Need to split this 2M page that covers stack range.
         //
@@ -240,6 +242,8 @@ HandOffToDxeCore (
   EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
   BOOLEAN                   BuildPageTablesIa32Pae;
 
+  ClearLegacyMemory (HobList.Raw);
+
   Status = PeiServicesAllocatePages (EfiBootServicesData, EFI_SIZE_TO_PAGES (STACK_SIZE), &BaseOfStack);
   ASSERT_EFI_ERROR (Status);
 
@@ -379,7 +383,10 @@ HandOffToDxeCore (
     TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
 
     PageTables = 0;
-    BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) && IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
+    BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () &&
+                                        (IsNullDetectionEnabled () ||
+                                         (PcdGetBool (PcdSetNxForStack) &&
+                                          IsExecuteDisableBitAvailable ())));
     if (BuildPageTablesIa32Pae) {
       PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
       EnableExecuteDisableBit ();
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
index 6488880eab..d93a9c5a2d 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
@@ -42,6 +42,8 @@ HandOffToDxeCore (
   EFI_VECTOR_HANDOFF_INFO         *VectorInfo;
   EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
 
+  ClearLegacyMemory (HobList.Raw);
+
   //
   // Get Vector Hand-off Info PPI and build Guided HOB
   //
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index 48150be4e1..80c1821eca 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -90,8 +90,16 @@ Split2MPageTo4K (
     //
     PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
     PageTableEntry->Bits.ReadWrite = 1;
-    PageTableEntry->Bits.Present = 1;
-    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + StackSize)) {
+
+    if (IsNullDetectionEnabled () && PhysicalAddress4K == 0) {
+      PageTableEntry->Bits.Present = 0;
+    } else {
+      PageTableEntry->Bits.Present = 1;
+    }
+
+    if (PcdGetBool (PcdSetNxForStack)
+        && (PhysicalAddress4K >= StackBase)
+        && (PhysicalAddress4K < StackBase + StackSize)) {
       //
       // Set Nx bit for stack.
       //
@@ -137,9 +145,12 @@ Split1GPageTo2M (
 
   PhysicalAddress2M = PhysicalAddress;
   for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += SIZE_2MB) {
-    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M + SIZE_2MB) > StackBase)) {
+    if ((IsNullDetectionEnabled () && PhysicalAddress2M == 0)
+        || (PcdGetBool (PcdSetNxForStack)
+            && (PhysicalAddress2M < StackBase + StackSize)
+            && ((PhysicalAddress2M + SIZE_2MB) > StackBase))) {
       //
-      // Need to split this 2M page that covers stack range.
+      // Need to split this 2M page that covers NULL or stack range.
       //
       Split2MPageTo4K (PhysicalAddress2M, (UINT64 *) PageDirectoryEntry, StackBase, StackSize);
     } else {
@@ -279,7 +290,10 @@ CreateIdentityMappingPageTables (
       PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
     
       for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress += SIZE_1GB) {
-        if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + StackSize) && ((PageAddress + SIZE_1GB) > StackBase)) {
+        if ((IsNullDetectionEnabled () && PageAddress == 0)
+            || (PcdGetBool (PcdSetNxForStack)
+                && (PageAddress < StackBase + StackSize)
+                && ((PageAddress + SIZE_1GB) > StackBase))) {
           Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
         } else {
           //
@@ -308,9 +322,12 @@ CreateIdentityMappingPageTables (
         PageDirectoryPointerEntry->Bits.Present = 1;
 
         for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress += SIZE_2MB) {
-          if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + StackSize) && ((PageAddress + SIZE_2MB) > StackBase)) {
+          if ((IsNullDetectionEnabled () && PageAddress == 0)
+              || (PcdGetBool (PcdSetNxForStack)
+                  && (PageAddress < StackBase + StackSize)
+                  && ((PageAddress + SIZE_2MB) > StackBase))) {
             //
-            // Need to split this 2M page that covers stack range.
+            // Need to split this 2M page that covers NULL or stack range.
             //
             Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry, StackBase, StackSize);
           } else {
-- 
2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
Posted by Zeng, Star 7 years, 2 months ago
Minor comments to this patch.

1. IsNullDetectionEnabled() function is put in DxeLoad.c that is shared by all ARCHs, and the function is consuming PcdNullPointerDetectionPropertyMask, but PcdNullPointerDetectionPropertyMask is only declared in "[Pcd.IA32,Pcd.X64]" in inf.
I am not sure whether it will cause build failure or not for non-IA32/X64 ARCHs.

2. How about using name "ClearFirst4KPage" instead of "ClearLegacyMemory" to be more clear?


Thanks,
Star
-----Original Message-----
From: Wang, Jian J 
Sent: Thursday, September 28, 2017 9:04 AM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>
Subject: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection

> According to Jiewen's feedback, change the page split condition for 
> NULL pointer detection to exclude IsExecuteDisableBitAvailable()
> (Ia32/DxeLoadFunc.c)

NULL pointer detection is done by making use of paging mechanism of CPU.
During page table setup, if enabled, the first 4-K page (0-4095) will be marked as NOT PRESENT. Any code which unintentionally access memory between
0-4095 will trigger a Page Fault exception which warns users that there's potential illegal code in BIOS.

This also means that legacy code which has to access memory between 0-4095 should be cautious to temporarily disable this feature before the access and re-enable it afterwards; or disalbe this feature at all.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ayellet Wolman <ayellet.wolman@intel.com>
Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            | 25 +++++++++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c           | 65 ++++++++++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  | 11 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 31 ++++++++---
 6 files changed, 126 insertions(+), 9 deletions(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
index 72d2532f50..1654bcd2dc 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
@@ -240,4 +240,29 @@ Decompress (
   OUT       UINTN                   *OutputSize
   );
 
+/**
+   Clear legacy memory located at the first 4K-page.
+
+   This function traverses the whole HOB list to check if memory from 0 to 4095
+   exists and has not been allocated, and then clear it if so.
+
+   @param HoStart         The start of HobList passed to DxeCore.
+
+**/
+VOID
+ClearLegacyMemory (
+  IN  VOID *HobStart
+  );
+
+/**
+   Return configure status of NULL pointer detection feature
+
+   @return TRUE   NULL pointer detection feature is enabled
+   @return FALSE  NULL pointer detection feature is disabled **/ 
+BOOLEAN IsNullDetectionEnabled (
+  VOID
+  );
+
 #endif
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index c54afe4aa6..9d0e76a293 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -115,6 +115,7 @@
 [Pcd.IA32,Pcd.X64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable                      ## SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask    ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
 
 [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ## SOMETIMES_CONSUMES
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
index 50b5440d15..0a71b1f3de 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
@@ -825,3 +825,68 @@ UpdateStackHob (
     Hob.Raw = GET_NEXT_HOB (Hob);
   }
 }
+
+/**
+   Clear legacy memory located at the first 4K-page, if available.
+
+   This function traverses the whole HOB list to check if memory from 0 to 4095
+   exists and has not been allocated, and then clear it if so.
+
+   @param HoStart                   The start of HobList passed to DxeCore.
+
+**/
+VOID
+ClearLegacyMemory (
+  IN  VOID *HobStart
+  )
+{
+  EFI_PEI_HOB_POINTERS          RscHob;
+  EFI_PEI_HOB_POINTERS          MemHob;
+  BOOLEAN                       DoClear;
+
+  RscHob.Raw = HobStart;
+  MemHob.Raw = HobStart;
+  DoClear = FALSE;
+
+  //
+  // Check if page 0 exists and free
+  //
+  while ((RscHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
+                                   RscHob.Raw)) != NULL) {
+    if (RscHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY &&
+        RscHob.ResourceDescriptor->PhysicalStart == 0) {
+      DoClear = TRUE;
+      //
+      // Make sure memory at 0-4095 has not been allocated.
+      //
+      while ((MemHob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION,
+                                       MemHob.Raw)) != NULL) {
+        if (MemHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress
+            < EFI_PAGE_SIZE) {
+          DoClear = FALSE;
+          break;
+        }
+        MemHob.Raw = GET_NEXT_HOB (MemHob);
+      }
+      break;
+    }
+    RscHob.Raw = GET_NEXT_HOB (RscHob);  }
+
+  if (DoClear) {
+    DEBUG ((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
+    SetMem (NULL, EFI_PAGE_SIZE, 0);
+  }
+
+  return;
+}
+
+BOOLEAN
+IsNullDetectionEnabled (
+  VOID
+  )
+{
+  return (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0) ?
+          TRUE : FALSE);
+}
+
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index 1957326caf..7a8421dd16 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -123,7 +123,9 @@ Create4GPageTablesIa32Pae (
     PageDirectoryPointerEntry->Bits.Present = 1;
 
     for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress += SIZE_2MB) {
-      if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase)) {
+      if ((IsNullDetectionEnabled () && PhysicalAddress == 0)
+          || ((PhysicalAddress < StackBase + StackSize)
+              && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
         //
         // Need to split this 2M page that covers stack range.
         //
@@ -240,6 +242,8 @@ HandOffToDxeCore (
   EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
   BOOLEAN                   BuildPageTablesIa32Pae;
 
+  ClearLegacyMemory (HobList.Raw);
+
   Status = PeiServicesAllocatePages (EfiBootServicesData, EFI_SIZE_TO_PAGES (STACK_SIZE), &BaseOfStack);
   ASSERT_EFI_ERROR (Status);
 
@@ -379,7 +383,10 @@ HandOffToDxeCore (
     TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
 
     PageTables = 0;
-    BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) && IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
+    BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () &&
+                                        (IsNullDetectionEnabled () ||
+                                         (PcdGetBool (PcdSetNxForStack) &&
+                                          IsExecuteDisableBitAvailable 
+ ())));
     if (BuildPageTablesIa32Pae) {
       PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
       EnableExecuteDisableBit ();
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
index 6488880eab..d93a9c5a2d 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
@@ -42,6 +42,8 @@ HandOffToDxeCore (
   EFI_VECTOR_HANDOFF_INFO         *VectorInfo;
   EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
 
+  ClearLegacyMemory (HobList.Raw);
+
   //
   // Get Vector Hand-off Info PPI and build Guided HOB
   //
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index 48150be4e1..80c1821eca 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -90,8 +90,16 @@ Split2MPageTo4K (
     //
     PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
     PageTableEntry->Bits.ReadWrite = 1;
-    PageTableEntry->Bits.Present = 1;
-    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + StackSize)) {
+
+    if (IsNullDetectionEnabled () && PhysicalAddress4K == 0) {
+      PageTableEntry->Bits.Present = 0;
+    } else {
+      PageTableEntry->Bits.Present = 1;
+    }
+
+    if (PcdGetBool (PcdSetNxForStack)
+        && (PhysicalAddress4K >= StackBase)
+        && (PhysicalAddress4K < StackBase + StackSize)) {
       //
       // Set Nx bit for stack.
       //
@@ -137,9 +145,12 @@ Split1GPageTo2M (
 
   PhysicalAddress2M = PhysicalAddress;
   for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += SIZE_2MB) {
-    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M + SIZE_2MB) > StackBase)) {
+    if ((IsNullDetectionEnabled () && PhysicalAddress2M == 0)
+        || (PcdGetBool (PcdSetNxForStack)
+            && (PhysicalAddress2M < StackBase + StackSize)
+            && ((PhysicalAddress2M + SIZE_2MB) > StackBase))) {
       //
-      // Need to split this 2M page that covers stack range.
+      // Need to split this 2M page that covers NULL or stack range.
       //
       Split2MPageTo4K (PhysicalAddress2M, (UINT64 *) PageDirectoryEntry, StackBase, StackSize);
     } else {
@@ -279,7 +290,10 @@ CreateIdentityMappingPageTables (
       PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
     
       for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress += SIZE_1GB) {
-        if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + StackSize) && ((PageAddress + SIZE_1GB) > StackBase)) {
+        if ((IsNullDetectionEnabled () && PageAddress == 0)
+            || (PcdGetBool (PcdSetNxForStack)
+                && (PageAddress < StackBase + StackSize)
+                && ((PageAddress + SIZE_1GB) > StackBase))) {
           Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
         } else {
           //
@@ -308,9 +322,12 @@ CreateIdentityMappingPageTables (
         PageDirectoryPointerEntry->Bits.Present = 1;
 
         for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress += SIZE_2MB) {
-          if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + StackSize) && ((PageAddress + SIZE_2MB) > StackBase)) {
+          if ((IsNullDetectionEnabled () && PageAddress == 0)
+              || (PcdGetBool (PcdSetNxForStack)
+                  && (PageAddress < StackBase + StackSize)
+                  && ((PageAddress + SIZE_2MB) > StackBase))) {
             //
-            // Need to split this 2M page that covers stack range.
+            // Need to split this 2M page that covers NULL or stack range.
             //
             Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry, StackBase, StackSize);
           } else {
--
2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
Posted by Zeng, Star 7 years, 2 months ago
Another comment.
Should IsNullDetectionEnabled() be checked before calling ClearLegacyMemory()?


Thanks,
Star
-----Original Message-----
From: Zeng, Star 
Sent: Thursday, September 28, 2017 11:24 AM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection

Minor comments to this patch.

1. IsNullDetectionEnabled() function is put in DxeLoad.c that is shared by all ARCHs, and the function is consuming PcdNullPointerDetectionPropertyMask, but PcdNullPointerDetectionPropertyMask is only declared in "[Pcd.IA32,Pcd.X64]" in inf.
I am not sure whether it will cause build failure or not for non-IA32/X64 ARCHs.

2. How about using name "ClearFirst4KPage" instead of "ClearLegacyMemory" to be more clear?


Thanks,
Star
-----Original Message-----
From: Wang, Jian J
Sent: Thursday, September 28, 2017 9:04 AM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>
Subject: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection

> According to Jiewen's feedback, change the page split condition for 
> NULL pointer detection to exclude IsExecuteDisableBitAvailable()
> (Ia32/DxeLoadFunc.c)

NULL pointer detection is done by making use of paging mechanism of CPU.
During page table setup, if enabled, the first 4-K page (0-4095) will be marked as NOT PRESENT. Any code which unintentionally access memory between
0-4095 will trigger a Page Fault exception which warns users that there's potential illegal code in BIOS.

This also means that legacy code which has to access memory between 0-4095 should be cautious to temporarily disable this feature before the access and re-enable it afterwards; or disalbe this feature at all.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ayellet Wolman <ayellet.wolman@intel.com>
Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            | 25 +++++++++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c           | 65 ++++++++++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  | 11 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 31 ++++++++---
 6 files changed, 126 insertions(+), 9 deletions(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
index 72d2532f50..1654bcd2dc 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
@@ -240,4 +240,29 @@ Decompress (
   OUT       UINTN                   *OutputSize
   );
 
+/**
+   Clear legacy memory located at the first 4K-page.
+
+   This function traverses the whole HOB list to check if memory from 0 to 4095
+   exists and has not been allocated, and then clear it if so.
+
+   @param HoStart         The start of HobList passed to DxeCore.
+
+**/
+VOID
+ClearLegacyMemory (
+  IN  VOID *HobStart
+  );
+
+/**
+   Return configure status of NULL pointer detection feature
+
+   @return TRUE   NULL pointer detection feature is enabled
+   @return FALSE  NULL pointer detection feature is disabled **/ 
+BOOLEAN IsNullDetectionEnabled (
+  VOID
+  );
+
 #endif
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index c54afe4aa6..9d0e76a293 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -115,6 +115,7 @@
 [Pcd.IA32,Pcd.X64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable                      ## SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask    ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
 
 [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ## SOMETIMES_CONSUMES
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
index 50b5440d15..0a71b1f3de 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
@@ -825,3 +825,68 @@ UpdateStackHob (
     Hob.Raw = GET_NEXT_HOB (Hob);
   }
 }
+
+/**
+   Clear legacy memory located at the first 4K-page, if available.
+
+   This function traverses the whole HOB list to check if memory from 0 to 4095
+   exists and has not been allocated, and then clear it if so.
+
+   @param HoStart                   The start of HobList passed to DxeCore.
+
+**/
+VOID
+ClearLegacyMemory (
+  IN  VOID *HobStart
+  )
+{
+  EFI_PEI_HOB_POINTERS          RscHob;
+  EFI_PEI_HOB_POINTERS          MemHob;
+  BOOLEAN                       DoClear;
+
+  RscHob.Raw = HobStart;
+  MemHob.Raw = HobStart;
+  DoClear = FALSE;
+
+  //
+  // Check if page 0 exists and free
+  //
+  while ((RscHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
+                                   RscHob.Raw)) != NULL) {
+    if (RscHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY &&
+        RscHob.ResourceDescriptor->PhysicalStart == 0) {
+      DoClear = TRUE;
+      //
+      // Make sure memory at 0-4095 has not been allocated.
+      //
+      while ((MemHob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION,
+                                       MemHob.Raw)) != NULL) {
+        if (MemHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress
+            < EFI_PAGE_SIZE) {
+          DoClear = FALSE;
+          break;
+        }
+        MemHob.Raw = GET_NEXT_HOB (MemHob);
+      }
+      break;
+    }
+    RscHob.Raw = GET_NEXT_HOB (RscHob);  }
+
+  if (DoClear) {
+    DEBUG ((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
+    SetMem (NULL, EFI_PAGE_SIZE, 0);
+  }
+
+  return;
+}
+
+BOOLEAN
+IsNullDetectionEnabled (
+  VOID
+  )
+{
+  return (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0) ?
+          TRUE : FALSE);
+}
+
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index 1957326caf..7a8421dd16 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -123,7 +123,9 @@ Create4GPageTablesIa32Pae (
     PageDirectoryPointerEntry->Bits.Present = 1;
 
     for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress += SIZE_2MB) {
-      if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase)) {
+      if ((IsNullDetectionEnabled () && PhysicalAddress == 0)
+          || ((PhysicalAddress < StackBase + StackSize)
+              && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
         //
         // Need to split this 2M page that covers stack range.
         //
@@ -240,6 +242,8 @@ HandOffToDxeCore (
   EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
   BOOLEAN                   BuildPageTablesIa32Pae;
 
+  ClearLegacyMemory (HobList.Raw);
+
   Status = PeiServicesAllocatePages (EfiBootServicesData, EFI_SIZE_TO_PAGES (STACK_SIZE), &BaseOfStack);
   ASSERT_EFI_ERROR (Status);
 
@@ -379,7 +383,10 @@ HandOffToDxeCore (
     TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
 
     PageTables = 0;
-    BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) && IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
+    BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () &&
+                                        (IsNullDetectionEnabled () ||
+                                         (PcdGetBool (PcdSetNxForStack) &&
+                                          IsExecuteDisableBitAvailable 
+ ())));
     if (BuildPageTablesIa32Pae) {
       PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
       EnableExecuteDisableBit ();
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
index 6488880eab..d93a9c5a2d 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
@@ -42,6 +42,8 @@ HandOffToDxeCore (
   EFI_VECTOR_HANDOFF_INFO         *VectorInfo;
   EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
 
+  ClearLegacyMemory (HobList.Raw);
+
   //
   // Get Vector Hand-off Info PPI and build Guided HOB
   //
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index 48150be4e1..80c1821eca 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -90,8 +90,16 @@ Split2MPageTo4K (
     //
     PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
     PageTableEntry->Bits.ReadWrite = 1;
-    PageTableEntry->Bits.Present = 1;
-    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + StackSize)) {
+
+    if (IsNullDetectionEnabled () && PhysicalAddress4K == 0) {
+      PageTableEntry->Bits.Present = 0;
+    } else {
+      PageTableEntry->Bits.Present = 1;
+    }
+
+    if (PcdGetBool (PcdSetNxForStack)
+        && (PhysicalAddress4K >= StackBase)
+        && (PhysicalAddress4K < StackBase + StackSize)) {
       //
       // Set Nx bit for stack.
       //
@@ -137,9 +145,12 @@ Split1GPageTo2M (
 
   PhysicalAddress2M = PhysicalAddress;
   for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += SIZE_2MB) {
-    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M + SIZE_2MB) > StackBase)) {
+    if ((IsNullDetectionEnabled () && PhysicalAddress2M == 0)
+        || (PcdGetBool (PcdSetNxForStack)
+            && (PhysicalAddress2M < StackBase + StackSize)
+            && ((PhysicalAddress2M + SIZE_2MB) > StackBase))) {
       //
-      // Need to split this 2M page that covers stack range.
+      // Need to split this 2M page that covers NULL or stack range.
       //
       Split2MPageTo4K (PhysicalAddress2M, (UINT64 *) PageDirectoryEntry, StackBase, StackSize);
     } else {
@@ -279,7 +290,10 @@ CreateIdentityMappingPageTables (
       PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
     
       for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress += SIZE_1GB) {
-        if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + StackSize) && ((PageAddress + SIZE_1GB) > StackBase)) {
+        if ((IsNullDetectionEnabled () && PageAddress == 0)
+            || (PcdGetBool (PcdSetNxForStack)
+                && (PageAddress < StackBase + StackSize)
+                && ((PageAddress + SIZE_1GB) > StackBase))) {
           Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
         } else {
           //
@@ -308,9 +322,12 @@ CreateIdentityMappingPageTables (
         PageDirectoryPointerEntry->Bits.Present = 1;
 
         for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress += SIZE_2MB) {
-          if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + StackSize) && ((PageAddress + SIZE_2MB) > StackBase)) {
+          if ((IsNullDetectionEnabled () && PageAddress == 0)
+              || (PcdGetBool (PcdSetNxForStack)
+                  && (PageAddress < StackBase + StackSize)
+                  && ((PageAddress + SIZE_2MB) > StackBase))) {
             //
-            // Need to split this 2M page that covers stack range.
+            // Need to split this 2M page that covers NULL or stack range.
             //
             Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry, StackBase, StackSize);
           } else {
--
2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
Posted by Wang, Jian J 7 years, 2 months ago
Clearing this block of memory has nothing to do with NULL pointer detection.
I'm not sure the extra check is necessary.

> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 11:31 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer
> detection
> 
> Another comment.
> Should IsNullDetectionEnabled() be checked before calling
> ClearLegacyMemory()?
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 11:24 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer
> detection
> 
> Minor comments to this patch.
> 
> 1. IsNullDetectionEnabled() function is put in DxeLoad.c that is shared by all
> ARCHs, and the function is consuming PcdNullPointerDetectionPropertyMask,
> but PcdNullPointerDetectionPropertyMask is only declared in
> "[Pcd.IA32,Pcd.X64]" in inf.
> I am not sure whether it will cause build failure or not for non-IA32/X64 ARCHs.
> 
> 2. How about using name "ClearFirst4KPage" instead of "ClearLegacyMemory"
> to be more clear?
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, September 28, 2017 9:04 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>
> Subject: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer
> detection
> 
> > According to Jiewen's feedback, change the page split condition for
> > NULL pointer detection to exclude IsExecuteDisableBitAvailable()
> > (Ia32/DxeLoadFunc.c)
> 
> NULL pointer detection is done by making use of paging mechanism of CPU.
> During page table setup, if enabled, the first 4-K page (0-4095) will be marked as
> NOT PRESENT. Any code which unintentionally access memory between
> 0-4095 will trigger a Page Fault exception which warns users that there's
> potential illegal code in BIOS.
> 
> This also means that legacy code which has to access memory between 0-4095
> should be cautious to temporarily disable this feature before the access and re-
> enable it afterwards; or disalbe this feature at all.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ayellet Wolman <ayellet.wolman@intel.com>
> Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            | 25 +++++++++
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>  MdeModulePkg/Core/DxeIplPeim/DxeLoad.c           | 65
> ++++++++++++++++++++++++
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  | 11 +++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 31 ++++++++---
>  6 files changed, 126 insertions(+), 9 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> index 72d2532f50..1654bcd2dc 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> @@ -240,4 +240,29 @@ Decompress (
>    OUT       UINTN                   *OutputSize
>    );
> 
> +/**
> +   Clear legacy memory located at the first 4K-page.
> +
> +   This function traverses the whole HOB list to check if memory from 0 to 4095
> +   exists and has not been allocated, and then clear it if so.
> +
> +   @param HoStart         The start of HobList passed to DxeCore.
> +
> +**/
> +VOID
> +ClearLegacyMemory (
> +  IN  VOID *HobStart
> +  );
> +
> +/**
> +   Return configure status of NULL pointer detection feature
> +
> +   @return TRUE   NULL pointer detection feature is enabled
> +   @return FALSE  NULL pointer detection feature is disabled **/
> +BOOLEAN IsNullDetectionEnabled (
> +  VOID
> +  );
> +
>  #endif
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index c54afe4aa6..9d0e76a293 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -115,6 +115,7 @@
>  [Pcd.IA32,Pcd.X64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable                      ##
> SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> ## CONSUMES
> 
>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
> SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> index 50b5440d15..0a71b1f3de 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> @@ -825,3 +825,68 @@ UpdateStackHob (
>      Hob.Raw = GET_NEXT_HOB (Hob);
>    }
>  }
> +
> +/**
> +   Clear legacy memory located at the first 4K-page, if available.
> +
> +   This function traverses the whole HOB list to check if memory from 0 to 4095
> +   exists and has not been allocated, and then clear it if so.
> +
> +   @param HoStart                   The start of HobList passed to DxeCore.
> +
> +**/
> +VOID
> +ClearLegacyMemory (
> +  IN  VOID *HobStart
> +  )
> +{
> +  EFI_PEI_HOB_POINTERS          RscHob;
> +  EFI_PEI_HOB_POINTERS          MemHob;
> +  BOOLEAN                       DoClear;
> +
> +  RscHob.Raw = HobStart;
> +  MemHob.Raw = HobStart;
> +  DoClear = FALSE;
> +
> +  //
> +  // Check if page 0 exists and free
> +  //
> +  while ((RscHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
> +                                   RscHob.Raw)) != NULL) {
> +    if (RscHob.ResourceDescriptor->ResourceType ==
> EFI_RESOURCE_SYSTEM_MEMORY &&
> +        RscHob.ResourceDescriptor->PhysicalStart == 0) {
> +      DoClear = TRUE;
> +      //
> +      // Make sure memory at 0-4095 has not been allocated.
> +      //
> +      while ((MemHob.Raw = GetNextHob
> (EFI_HOB_TYPE_MEMORY_ALLOCATION,
> +                                       MemHob.Raw)) != NULL) {
> +        if (MemHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress
> +            < EFI_PAGE_SIZE) {
> +          DoClear = FALSE;
> +          break;
> +        }
> +        MemHob.Raw = GET_NEXT_HOB (MemHob);
> +      }
> +      break;
> +    }
> +    RscHob.Raw = GET_NEXT_HOB (RscHob);  }
> +
> +  if (DoClear) {
> +    DEBUG ((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
> +    SetMem (NULL, EFI_PAGE_SIZE, 0);
> +  }
> +
> +  return;
> +}
> +
> +BOOLEAN
> +IsNullDetectionEnabled (
> +  VOID
> +  )
> +{
> +  return (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0) ?
> +          TRUE : FALSE);
> +}
> +
> diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> index 1957326caf..7a8421dd16 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> @@ -123,7 +123,9 @@ Create4GPageTablesIa32Pae (
>      PageDirectoryPointerEntry->Bits.Present = 1;
> 
>      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress +=
> SIZE_2MB) {
> -      if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress +
> SIZE_2MB) > StackBase)) {
> +      if ((IsNullDetectionEnabled () && PhysicalAddress == 0)
> +          || ((PhysicalAddress < StackBase + StackSize)
> +              && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
>          //
>          // Need to split this 2M page that covers stack range.
>          //
> @@ -240,6 +242,8 @@ HandOffToDxeCore (
>    EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
>    BOOLEAN                   BuildPageTablesIa32Pae;
> 
> +  ClearLegacyMemory (HobList.Raw);
> +
>    Status = PeiServicesAllocatePages (EfiBootServicesData, EFI_SIZE_TO_PAGES
> (STACK_SIZE), &BaseOfStack);
>    ASSERT_EFI_ERROR (Status);
> 
> @@ -379,7 +383,10 @@ HandOffToDxeCore (
>      TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER
> (TopOfStack, CPU_STACK_ALIGNMENT);
> 
>      PageTables = 0;
> -    BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) &&
> IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
> +    BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () &&
> +                                        (IsNullDetectionEnabled () ||
> +                                         (PcdGetBool (PcdSetNxForStack) &&
> +                                          IsExecuteDisableBitAvailable
> + ())));
>      if (BuildPageTablesIa32Pae) {
>        PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
>        EnableExecuteDisableBit ();
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> index 6488880eab..d93a9c5a2d 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> @@ -42,6 +42,8 @@ HandOffToDxeCore (
>    EFI_VECTOR_HANDOFF_INFO         *VectorInfo;
>    EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
> 
> +  ClearLegacyMemory (HobList.Raw);
> +
>    //
>    // Get Vector Hand-off Info PPI and build Guided HOB
>    //
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index 48150be4e1..80c1821eca 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -90,8 +90,16 @@ Split2MPageTo4K (
>      //
>      PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
>      PageTableEntry->Bits.ReadWrite = 1;
> -    PageTableEntry->Bits.Present = 1;
> -    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase +
> StackSize)) {
> +
> +    if (IsNullDetectionEnabled () && PhysicalAddress4K == 0) {
> +      PageTableEntry->Bits.Present = 0;
> +    } else {
> +      PageTableEntry->Bits.Present = 1;
> +    }
> +
> +    if (PcdGetBool (PcdSetNxForStack)
> +        && (PhysicalAddress4K >= StackBase)
> +        && (PhysicalAddress4K < StackBase + StackSize)) {
>        //
>        // Set Nx bit for stack.
>        //
> @@ -137,9 +145,12 @@ Split1GPageTo2M (
> 
>    PhysicalAddress2M = PhysicalAddress;
>    for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M +=
> SIZE_2MB) {
> -    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M +
> SIZE_2MB) > StackBase)) {
> +    if ((IsNullDetectionEnabled () && PhysicalAddress2M == 0)
> +        || (PcdGetBool (PcdSetNxForStack)
> +            && (PhysicalAddress2M < StackBase + StackSize)
> +            && ((PhysicalAddress2M + SIZE_2MB) > StackBase))) {
>        //
> -      // Need to split this 2M page that covers stack range.
> +      // Need to split this 2M page that covers NULL or stack range.
>        //
>        Split2MPageTo4K (PhysicalAddress2M, (UINT64 *) PageDirectoryEntry,
> StackBase, StackSize);
>      } else {
> @@ -279,7 +290,10 @@ CreateIdentityMappingPageTables (
>        PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> 
>        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress +=
> SIZE_1GB) {
> -        if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase +
> StackSize) && ((PageAddress + SIZE_1GB) > StackBase)) {
> +        if ((IsNullDetectionEnabled () && PageAddress == 0)
> +            || (PcdGetBool (PcdSetNxForStack)
> +                && (PageAddress < StackBase + StackSize)
> +                && ((PageAddress + SIZE_1GB) > StackBase))) {
>            Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry,
> StackBase, StackSize);
>          } else {
>            //
> @@ -308,9 +322,12 @@ CreateIdentityMappingPageTables (
>          PageDirectoryPointerEntry->Bits.Present = 1;
> 
>          for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
> SIZE_2MB) {
> -          if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase +
> StackSize) && ((PageAddress + SIZE_2MB) > StackBase)) {
> +          if ((IsNullDetectionEnabled () && PageAddress == 0)
> +              || (PcdGetBool (PcdSetNxForStack)
> +                  && (PageAddress < StackBase + StackSize)
> +                  && ((PageAddress + SIZE_2MB) > StackBase))) {
>              //
> -            // Need to split this 2M page that covers stack range.
> +            // Need to split this 2M page that covers NULL or stack range.
>              //
>              Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry,
> StackBase, StackSize);
>            } else {
> --
> 2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
Posted by Zeng, Star 7 years, 2 months ago
If NULL pointer detection is disabled, the code should be behave same with before, and ClearLegacyMemory() is not needed to be called because DxeCore will behave same with before to handle the first 4K page clear , right?


Thanks,
Star
-----Original Message-----
From: Wang, Jian J 
Sent: Thursday, September 28, 2017 11:55 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>
Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection

Clearing this block of memory has nothing to do with NULL pointer detection.
I'm not sure the extra check is necessary.

> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 11:31 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek 
> <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, 
> Michael D <michael.d.kinney@intel.com>; Justen, Jordan L 
> <jordan.l.justen@intel.com>; Wolman, Ayellet 
> <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL 
> pointer detection
> 
> Another comment.
> Should IsNullDetectionEnabled() be checked before calling 
> ClearLegacyMemory()?
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 11:24 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek 
> <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, 
> Michael D <michael.d.kinney@intel.com>; Justen, Jordan L 
> <jordan.l.justen@intel.com>; Wolman, Ayellet 
> <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL 
> pointer detection
> 
> Minor comments to this patch.
> 
> 1. IsNullDetectionEnabled() function is put in DxeLoad.c that is 
> shared by all ARCHs, and the function is consuming 
> PcdNullPointerDetectionPropertyMask,
> but PcdNullPointerDetectionPropertyMask is only declared in 
> "[Pcd.IA32,Pcd.X64]" in inf.
> I am not sure whether it will cause build failure or not for non-IA32/X64 ARCHs.
> 
> 2. How about using name "ClearFirst4KPage" instead of "ClearLegacyMemory"
> to be more clear?
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, September 28, 2017 9:04 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric 
> <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen 
> <jiewen.yao@intel.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>; Justen, Jordan L 
> <jordan.l.justen@intel.com>; Wolman, Ayellet 
> <ayellet.wolman@intel.com>
> Subject: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer 
> detection
> 
> > According to Jiewen's feedback, change the page split condition for 
> > NULL pointer detection to exclude IsExecuteDisableBitAvailable()
> > (Ia32/DxeLoadFunc.c)
> 
> NULL pointer detection is done by making use of paging mechanism of CPU.
> During page table setup, if enabled, the first 4-K page (0-4095) will 
> be marked as NOT PRESENT. Any code which unintentionally access memory 
> between
> 0-4095 will trigger a Page Fault exception which warns users that 
> there's potential illegal code in BIOS.
> 
> This also means that legacy code which has to access memory between 
> 0-4095 should be cautious to temporarily disable this feature before 
> the access and re- enable it afterwards; or disalbe this feature at all.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ayellet Wolman <ayellet.wolman@intel.com>
> Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            | 25 +++++++++
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>  MdeModulePkg/Core/DxeIplPeim/DxeLoad.c           | 65
> ++++++++++++++++++++++++
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  | 11 +++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 31 ++++++++---
>  6 files changed, 126 insertions(+), 9 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> index 72d2532f50..1654bcd2dc 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> @@ -240,4 +240,29 @@ Decompress (
>    OUT       UINTN                   *OutputSize
>    );
> 
> +/**
> +   Clear legacy memory located at the first 4K-page.
> +
> +   This function traverses the whole HOB list to check if memory from 0 to 4095
> +   exists and has not been allocated, and then clear it if so.
> +
> +   @param HoStart         The start of HobList passed to DxeCore.
> +
> +**/
> +VOID
> +ClearLegacyMemory (
> +  IN  VOID *HobStart
> +  );
> +
> +/**
> +   Return configure status of NULL pointer detection feature
> +
> +   @return TRUE   NULL pointer detection feature is enabled
> +   @return FALSE  NULL pointer detection feature is disabled **/ 
> +BOOLEAN IsNullDetectionEnabled (
> +  VOID
> +  );
> +
>  #endif
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index c54afe4aa6..9d0e76a293 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -115,6 +115,7 @@
>  [Pcd.IA32,Pcd.X64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable                      ##
> SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> ## CONSUMES
> 
>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
> SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> index 50b5440d15..0a71b1f3de 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> @@ -825,3 +825,68 @@ UpdateStackHob (
>      Hob.Raw = GET_NEXT_HOB (Hob);
>    }
>  }
> +
> +/**
> +   Clear legacy memory located at the first 4K-page, if available.
> +
> +   This function traverses the whole HOB list to check if memory from 0 to 4095
> +   exists and has not been allocated, and then clear it if so.
> +
> +   @param HoStart                   The start of HobList passed to DxeCore.
> +
> +**/
> +VOID
> +ClearLegacyMemory (
> +  IN  VOID *HobStart
> +  )
> +{
> +  EFI_PEI_HOB_POINTERS          RscHob;
> +  EFI_PEI_HOB_POINTERS          MemHob;
> +  BOOLEAN                       DoClear;
> +
> +  RscHob.Raw = HobStart;
> +  MemHob.Raw = HobStart;
> +  DoClear = FALSE;
> +
> +  //
> +  // Check if page 0 exists and free
> +  //
> +  while ((RscHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
> +                                   RscHob.Raw)) != NULL) {
> +    if (RscHob.ResourceDescriptor->ResourceType ==
> EFI_RESOURCE_SYSTEM_MEMORY &&
> +        RscHob.ResourceDescriptor->PhysicalStart == 0) {
> +      DoClear = TRUE;
> +      //
> +      // Make sure memory at 0-4095 has not been allocated.
> +      //
> +      while ((MemHob.Raw = GetNextHob
> (EFI_HOB_TYPE_MEMORY_ALLOCATION,
> +                                       MemHob.Raw)) != NULL) {
> +        if (MemHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress
> +            < EFI_PAGE_SIZE) {
> +          DoClear = FALSE;
> +          break;
> +        }
> +        MemHob.Raw = GET_NEXT_HOB (MemHob);
> +      }
> +      break;
> +    }
> +    RscHob.Raw = GET_NEXT_HOB (RscHob);  }
> +
> +  if (DoClear) {
> +    DEBUG ((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
> +    SetMem (NULL, EFI_PAGE_SIZE, 0);
> +  }
> +
> +  return;
> +}
> +
> +BOOLEAN
> +IsNullDetectionEnabled (
> +  VOID
> +  )
> +{
> +  return (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0) ?
> +          TRUE : FALSE);
> +}
> +
> diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> index 1957326caf..7a8421dd16 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> @@ -123,7 +123,9 @@ Create4GPageTablesIa32Pae (
>      PageDirectoryPointerEntry->Bits.Present = 1;
> 
>      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries 
> < 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress 
> IndexOfPageDirectoryEntries+++=
> SIZE_2MB) {
> -      if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress +
> SIZE_2MB) > StackBase)) {
> +      if ((IsNullDetectionEnabled () && PhysicalAddress == 0)
> +          || ((PhysicalAddress < StackBase + StackSize)
> +              && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
>          //
>          // Need to split this 2M page that covers stack range.
>          //
> @@ -240,6 +242,8 @@ HandOffToDxeCore (
>    EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
>    BOOLEAN                   BuildPageTablesIa32Pae;
> 
> +  ClearLegacyMemory (HobList.Raw);
> +
>    Status = PeiServicesAllocatePages (EfiBootServicesData, 
> EFI_SIZE_TO_PAGES (STACK_SIZE), &BaseOfStack);
>    ASSERT_EFI_ERROR (Status);
> 
> @@ -379,7 +383,10 @@ HandOffToDxeCore (
>      TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER 
> (TopOfStack, CPU_STACK_ALIGNMENT);
> 
>      PageTables = 0;
> -    BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) &&
> IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
> +    BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () &&
> +                                        (IsNullDetectionEnabled () ||
> +                                         (PcdGetBool (PcdSetNxForStack) &&
> +                                          
> + IsExecuteDisableBitAvailable ())));
>      if (BuildPageTablesIa32Pae) {
>        PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
>        EnableExecuteDisableBit ();
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> index 6488880eab..d93a9c5a2d 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> @@ -42,6 +42,8 @@ HandOffToDxeCore (
>    EFI_VECTOR_HANDOFF_INFO         *VectorInfo;
>    EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
> 
> +  ClearLegacyMemory (HobList.Raw);
> +
>    //
>    // Get Vector Hand-off Info PPI and build Guided HOB
>    //
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index 48150be4e1..80c1821eca 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -90,8 +90,16 @@ Split2MPageTo4K (
>      //
>      PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
>      PageTableEntry->Bits.ReadWrite = 1;
> -    PageTableEntry->Bits.Present = 1;
> -    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase +
> StackSize)) {
> +
> +    if (IsNullDetectionEnabled () && PhysicalAddress4K == 0) {
> +      PageTableEntry->Bits.Present = 0;
> +    } else {
> +      PageTableEntry->Bits.Present = 1;
> +    }
> +
> +    if (PcdGetBool (PcdSetNxForStack)
> +        && (PhysicalAddress4K >= StackBase)
> +        && (PhysicalAddress4K < StackBase + StackSize)) {
>        //
>        // Set Nx bit for stack.
>        //
> @@ -137,9 +145,12 @@ Split1GPageTo2M (
> 
>    PhysicalAddress2M = PhysicalAddress;
>    for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 
> 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M 
> IndexOfPageDirectoryEntries+++=
> SIZE_2MB) {
> -    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M +
> SIZE_2MB) > StackBase)) {
> +    if ((IsNullDetectionEnabled () && PhysicalAddress2M == 0)
> +        || (PcdGetBool (PcdSetNxForStack)
> +            && (PhysicalAddress2M < StackBase + StackSize)
> +            && ((PhysicalAddress2M + SIZE_2MB) > StackBase))) {
>        //
> -      // Need to split this 2M page that covers stack range.
> +      // Need to split this 2M page that covers NULL or stack range.
>        //
>        Split2MPageTo4K (PhysicalAddress2M, (UINT64 *) 
> PageDirectoryEntry, StackBase, StackSize);
>      } else {
> @@ -279,7 +290,10 @@ CreateIdentityMappingPageTables (
>        PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> 
>        for (IndexOfPageDirectoryEntries = 0; 
> IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress +=
> SIZE_1GB) {
> -        if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase +
> StackSize) && ((PageAddress + SIZE_1GB) > StackBase)) {
> +        if ((IsNullDetectionEnabled () && PageAddress == 0)
> +            || (PcdGetBool (PcdSetNxForStack)
> +                && (PageAddress < StackBase + StackSize)
> +                && ((PageAddress + SIZE_1GB) > StackBase))) {
>            Split1GPageTo2M (PageAddress, (UINT64 *) 
> PageDirectory1GEntry, StackBase, StackSize);
>          } else {
>            //
> @@ -308,9 +322,12 @@ CreateIdentityMappingPageTables (
>          PageDirectoryPointerEntry->Bits.Present = 1;
> 
>          for (IndexOfPageDirectoryEntries = 0; 
> IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
> SIZE_2MB) {
> -          if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase +
> StackSize) && ((PageAddress + SIZE_2MB) > StackBase)) {
> +          if ((IsNullDetectionEnabled () && PageAddress == 0)
> +              || (PcdGetBool (PcdSetNxForStack)
> +                  && (PageAddress < StackBase + StackSize)
> +                  && ((PageAddress + SIZE_2MB) > StackBase))) {
>              //
> -            // Need to split this 2M page that covers stack range.
> +            // Need to split this 2M page that covers NULL or stack range.
>              //
>              Split2MPageTo4K (PageAddress, (UINT64 *) 
> PageDirectoryEntry, StackBase, StackSize);
>            } else {
> --
> 2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
Posted by Wang, Jian J 7 years, 2 months ago
From this perspective, you're right.

> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 1:10 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer
> detection
> 
> If NULL pointer detection is disabled, the code should be behave same with
> before, and ClearLegacyMemory() is not needed to be called because DxeCore
> will behave same with before to handle the first 4K page clear , right?
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, September 28, 2017 11:55 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Wolman, Ayellet <ayellet.wolman@intel.com>
> Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer
> detection
> 
> Clearing this block of memory has nothing to do with NULL pointer detection.
> I'm not sure the extra check is necessary.
> 
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Thursday, September 28, 2017 11:31 AM
> > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney,
> > Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Wolman, Ayellet
> > <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL
> > pointer detection
> >
> > Another comment.
> > Should IsNullDetectionEnabled() be checked before calling
> > ClearLegacyMemory()?
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Thursday, September 28, 2017 11:24 AM
> > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney,
> > Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Wolman, Ayellet
> > <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL
> > pointer detection
> >
> > Minor comments to this patch.
> >
> > 1. IsNullDetectionEnabled() function is put in DxeLoad.c that is
> > shared by all ARCHs, and the function is consuming
> > PcdNullPointerDetectionPropertyMask,
> > but PcdNullPointerDetectionPropertyMask is only declared in
> > "[Pcd.IA32,Pcd.X64]" in inf.
> > I am not sure whether it will cause build failure or not for non-IA32/X64 ARCHs.
> >
> > 2. How about using name "ClearFirst4KPage" instead of "ClearLegacyMemory"
> > to be more clear?
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Thursday, September 28, 2017 9:04 AM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric
> > <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Wolman, Ayellet
> > <ayellet.wolman@intel.com>
> > Subject: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer
> > detection
> >
> > > According to Jiewen's feedback, change the page split condition for
> > > NULL pointer detection to exclude IsExecuteDisableBitAvailable()
> > > (Ia32/DxeLoadFunc.c)
> >
> > NULL pointer detection is done by making use of paging mechanism of CPU.
> > During page table setup, if enabled, the first 4-K page (0-4095) will
> > be marked as NOT PRESENT. Any code which unintentionally access memory
> > between
> > 0-4095 will trigger a Page Fault exception which warns users that
> > there's potential illegal code in BIOS.
> >
> > This also means that legacy code which has to access memory between
> > 0-4095 should be cautious to temporarily disable this feature before
> > the access and re- enable it afterwards; or disalbe this feature at all.
> >
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Michael Kinney <michael.d.kinney@intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Ayellet Wolman <ayellet.wolman@intel.com>
> > Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            | 25 +++++++++
> >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
> >  MdeModulePkg/Core/DxeIplPeim/DxeLoad.c           | 65
> > ++++++++++++++++++++++++
> >  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  | 11 +++-
> >  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
> >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 31 ++++++++---
> >  6 files changed, 126 insertions(+), 9 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> > b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> > index 72d2532f50..1654bcd2dc 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> > @@ -240,4 +240,29 @@ Decompress (
> >    OUT       UINTN                   *OutputSize
> >    );
> >
> > +/**
> > +   Clear legacy memory located at the first 4K-page.
> > +
> > +   This function traverses the whole HOB list to check if memory from 0 to
> 4095
> > +   exists and has not been allocated, and then clear it if so.
> > +
> > +   @param HoStart         The start of HobList passed to DxeCore.
> > +
> > +**/
> > +VOID
> > +ClearLegacyMemory (
> > +  IN  VOID *HobStart
> > +  );
> > +
> > +/**
> > +   Return configure status of NULL pointer detection feature
> > +
> > +   @return TRUE   NULL pointer detection feature is enabled
> > +   @return FALSE  NULL pointer detection feature is disabled **/
> > +BOOLEAN IsNullDetectionEnabled (
> > +  VOID
> > +  );
> > +
> >  #endif
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > index c54afe4aa6..9d0e76a293 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > @@ -115,6 +115,7 @@
> >  [Pcd.IA32,Pcd.X64]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable                      ##
> > SOMETIMES_CONSUMES
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> > ## CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> > ## CONSUMES
> >
> >  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
> > SOMETIMES_CONSUMES
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> > b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> > index 50b5440d15..0a71b1f3de 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> > @@ -825,3 +825,68 @@ UpdateStackHob (
> >      Hob.Raw = GET_NEXT_HOB (Hob);
> >    }
> >  }
> > +
> > +/**
> > +   Clear legacy memory located at the first 4K-page, if available.
> > +
> > +   This function traverses the whole HOB list to check if memory from 0 to
> 4095
> > +   exists and has not been allocated, and then clear it if so.
> > +
> > +   @param HoStart                   The start of HobList passed to DxeCore.
> > +
> > +**/
> > +VOID
> > +ClearLegacyMemory (
> > +  IN  VOID *HobStart
> > +  )
> > +{
> > +  EFI_PEI_HOB_POINTERS          RscHob;
> > +  EFI_PEI_HOB_POINTERS          MemHob;
> > +  BOOLEAN                       DoClear;
> > +
> > +  RscHob.Raw = HobStart;
> > +  MemHob.Raw = HobStart;
> > +  DoClear = FALSE;
> > +
> > +  //
> > +  // Check if page 0 exists and free
> > +  //
> > +  while ((RscHob.Raw = GetNextHob
> (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
> > +                                   RscHob.Raw)) != NULL) {
> > +    if (RscHob.ResourceDescriptor->ResourceType ==
> > EFI_RESOURCE_SYSTEM_MEMORY &&
> > +        RscHob.ResourceDescriptor->PhysicalStart == 0) {
> > +      DoClear = TRUE;
> > +      //
> > +      // Make sure memory at 0-4095 has not been allocated.
> > +      //
> > +      while ((MemHob.Raw = GetNextHob
> > (EFI_HOB_TYPE_MEMORY_ALLOCATION,
> > +                                       MemHob.Raw)) != NULL) {
> > +        if (MemHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress
> > +            < EFI_PAGE_SIZE) {
> > +          DoClear = FALSE;
> > +          break;
> > +        }
> > +        MemHob.Raw = GET_NEXT_HOB (MemHob);
> > +      }
> > +      break;
> > +    }
> > +    RscHob.Raw = GET_NEXT_HOB (RscHob);  }
> > +
> > +  if (DoClear) {
> > +    DEBUG ((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
> > +    SetMem (NULL, EFI_PAGE_SIZE, 0);
> > +  }
> > +
> > +  return;
> > +}
> > +
> > +BOOLEAN
> > +IsNullDetectionEnabled (
> > +  VOID
> > +  )
> > +{
> > +  return (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0) ?
> > +          TRUE : FALSE);
> > +}
> > +
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> > b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> > index 1957326caf..7a8421dd16 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> > +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> > @@ -123,7 +123,9 @@ Create4GPageTablesIa32Pae (
> >      PageDirectoryPointerEntry->Bits.Present = 1;
> >
> >      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries
> > < 512;
> > IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress
> > IndexOfPageDirectoryEntries+++=
> > SIZE_2MB) {
> > -      if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress +
> > SIZE_2MB) > StackBase)) {
> > +      if ((IsNullDetectionEnabled () && PhysicalAddress == 0)
> > +          || ((PhysicalAddress < StackBase + StackSize)
> > +              && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
> >          //
> >          // Need to split this 2M page that covers stack range.
> >          //
> > @@ -240,6 +242,8 @@ HandOffToDxeCore (
> >    EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
> >    BOOLEAN                   BuildPageTablesIa32Pae;
> >
> > +  ClearLegacyMemory (HobList.Raw);
> > +
> >    Status = PeiServicesAllocatePages (EfiBootServicesData,
> > EFI_SIZE_TO_PAGES (STACK_SIZE), &BaseOfStack);
> >    ASSERT_EFI_ERROR (Status);
> >
> > @@ -379,7 +383,10 @@ HandOffToDxeCore (
> >      TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER
> > (TopOfStack, CPU_STACK_ALIGNMENT);
> >
> >      PageTables = 0;
> > -    BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) &&
> > IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
> > +    BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () &&
> > +                                        (IsNullDetectionEnabled () ||
> > +                                         (PcdGetBool (PcdSetNxForStack) &&
> > +
> > + IsExecuteDisableBitAvailable ())));
> >      if (BuildPageTablesIa32Pae) {
> >        PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
> >        EnableExecuteDisableBit ();
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> > b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> > index 6488880eab..d93a9c5a2d 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> > +++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> > @@ -42,6 +42,8 @@ HandOffToDxeCore (
> >    EFI_VECTOR_HANDOFF_INFO         *VectorInfo;
> >    EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
> >
> > +  ClearLegacyMemory (HobList.Raw);
> > +
> >    //
> >    // Get Vector Hand-off Info PPI and build Guided HOB
> >    //
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > index 48150be4e1..80c1821eca 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> > @@ -90,8 +90,16 @@ Split2MPageTo4K (
> >      //
> >      PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
> >      PageTableEntry->Bits.ReadWrite = 1;
> > -    PageTableEntry->Bits.Present = 1;
> > -    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase +
> > StackSize)) {
> > +
> > +    if (IsNullDetectionEnabled () && PhysicalAddress4K == 0) {
> > +      PageTableEntry->Bits.Present = 0;
> > +    } else {
> > +      PageTableEntry->Bits.Present = 1;
> > +    }
> > +
> > +    if (PcdGetBool (PcdSetNxForStack)
> > +        && (PhysicalAddress4K >= StackBase)
> > +        && (PhysicalAddress4K < StackBase + StackSize)) {
> >        //
> >        // Set Nx bit for stack.
> >        //
> > @@ -137,9 +145,12 @@ Split1GPageTo2M (
> >
> >    PhysicalAddress2M = PhysicalAddress;
> >    for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
> > 512;
> > IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M
> > IndexOfPageDirectoryEntries+++=
> > SIZE_2MB) {
> > -    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M +
> > SIZE_2MB) > StackBase)) {
> > +    if ((IsNullDetectionEnabled () && PhysicalAddress2M == 0)
> > +        || (PcdGetBool (PcdSetNxForStack)
> > +            && (PhysicalAddress2M < StackBase + StackSize)
> > +            && ((PhysicalAddress2M + SIZE_2MB) > StackBase))) {
> >        //
> > -      // Need to split this 2M page that covers stack range.
> > +      // Need to split this 2M page that covers NULL or stack range.
> >        //
> >        Split2MPageTo4K (PhysicalAddress2M, (UINT64 *)
> > PageDirectoryEntry, StackBase, StackSize);
> >      } else {
> > @@ -279,7 +290,10 @@ CreateIdentityMappingPageTables (
> >        PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> >
> >        for (IndexOfPageDirectoryEntries = 0;
> > IndexOfPageDirectoryEntries < 512;
> > IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress +=
> > SIZE_1GB) {
> > -        if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase +
> > StackSize) && ((PageAddress + SIZE_1GB) > StackBase)) {
> > +        if ((IsNullDetectionEnabled () && PageAddress == 0)
> > +            || (PcdGetBool (PcdSetNxForStack)
> > +                && (PageAddress < StackBase + StackSize)
> > +                && ((PageAddress + SIZE_1GB) > StackBase))) {
> >            Split1GPageTo2M (PageAddress, (UINT64 *)
> > PageDirectory1GEntry, StackBase, StackSize);
> >          } else {
> >            //
> > @@ -308,9 +322,12 @@ CreateIdentityMappingPageTables (
> >          PageDirectoryPointerEntry->Bits.Present = 1;
> >
> >          for (IndexOfPageDirectoryEntries = 0;
> > IndexOfPageDirectoryEntries < 512;
> > IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
> > SIZE_2MB) {
> > -          if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase +
> > StackSize) && ((PageAddress + SIZE_2MB) > StackBase)) {
> > +          if ((IsNullDetectionEnabled () && PageAddress == 0)
> > +              || (PcdGetBool (PcdSetNxForStack)
> > +                  && (PageAddress < StackBase + StackSize)
> > +                  && ((PageAddress + SIZE_2MB) > StackBase))) {
> >              //
> > -            // Need to split this 2M page that covers stack range.
> > +            // Need to split this 2M page that covers NULL or stack range.
> >              //
> >              Split2MPageTo4K (PageAddress, (UINT64 *)
> > PageDirectoryEntry, StackBase, StackSize);
> >            } else {
> > --
> > 2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
Posted by Wang, Jian J 7 years, 2 months ago
Please see my comments inline below.

> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 11:24 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer
> detection
> 
> Minor comments to this patch.
> 
> 1. IsNullDetectionEnabled() function is put in DxeLoad.c that is shared by all
> ARCHs, and the function is consuming PcdNullPointerDetectionPropertyMask,
> but PcdNullPointerDetectionPropertyMask is only declared in
> "[Pcd.IA32,Pcd.X64]" in inf.
> I am not sure whether it will cause build failure or not for non-IA32/X64 ARCHs.
> 

You're right. The PCD should not be under this section.

> 2. How about using name "ClearFirst4KPage" instead of "ClearLegacyMemory"
> to be more clear?
> 

I prefer a bit more general name in case of potential future change.

> 
> Thanks,
> Star
> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, September 28, 2017 9:04 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>
> Subject: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer
> detection
> 
> > According to Jiewen's feedback, change the page split condition for
> > NULL pointer detection to exclude IsExecuteDisableBitAvailable()
> > (Ia32/DxeLoadFunc.c)
> 
> NULL pointer detection is done by making use of paging mechanism of CPU.
> During page table setup, if enabled, the first 4-K page (0-4095) will be marked as
> NOT PRESENT. Any code which unintentionally access memory between
> 0-4095 will trigger a Page Fault exception which warns users that there's
> potential illegal code in BIOS.
> 
> This also means that legacy code which has to access memory between 0-4095
> should be cautious to temporarily disable this feature before the access and re-
> enable it afterwards; or disalbe this feature at all.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ayellet Wolman <ayellet.wolman@intel.com>
> Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            | 25 +++++++++
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>  MdeModulePkg/Core/DxeIplPeim/DxeLoad.c           | 65
> ++++++++++++++++++++++++
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  | 11 +++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 31 ++++++++---
>  6 files changed, 126 insertions(+), 9 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> index 72d2532f50..1654bcd2dc 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> @@ -240,4 +240,29 @@ Decompress (
>    OUT       UINTN                   *OutputSize
>    );
> 
> +/**
> +   Clear legacy memory located at the first 4K-page.
> +
> +   This function traverses the whole HOB list to check if memory from 0 to 4095
> +   exists and has not been allocated, and then clear it if so.
> +
> +   @param HoStart         The start of HobList passed to DxeCore.
> +
> +**/
> +VOID
> +ClearLegacyMemory (
> +  IN  VOID *HobStart
> +  );
> +
> +/**
> +   Return configure status of NULL pointer detection feature
> +
> +   @return TRUE   NULL pointer detection feature is enabled
> +   @return FALSE  NULL pointer detection feature is disabled **/
> +BOOLEAN IsNullDetectionEnabled (
> +  VOID
> +  );
> +
>  #endif
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index c54afe4aa6..9d0e76a293 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -115,6 +115,7 @@
>  [Pcd.IA32,Pcd.X64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable                      ##
> SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> ## CONSUMES
> 
>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
> SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> index 50b5440d15..0a71b1f3de 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> @@ -825,3 +825,68 @@ UpdateStackHob (
>      Hob.Raw = GET_NEXT_HOB (Hob);
>    }
>  }
> +
> +/**
> +   Clear legacy memory located at the first 4K-page, if available.
> +
> +   This function traverses the whole HOB list to check if memory from 0 to 4095
> +   exists and has not been allocated, and then clear it if so.
> +
> +   @param HoStart                   The start of HobList passed to DxeCore.
> +
> +**/
> +VOID
> +ClearLegacyMemory (
> +  IN  VOID *HobStart
> +  )
> +{
> +  EFI_PEI_HOB_POINTERS          RscHob;
> +  EFI_PEI_HOB_POINTERS          MemHob;
> +  BOOLEAN                       DoClear;
> +
> +  RscHob.Raw = HobStart;
> +  MemHob.Raw = HobStart;
> +  DoClear = FALSE;
> +
> +  //
> +  // Check if page 0 exists and free
> +  //
> +  while ((RscHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
> +                                   RscHob.Raw)) != NULL) {
> +    if (RscHob.ResourceDescriptor->ResourceType ==
> EFI_RESOURCE_SYSTEM_MEMORY &&
> +        RscHob.ResourceDescriptor->PhysicalStart == 0) {
> +      DoClear = TRUE;
> +      //
> +      // Make sure memory at 0-4095 has not been allocated.
> +      //
> +      while ((MemHob.Raw = GetNextHob
> (EFI_HOB_TYPE_MEMORY_ALLOCATION,
> +                                       MemHob.Raw)) != NULL) {
> +        if (MemHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress
> +            < EFI_PAGE_SIZE) {
> +          DoClear = FALSE;
> +          break;
> +        }
> +        MemHob.Raw = GET_NEXT_HOB (MemHob);
> +      }
> +      break;
> +    }
> +    RscHob.Raw = GET_NEXT_HOB (RscHob);  }
> +
> +  if (DoClear) {
> +    DEBUG ((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
> +    SetMem (NULL, EFI_PAGE_SIZE, 0);
> +  }
> +
> +  return;
> +}
> +
> +BOOLEAN
> +IsNullDetectionEnabled (
> +  VOID
> +  )
> +{
> +  return (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0) ?
> +          TRUE : FALSE);
> +}
> +
> diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> index 1957326caf..7a8421dd16 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> @@ -123,7 +123,9 @@ Create4GPageTablesIa32Pae (
>      PageDirectoryPointerEntry->Bits.Present = 1;
> 
>      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress +=
> SIZE_2MB) {
> -      if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress +
> SIZE_2MB) > StackBase)) {
> +      if ((IsNullDetectionEnabled () && PhysicalAddress == 0)
> +          || ((PhysicalAddress < StackBase + StackSize)
> +              && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
>          //
>          // Need to split this 2M page that covers stack range.
>          //
> @@ -240,6 +242,8 @@ HandOffToDxeCore (
>    EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
>    BOOLEAN                   BuildPageTablesIa32Pae;
> 
> +  ClearLegacyMemory (HobList.Raw);
> +
>    Status = PeiServicesAllocatePages (EfiBootServicesData, EFI_SIZE_TO_PAGES
> (STACK_SIZE), &BaseOfStack);
>    ASSERT_EFI_ERROR (Status);
> 
> @@ -379,7 +383,10 @@ HandOffToDxeCore (
>      TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER
> (TopOfStack, CPU_STACK_ALIGNMENT);
> 
>      PageTables = 0;
> -    BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) &&
> IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
> +    BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () &&
> +                                        (IsNullDetectionEnabled () ||
> +                                         (PcdGetBool (PcdSetNxForStack) &&
> +                                          IsExecuteDisableBitAvailable
> + ())));
>      if (BuildPageTablesIa32Pae) {
>        PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
>        EnableExecuteDisableBit ();
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> index 6488880eab..d93a9c5a2d 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> @@ -42,6 +42,8 @@ HandOffToDxeCore (
>    EFI_VECTOR_HANDOFF_INFO         *VectorInfo;
>    EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
> 
> +  ClearLegacyMemory (HobList.Raw);
> +
>    //
>    // Get Vector Hand-off Info PPI and build Guided HOB
>    //
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index 48150be4e1..80c1821eca 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -90,8 +90,16 @@ Split2MPageTo4K (
>      //
>      PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
>      PageTableEntry->Bits.ReadWrite = 1;
> -    PageTableEntry->Bits.Present = 1;
> -    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase +
> StackSize)) {
> +
> +    if (IsNullDetectionEnabled () && PhysicalAddress4K == 0) {
> +      PageTableEntry->Bits.Present = 0;
> +    } else {
> +      PageTableEntry->Bits.Present = 1;
> +    }
> +
> +    if (PcdGetBool (PcdSetNxForStack)
> +        && (PhysicalAddress4K >= StackBase)
> +        && (PhysicalAddress4K < StackBase + StackSize)) {
>        //
>        // Set Nx bit for stack.
>        //
> @@ -137,9 +145,12 @@ Split1GPageTo2M (
> 
>    PhysicalAddress2M = PhysicalAddress;
>    for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M +=
> SIZE_2MB) {
> -    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M +
> SIZE_2MB) > StackBase)) {
> +    if ((IsNullDetectionEnabled () && PhysicalAddress2M == 0)
> +        || (PcdGetBool (PcdSetNxForStack)
> +            && (PhysicalAddress2M < StackBase + StackSize)
> +            && ((PhysicalAddress2M + SIZE_2MB) > StackBase))) {
>        //
> -      // Need to split this 2M page that covers stack range.
> +      // Need to split this 2M page that covers NULL or stack range.
>        //
>        Split2MPageTo4K (PhysicalAddress2M, (UINT64 *) PageDirectoryEntry,
> StackBase, StackSize);
>      } else {
> @@ -279,7 +290,10 @@ CreateIdentityMappingPageTables (
>        PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> 
>        for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress +=
> SIZE_1GB) {
> -        if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase +
> StackSize) && ((PageAddress + SIZE_1GB) > StackBase)) {
> +        if ((IsNullDetectionEnabled () && PageAddress == 0)
> +            || (PcdGetBool (PcdSetNxForStack)
> +                && (PageAddress < StackBase + StackSize)
> +                && ((PageAddress + SIZE_1GB) > StackBase))) {
>            Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry,
> StackBase, StackSize);
>          } else {
>            //
> @@ -308,9 +322,12 @@ CreateIdentityMappingPageTables (
>          PageDirectoryPointerEntry->Bits.Present = 1;
> 
>          for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
> SIZE_2MB) {
> -          if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase +
> StackSize) && ((PageAddress + SIZE_2MB) > StackBase)) {
> +          if ((IsNullDetectionEnabled () && PageAddress == 0)
> +              || (PcdGetBool (PcdSetNxForStack)
> +                  && (PageAddress < StackBase + StackSize)
> +                  && ((PageAddress + SIZE_2MB) > StackBase))) {
>              //
> -            // Need to split this 2M page that covers stack range.
> +            // Need to split this 2M page that covers NULL or stack range.
>              //
>              Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry,
> StackBase, StackSize);
>            } else {
> --
> 2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
Posted by Zeng, Star 7 years, 2 months ago
I am curious about what is potential future change.

Thanks,
Star
-----Original Message-----
From: Wang, Jian J 
Sent: Thursday, September 28, 2017 11:50 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>
Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection

Please see my comments inline below.

> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 11:24 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek 
> <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, 
> Michael D <michael.d.kinney@intel.com>; Justen, Jordan L 
> <jordan.l.justen@intel.com>; Wolman, Ayellet 
> <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL 
> pointer detection
> 
> Minor comments to this patch.
> 
> 1. IsNullDetectionEnabled() function is put in DxeLoad.c that is 
> shared by all ARCHs, and the function is consuming 
> PcdNullPointerDetectionPropertyMask,
> but PcdNullPointerDetectionPropertyMask is only declared in 
> "[Pcd.IA32,Pcd.X64]" in inf.
> I am not sure whether it will cause build failure or not for non-IA32/X64 ARCHs.
> 

You're right. The PCD should not be under this section.

> 2. How about using name "ClearFirst4KPage" instead of "ClearLegacyMemory"
> to be more clear?
> 

I prefer a bit more general name in case of potential future change.

> 
> Thanks,
> Star
> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, September 28, 2017 9:04 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric 
> <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen 
> <jiewen.yao@intel.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>; Justen, Jordan L 
> <jordan.l.justen@intel.com>; Wolman, Ayellet 
> <ayellet.wolman@intel.com>
> Subject: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer 
> detection
> 
> > According to Jiewen's feedback, change the page split condition for 
> > NULL pointer detection to exclude IsExecuteDisableBitAvailable()
> > (Ia32/DxeLoadFunc.c)
> 
> NULL pointer detection is done by making use of paging mechanism of CPU.
> During page table setup, if enabled, the first 4-K page (0-4095) will 
> be marked as NOT PRESENT. Any code which unintentionally access memory 
> between
> 0-4095 will trigger a Page Fault exception which warns users that 
> there's potential illegal code in BIOS.
> 
> This also means that legacy code which has to access memory between 
> 0-4095 should be cautious to temporarily disable this feature before 
> the access and re- enable it afterwards; or disalbe this feature at all.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ayellet Wolman <ayellet.wolman@intel.com>
> Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            | 25 +++++++++
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>  MdeModulePkg/Core/DxeIplPeim/DxeLoad.c           | 65
> ++++++++++++++++++++++++
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  | 11 +++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 31 ++++++++---
>  6 files changed, 126 insertions(+), 9 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> index 72d2532f50..1654bcd2dc 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> @@ -240,4 +240,29 @@ Decompress (
>    OUT       UINTN                   *OutputSize
>    );
> 
> +/**
> +   Clear legacy memory located at the first 4K-page.
> +
> +   This function traverses the whole HOB list to check if memory from 0 to 4095
> +   exists and has not been allocated, and then clear it if so.
> +
> +   @param HoStart         The start of HobList passed to DxeCore.
> +
> +**/
> +VOID
> +ClearLegacyMemory (
> +  IN  VOID *HobStart
> +  );
> +
> +/**
> +   Return configure status of NULL pointer detection feature
> +
> +   @return TRUE   NULL pointer detection feature is enabled
> +   @return FALSE  NULL pointer detection feature is disabled **/ 
> +BOOLEAN IsNullDetectionEnabled (
> +  VOID
> +  );
> +
>  #endif
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index c54afe4aa6..9d0e76a293 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -115,6 +115,7 @@
>  [Pcd.IA32,Pcd.X64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable                      ##
> SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> ## CONSUMES
> 
>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
> SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> index 50b5440d15..0a71b1f3de 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> @@ -825,3 +825,68 @@ UpdateStackHob (
>      Hob.Raw = GET_NEXT_HOB (Hob);
>    }
>  }
> +
> +/**
> +   Clear legacy memory located at the first 4K-page, if available.
> +
> +   This function traverses the whole HOB list to check if memory from 0 to 4095
> +   exists and has not been allocated, and then clear it if so.
> +
> +   @param HoStart                   The start of HobList passed to DxeCore.
> +
> +**/
> +VOID
> +ClearLegacyMemory (
> +  IN  VOID *HobStart
> +  )
> +{
> +  EFI_PEI_HOB_POINTERS          RscHob;
> +  EFI_PEI_HOB_POINTERS          MemHob;
> +  BOOLEAN                       DoClear;
> +
> +  RscHob.Raw = HobStart;
> +  MemHob.Raw = HobStart;
> +  DoClear = FALSE;
> +
> +  //
> +  // Check if page 0 exists and free
> +  //
> +  while ((RscHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
> +                                   RscHob.Raw)) != NULL) {
> +    if (RscHob.ResourceDescriptor->ResourceType ==
> EFI_RESOURCE_SYSTEM_MEMORY &&
> +        RscHob.ResourceDescriptor->PhysicalStart == 0) {
> +      DoClear = TRUE;
> +      //
> +      // Make sure memory at 0-4095 has not been allocated.
> +      //
> +      while ((MemHob.Raw = GetNextHob
> (EFI_HOB_TYPE_MEMORY_ALLOCATION,
> +                                       MemHob.Raw)) != NULL) {
> +        if (MemHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress
> +            < EFI_PAGE_SIZE) {
> +          DoClear = FALSE;
> +          break;
> +        }
> +        MemHob.Raw = GET_NEXT_HOB (MemHob);
> +      }
> +      break;
> +    }
> +    RscHob.Raw = GET_NEXT_HOB (RscHob);  }
> +
> +  if (DoClear) {
> +    DEBUG ((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
> +    SetMem (NULL, EFI_PAGE_SIZE, 0);
> +  }
> +
> +  return;
> +}
> +
> +BOOLEAN
> +IsNullDetectionEnabled (
> +  VOID
> +  )
> +{
> +  return (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0) ?
> +          TRUE : FALSE);
> +}
> +
> diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> index 1957326caf..7a8421dd16 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> @@ -123,7 +123,9 @@ Create4GPageTablesIa32Pae (
>      PageDirectoryPointerEntry->Bits.Present = 1;
> 
>      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries 
> < 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress 
> IndexOfPageDirectoryEntries+++=
> SIZE_2MB) {
> -      if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress +
> SIZE_2MB) > StackBase)) {
> +      if ((IsNullDetectionEnabled () && PhysicalAddress == 0)
> +          || ((PhysicalAddress < StackBase + StackSize)
> +              && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
>          //
>          // Need to split this 2M page that covers stack range.
>          //
> @@ -240,6 +242,8 @@ HandOffToDxeCore (
>    EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
>    BOOLEAN                   BuildPageTablesIa32Pae;
> 
> +  ClearLegacyMemory (HobList.Raw);
> +
>    Status = PeiServicesAllocatePages (EfiBootServicesData, 
> EFI_SIZE_TO_PAGES (STACK_SIZE), &BaseOfStack);
>    ASSERT_EFI_ERROR (Status);
> 
> @@ -379,7 +383,10 @@ HandOffToDxeCore (
>      TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER 
> (TopOfStack, CPU_STACK_ALIGNMENT);
> 
>      PageTables = 0;
> -    BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) &&
> IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
> +    BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () &&
> +                                        (IsNullDetectionEnabled () ||
> +                                         (PcdGetBool (PcdSetNxForStack) &&
> +                                          
> + IsExecuteDisableBitAvailable ())));
>      if (BuildPageTablesIa32Pae) {
>        PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
>        EnableExecuteDisableBit ();
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> index 6488880eab..d93a9c5a2d 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> @@ -42,6 +42,8 @@ HandOffToDxeCore (
>    EFI_VECTOR_HANDOFF_INFO         *VectorInfo;
>    EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
> 
> +  ClearLegacyMemory (HobList.Raw);
> +
>    //
>    // Get Vector Hand-off Info PPI and build Guided HOB
>    //
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index 48150be4e1..80c1821eca 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -90,8 +90,16 @@ Split2MPageTo4K (
>      //
>      PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
>      PageTableEntry->Bits.ReadWrite = 1;
> -    PageTableEntry->Bits.Present = 1;
> -    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase +
> StackSize)) {
> +
> +    if (IsNullDetectionEnabled () && PhysicalAddress4K == 0) {
> +      PageTableEntry->Bits.Present = 0;
> +    } else {
> +      PageTableEntry->Bits.Present = 1;
> +    }
> +
> +    if (PcdGetBool (PcdSetNxForStack)
> +        && (PhysicalAddress4K >= StackBase)
> +        && (PhysicalAddress4K < StackBase + StackSize)) {
>        //
>        // Set Nx bit for stack.
>        //
> @@ -137,9 +145,12 @@ Split1GPageTo2M (
> 
>    PhysicalAddress2M = PhysicalAddress;
>    for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 
> 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M 
> IndexOfPageDirectoryEntries+++=
> SIZE_2MB) {
> -    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M +
> SIZE_2MB) > StackBase)) {
> +    if ((IsNullDetectionEnabled () && PhysicalAddress2M == 0)
> +        || (PcdGetBool (PcdSetNxForStack)
> +            && (PhysicalAddress2M < StackBase + StackSize)
> +            && ((PhysicalAddress2M + SIZE_2MB) > StackBase))) {
>        //
> -      // Need to split this 2M page that covers stack range.
> +      // Need to split this 2M page that covers NULL or stack range.
>        //
>        Split2MPageTo4K (PhysicalAddress2M, (UINT64 *) 
> PageDirectoryEntry, StackBase, StackSize);
>      } else {
> @@ -279,7 +290,10 @@ CreateIdentityMappingPageTables (
>        PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> 
>        for (IndexOfPageDirectoryEntries = 0; 
> IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress +=
> SIZE_1GB) {
> -        if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase +
> StackSize) && ((PageAddress + SIZE_1GB) > StackBase)) {
> +        if ((IsNullDetectionEnabled () && PageAddress == 0)
> +            || (PcdGetBool (PcdSetNxForStack)
> +                && (PageAddress < StackBase + StackSize)
> +                && ((PageAddress + SIZE_1GB) > StackBase))) {
>            Split1GPageTo2M (PageAddress, (UINT64 *) 
> PageDirectory1GEntry, StackBase, StackSize);
>          } else {
>            //
> @@ -308,9 +322,12 @@ CreateIdentityMappingPageTables (
>          PageDirectoryPointerEntry->Bits.Present = 1;
> 
>          for (IndexOfPageDirectoryEntries = 0; 
> IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
> SIZE_2MB) {
> -          if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase +
> StackSize) && ((PageAddress + SIZE_2MB) > StackBase)) {
> +          if ((IsNullDetectionEnabled () && PageAddress == 0)
> +              || (PcdGetBool (PcdSetNxForStack)
> +                  && (PageAddress < StackBase + StackSize)
> +                  && ((PageAddress + SIZE_2MB) > StackBase))) {
>              //
> -            // Need to split this 2M page that covers stack range.
> +            // Need to split this 2M page that covers NULL or stack range.
>              //
>              Split2MPageTo4K (PageAddress, (UINT64 *) 
> PageDirectoryEntry, StackBase, StackSize);
>            } else {
> --
> 2.14.1.windows.1

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