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

["Zeng, Star" <star.zeng@intel.com>]

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