[v1] Implement NULL pointer detection feature for special pool

[edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

Posted by Wang, Jian J 7 years, 3 months ago

The mechanism behind is to trigger a page fault exception at address 0. This can be made by disabling page 0 (0-4095) during page table setup. So this feature can only be available on platform with paging enabled. Once this feature is enabled, any code, like CSM, which has to access memory in page 0 needs to enable this page temporarily in advance and disable it afterwards. PcdNullPointerDetectionPropertyMask is used to control and elaborate the use cases.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Justen, Jordan L <jordan.l.justen@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Wolman, Ayellet <ayellet.wolman@intel.com>
Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 +-
 MdeModulePkg/Core/Dxe/Mem/Page.c                 | 21 ++++++----
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c    | 47 +++++++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            | 15 +++++++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  3 +-
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c           | 53 ++++++++++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  8 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 23 ++++++----
 MdeModulePkg/MdeModulePkg.dec                    | 12 ++++++
 10 files changed, 167 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 30d5984f7c..273b8b7c0e 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -179,7 +179,7 @@
   gEfiWatchdogTimerArchProtocolGuid             ## CONSUMES
 
 [FeaturePcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport	   ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport          ## CONSUMES
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber    ## SOMETIMES_CONSUMES
@@ -192,6 +192,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy             ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask        ## CONSUMES
 
 # [Hob]
 # RESOURCE_DESCRIPTOR   ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index a142c79ee2..2e0b72f864 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -170,6 +170,7 @@ CoreAddRange (
 {
   LIST_ENTRY        *Link;
   MEMORY_MAP        *Entry;
+  EFI_STATUS        Status;
 
   ASSERT ((Start & EFI_PAGE_MASK) == 0);
   ASSERT (End > Start) ;
@@ -188,7 +189,17 @@ CoreAddRange (
   // used for other purposes.
   //  
   if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 1)) {
-    SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+    if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+      SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+    } else if (gCpu != NULL) {
+      Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
+      ASSERT_EFI_ERROR(Status);
+
+      SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+
+      Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, EFI_MEMORY_RP);
+      ASSERT_EFI_ERROR(Status);
+    }
   }
   
   //
@@ -1972,11 +1983,3 @@ Done:
   return Status;
 }
 
-
-
-
-
-
-
-
-
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index a73c4ccd64..2367d674e1 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -995,6 +995,36 @@ MemoryProtectionExitBootServicesCallback (
   }
 }
 
+/**
+  Disable NULL pointer detection after EndOfDxe. This is a workaround resort in 
+  order to skip unfixable NULL pointer access issues detected in OptionROM or 
+  boot loaders.
+
+  @param[in]  Event     The Event this notify function registered to.
+  @param[in]  Context   Pointer to the context data registered to the Event.
+**/
+VOID
+EFIAPI
+DisableNullDetectionAtTheEndOfDxe (
+  EFI_EVENT                               Event,
+  VOID                                    *Context
+  )
+{
+  EFI_STATUS                        Status;
+
+  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): start\r\n"));
+  //
+  // Disable NULL pointer detection by enabling first 4K page
+  //
+  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
+  ASSERT_EFI_ERROR(Status);
+
+  CoreCloseEvent (Event);
+  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): end\r\n"));
+
+  return;
+}
+
 /**
   Initialize Memory Protection support.
 **/
@@ -1006,6 +1036,7 @@ CoreInitializeMemoryProtection (
 {
   EFI_STATUS  Status;
   EFI_EVENT   Event;
+  EFI_EVENT   EndOfDxeEvent;
   VOID        *Registration;
 
   mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
@@ -1044,6 +1075,22 @@ CoreInitializeMemoryProtection (
                );
     ASSERT_EFI_ERROR(Status);
   }
+
+  //
+  // Register a callback to disable NULL pointer detection at EndOfDxe
+  //
+  if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == (BIT0|BIT7)) {
+    Status = CoreCreateEventEx (
+                    EVT_NOTIFY_SIGNAL,
+                    TPL_NOTIFY,
+                    DisableNullDetectionAtTheEndOfDxe,
+                    NULL,
+                    &gEfiEndOfDxeEventGroupGuid,
+                    &EndOfDxeEvent
+                    );
+    ASSERT_EFI_ERROR (Status);
+  }
+
   return ;
 }
 
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
index 72d2532f50..104599156c 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
@@ -52,6 +52,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #define STACK_SIZE      0x20000
 #define BSP_STORE_SIZE  0x4000
 
+#define NULL_DETECTION_ENABLED  ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) != 0)
 
 //
 // This PPI is installed to indicate the end of the PEI usage of memory
@@ -240,4 +241,18 @@ 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
+  );
+
 #endif
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index c54afe4aa6..fde70f94bb 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -110,11 +110,12 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplBuildPageTables       ## CONSUMES
 
 [FeaturePcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress         ## CONSUMES
 
 [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..b5f9d92f5b 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
@@ -825,3 +825,56 @@ 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          RscDescHob;
+  EFI_PEI_HOB_POINTERS          MemAllocHob;
+  BOOLEAN                       DoClear;
+
+  RscDescHob.Raw = HobStart;
+  MemAllocHob.Raw = HobStart;
+  DoClear = FALSE;
+
+  //
+  // Check if page 0 exists and free
+  //
+  while ((RscDescHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, RscDescHob.Raw)) != NULL) {
+    if (RscDescHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY && 
+        RscDescHob.ResourceDescriptor->PhysicalStart == 0) {
+      DoClear = TRUE;
+      // 
+      // Make sure memory at 0-4095 has not been allocated.
+      //
+      while ((MemAllocHob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, MemAllocHob.Raw)) != NULL) {
+        if (MemAllocHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress < EFI_PAGE_SIZE) {
+          DoClear = FALSE;
+          break;
+        }
+        MemAllocHob.Raw = GET_NEXT_HOB (MemAllocHob);
+      }
+      break;
+    }
+    RscDescHob.Raw = GET_NEXT_HOB (RscDescHob);
+  }
+
+  if (DoClear) {
+    DEBUG((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
+    SetMem(NULL, EFI_PAGE_SIZE, 0);
+  }
+
+  return;
+}
+
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index 1957326caf..a8aa0d5d1b 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -123,7 +123,8 @@ Create4GPageTablesIa32Pae (
     PageDirectoryPointerEntry->Bits.Present = 1;
 
     for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress += SIZE_2MB) {
-      if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase)) {
+      if ((NULL_DETECTION_ENABLED && PhysicalAddress == 0)
+          || ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
         //
         // Need to split this 2M page that covers stack range.
         //
@@ -240,6 +241,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 +382,8 @@ HandOffToDxeCore (
     TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
 
     PageTables = 0;
-    BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) && IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
+    BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () && IsExecuteDisableBitAvailable ()
+                                        && (PcdGetBool (PcdSetNxForStack) || NULL_DETECTION_ENABLED));
     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..50a8d77a5b 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..ccd6e10cb2 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -90,8 +90,14 @@ Split2MPageTo4K (
     //
     PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
     PageTableEntry->Bits.ReadWrite = 1;
-    PageTableEntry->Bits.Present = 1;
-    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + StackSize)) {
+
+    if (NULL_DETECTION_ENABLED && 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 +143,10 @@ Split1GPageTo2M (
 
   PhysicalAddress2M = PhysicalAddress;
   for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += SIZE_2MB) {
-    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M + SIZE_2MB) > StackBase)) {
+    if ((NULL_DETECTION_ENABLED && 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 +286,8 @@ 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 ((NULL_DETECTION_ENABLED && PageAddress == 0)
+            || (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + StackSize) && ((PageAddress + SIZE_1GB) > StackBase))) {
           Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
         } else {
           //
@@ -308,9 +316,10 @@ 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 ((NULL_DETECTION_ENABLED && 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 {
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 593bff357a..1cc84894af 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -860,6 +860,18 @@
   # @ValidList  0x80000006 | 0x03058002
   gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable|0x03058002|UINT32|0x30001040
 
+  ## Mask to control the NULL address detection in code for different phases.
+  #  If enabled, accessing NULL address in UEFI or SMM code can be caught.<BR><BR>
+  #    BIT0    - Enable NULL pointer detection for UEFI.<BR>
+  #    BIT1    - Enable NULL pointer detection for SMM.<BR>
+  #    BIT2..6 - Reserved for future uses.<BR>
+  #    BIT7    - Disable NULL pointer detection just after EndOfDxe. <BR>
+  #              This is a workaround for those unsolvable NULL access issues in OptionROM, boot loader, etc.
+  #              It can also help to avoid unnecessary exception caused by legacy memory (0-4095) access after 
+  #              EndOfDxe, such as Windows 7 boot on Qemu.<BR>
+  # @Prompt Enable NULL address detection.
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask|0x0|UINT8|0x30001050
+
 [PcdsFixedAtBuild, PcdsPatchableInModule]
   ## Dynamic type PCD can be registered callback function for Pcd setting action.
   #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function
-- 
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 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

Posted by Johnson, Brian (EXL - Eagan) 7 years, 3 months ago

ClearLegacyMemory() assumes that the memory allocation HOB comes after the resource descriptor HOB in the HOB list.  Is that guaranteed?  I'd think that the memory allocation HOB traversal should be a separate loop, after the resource descriptor HOB traversal loop.

Other than that:
Reviewed-by: Brian J. Johnson <brian.johnson@hpe.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Jian J
Sent: Wednesday, September 13, 2017 4:25 AM
To: edk2-devel@lists.01.org
Cc: Justen@ml01.01.org; Eric Dong <eric.dong@intel.com>; Kinney@ml01.01.org; Jordan L <jordan.l.justen@intel.com>; Wolman@ml01.01.org; Jiewen Yao <jiewen.yao@intel.com>; Ayellet <ayellet.wolman@intel.com>; Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>; Star Zeng <star.zeng@intel.com>
Subject: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

The mechanism behind is to trigger a page fault exception at address 0. This can be made by disabling page 0 (0-4095) during page table setup. So this feature can only be available on platform with paging enabled. Once this feature is enabled, any code, like CSM, which has to access memory in page 0 needs to enable this page temporarily in advance and disable it afterwards. PcdNullPointerDetectionPropertyMask is used to control and elaborate the use cases.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Justen, Jordan L <jordan.l.justen@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Wolman, Ayellet <ayellet.wolman@intel.com>
Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 +-
 MdeModulePkg/Core/Dxe/Mem/Page.c                 | 21 ++++++----
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c    | 47 +++++++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            | 15 +++++++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  3 +-
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c           | 53 ++++++++++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  8 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 23 ++++++----
 MdeModulePkg/MdeModulePkg.dec                    | 12 ++++++
 10 files changed, 167 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 30d5984f7c..273b8b7c0e 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -179,7 +179,7 @@
   gEfiWatchdogTimerArchProtocolGuid             ## CONSUMES
 
 [FeaturePcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport	   ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport          ## CONSUMES
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber    ## SOMETIMES_CONSUMES
@@ -192,6 +192,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy             ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask        ## CONSUMES
 
 # [Hob]
 # RESOURCE_DESCRIPTOR   ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index a142c79ee2..2e0b72f864 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -170,6 +170,7 @@ CoreAddRange (
 {
   LIST_ENTRY        *Link;
   MEMORY_MAP        *Entry;
+  EFI_STATUS        Status;
 
   ASSERT ((Start & EFI_PAGE_MASK) == 0);
   ASSERT (End > Start) ;
@@ -188,7 +189,17 @@ CoreAddRange (
   // used for other purposes.
   //  
   if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 1)) {
-    SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+    if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+      SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+    } else if (gCpu != NULL) {
+      Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
+      ASSERT_EFI_ERROR(Status);
+
+      SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+
+      Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, EFI_MEMORY_RP);
+      ASSERT_EFI_ERROR(Status);
+    }
   }
   
   //
@@ -1972,11 +1983,3 @@ Done:
   return Status;
 }
 
-
-
-
-
-
-
-
-
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index a73c4ccd64..2367d674e1 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -995,6 +995,36 @@ MemoryProtectionExitBootServicesCallback (
   }
 }
 
+/**
+  Disable NULL pointer detection after EndOfDxe. This is a workaround resort in 
+  order to skip unfixable NULL pointer access issues detected in OptionROM or 
+  boot loaders.
+
+  @param[in]  Event     The Event this notify function registered to.
+  @param[in]  Context   Pointer to the context data registered to the Event.
+**/
+VOID
+EFIAPI
+DisableNullDetectionAtTheEndOfDxe (
+  EFI_EVENT                               Event,
+  VOID                                    *Context
+  )
+{
+  EFI_STATUS                        Status;
+
+  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): start\r\n"));
+  //
+  // Disable NULL pointer detection by enabling first 4K page
+  //
+  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
+  ASSERT_EFI_ERROR(Status);
+
+  CoreCloseEvent (Event);
+  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): end\r\n"));
+
+  return;
+}
+
 /**
   Initialize Memory Protection support.
 **/
@@ -1006,6 +1036,7 @@ CoreInitializeMemoryProtection (
 {
   EFI_STATUS  Status;
   EFI_EVENT   Event;
+  EFI_EVENT   EndOfDxeEvent;
   VOID        *Registration;
 
   mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
@@ -1044,6 +1075,22 @@ CoreInitializeMemoryProtection (
                );
     ASSERT_EFI_ERROR(Status);
   }
+
+  //
+  // Register a callback to disable NULL pointer detection at EndOfDxe
+  //
+  if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == (BIT0|BIT7)) {
+    Status = CoreCreateEventEx (
+                    EVT_NOTIFY_SIGNAL,
+                    TPL_NOTIFY,
+                    DisableNullDetectionAtTheEndOfDxe,
+                    NULL,
+                    &gEfiEndOfDxeEventGroupGuid,
+                    &EndOfDxeEvent
+                    );
+    ASSERT_EFI_ERROR (Status);
+  }
+
   return ;
 }
 
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
index 72d2532f50..104599156c 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
@@ -52,6 +52,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #define STACK_SIZE      0x20000
 #define BSP_STORE_SIZE  0x4000
 
+#define NULL_DETECTION_ENABLED  ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) != 0)
 
 //
 // This PPI is installed to indicate the end of the PEI usage of memory
@@ -240,4 +241,18 @@ 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
+  );
+
 #endif
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index c54afe4aa6..fde70f94bb 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -110,11 +110,12 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplBuildPageTables       ## CONSUMES
 
 [FeaturePcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress         ## CONSUMES
 
 [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..b5f9d92f5b 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
@@ -825,3 +825,56 @@ 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          RscDescHob;
+  EFI_PEI_HOB_POINTERS          MemAllocHob;
+  BOOLEAN                       DoClear;
+
+  RscDescHob.Raw = HobStart;
+  MemAllocHob.Raw = HobStart;
+  DoClear = FALSE;
+
+  //
+  // Check if page 0 exists and free
+  //
+  while ((RscDescHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, RscDescHob.Raw)) != NULL) {
+    if (RscDescHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY && 
+        RscDescHob.ResourceDescriptor->PhysicalStart == 0) {
+      DoClear = TRUE;
+      // 
+      // Make sure memory at 0-4095 has not been allocated.
+      //
+      while ((MemAllocHob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, MemAllocHob.Raw)) != NULL) {
+        if (MemAllocHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress < EFI_PAGE_SIZE) {
+          DoClear = FALSE;
+          break;
+        }
+        MemAllocHob.Raw = GET_NEXT_HOB (MemAllocHob);
+      }
+      break;
+    }
+    RscDescHob.Raw = GET_NEXT_HOB (RscDescHob);
+  }
+
+  if (DoClear) {
+    DEBUG((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
+    SetMem(NULL, EFI_PAGE_SIZE, 0);
+  }
+
+  return;
+}
+
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index 1957326caf..a8aa0d5d1b 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -123,7 +123,8 @@ Create4GPageTablesIa32Pae (
     PageDirectoryPointerEntry->Bits.Present = 1;
 
     for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress += SIZE_2MB) {
-      if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase)) {
+      if ((NULL_DETECTION_ENABLED && PhysicalAddress == 0)
+          || ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
         //
         // Need to split this 2M page that covers stack range.
         //
@@ -240,6 +241,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 +382,8 @@ HandOffToDxeCore (
     TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
 
     PageTables = 0;
-    BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) && IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
+    BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () && IsExecuteDisableBitAvailable ()
+                                        && (PcdGetBool (PcdSetNxForStack) || NULL_DETECTION_ENABLED));
     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..50a8d77a5b 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..ccd6e10cb2 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -90,8 +90,14 @@ Split2MPageTo4K (
     //
     PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
     PageTableEntry->Bits.ReadWrite = 1;
-    PageTableEntry->Bits.Present = 1;
-    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + StackSize)) {
+
+    if (NULL_DETECTION_ENABLED && 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 +143,10 @@ Split1GPageTo2M (
 
   PhysicalAddress2M = PhysicalAddress;
   for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += SIZE_2MB) {
-    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M + SIZE_2MB) > StackBase)) {
+    if ((NULL_DETECTION_ENABLED && 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 +286,8 @@ 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 ((NULL_DETECTION_ENABLED && PageAddress == 0)
+            || (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + StackSize) && ((PageAddress + SIZE_1GB) > StackBase))) {
           Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
         } else {
           //
@@ -308,9 +316,10 @@ 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 ((NULL_DETECTION_ENABLED && 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 {
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 593bff357a..1cc84894af 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -860,6 +860,18 @@
   # @ValidList  0x80000006 | 0x03058002
   gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable|0x03058002|UINT32|0x30001040
 
+  ## Mask to control the NULL address detection in code for different phases.
+  #  If enabled, accessing NULL address in UEFI or SMM code can be caught.<BR><BR>
+  #    BIT0    - Enable NULL pointer detection for UEFI.<BR>
+  #    BIT1    - Enable NULL pointer detection for SMM.<BR>
+  #    BIT2..6 - Reserved for future uses.<BR>
+  #    BIT7    - Disable NULL pointer detection just after EndOfDxe. <BR>
+  #              This is a workaround for those unsolvable NULL access issues in OptionROM, boot loader, etc.
+  #              It can also help to avoid unnecessary exception caused by legacy memory (0-4095) access after 
+  #              EndOfDxe, such as Windows 7 boot on Qemu.<BR>
+  # @Prompt Enable NULL address detection.
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask|0x0|UINT8|0x30001050
+
 [PcdsFixedAtBuild, PcdsPatchableInModule]
   ## Dynamic type PCD can be registered callback function for Pcd setting action.
   #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function
-- 
2.14.1.windows.1

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

Re: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

Posted by Wang, Jian J 7 years, 3 months ago

Thanks for the comment. I think there's no problem here because I'm using two separate HOB pointers (RscDescHob and MemAllocHob) to do the traversal. One won't interfere with another. 

-----Original Message-----
From: Johnson, Brian (EXL - Eagan) [mailto:brian.johnson@hpe.com] 
Sent: Thursday, September 14, 2017 12:33 AM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: Justen@ml01.01.org; Dong, Eric <eric.dong@intel.com>; Kinney@ml01.01.org; Justen, Jordan L <jordan.l.justen@intel.com>; Wolman@ml01.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

ClearLegacyMemory() assumes that the memory allocation HOB comes after the resource descriptor HOB in the HOB list.  Is that guaranteed?  I'd think that the memory allocation HOB traversal should be a separate loop, after the resource descriptor HOB traversal loop.

Other than that:
Reviewed-by: Brian J. Johnson <brian.johnson@hpe.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Jian J
Sent: Wednesday, September 13, 2017 4:25 AM
To: edk2-devel@lists.01.org
Cc: Justen@ml01.01.org; Eric Dong <eric.dong@intel.com>; Kinney@ml01.01.org; Jordan L <jordan.l.justen@intel.com>; Wolman@ml01.01.org; Jiewen Yao <jiewen.yao@intel.com>; Ayellet <ayellet.wolman@intel.com>; Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>; Star Zeng <star.zeng@intel.com>
Subject: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

The mechanism behind is to trigger a page fault exception at address 0. This can be made by disabling page 0 (0-4095) during page table setup. So this feature can only be available on platform with paging enabled. Once this feature is enabled, any code, like CSM, which has to access memory in page 0 needs to enable this page temporarily in advance and disable it afterwards. PcdNullPointerDetectionPropertyMask is used to control and elaborate the use cases.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Justen, Jordan L <jordan.l.justen@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Wolman, Ayellet <ayellet.wolman@intel.com>
Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 +-
 MdeModulePkg/Core/Dxe/Mem/Page.c                 | 21 ++++++----
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c    | 47 +++++++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            | 15 +++++++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  3 +-
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c           | 53 ++++++++++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  8 +++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 23 ++++++----
 MdeModulePkg/MdeModulePkg.dec                    | 12 ++++++
 10 files changed, 167 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 30d5984f7c..273b8b7c0e 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -179,7 +179,7 @@
   gEfiWatchdogTimerArchProtocolGuid             ## CONSUMES
 
 [FeaturePcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport   ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport          ## CONSUMES
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber    ## SOMETIMES_CONSUMES
@@ -192,6 +192,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy             ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask        ## CONSUMES
 
 # [Hob]
 # RESOURCE_DESCRIPTOR   ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index a142c79ee2..2e0b72f864 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -170,6 +170,7 @@ CoreAddRange (
 {
   LIST_ENTRY        *Link;
   MEMORY_MAP        *Entry;
+  EFI_STATUS        Status;
 
   ASSERT ((Start & EFI_PAGE_MASK) == 0);
   ASSERT (End > Start) ;
@@ -188,7 +189,17 @@ CoreAddRange (
   // used for other purposes.
   //  
   if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 1)) {
-    SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+    if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+      SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+    } else if (gCpu != NULL) {
+      Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
+      ASSERT_EFI_ERROR(Status);
+
+      SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+
+      Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, EFI_MEMORY_RP);
+      ASSERT_EFI_ERROR(Status);
+    }
   }
   
   //
@@ -1972,11 +1983,3 @@ Done:
   return Status;
 }
 
-
-
-
-
-
-
-
-
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index a73c4ccd64..2367d674e1 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -995,6 +995,36 @@ MemoryProtectionExitBootServicesCallback (
   }
 }
 
+/**
+  Disable NULL pointer detection after EndOfDxe. This is a workaround resort in 
+  order to skip unfixable NULL pointer access issues detected in OptionROM or 
+  boot loaders.
+
+  @param[in]  Event     The Event this notify function registered to.
+  @param[in]  Context   Pointer to the context data registered to the Event.
+**/
+VOID
+EFIAPI
+DisableNullDetectionAtTheEndOfDxe (
+  EFI_EVENT                               Event,
+  VOID                                    *Context
+  )
+{
+  EFI_STATUS                        Status;
+
+  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): start\r\n"));
+  //
+  // Disable NULL pointer detection by enabling first 4K page
+  //
+  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
+  ASSERT_EFI_ERROR(Status);
+
+  CoreCloseEvent (Event);
+  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): end\r\n"));
+
+  return;
+}
+
 /**
   Initialize Memory Protection support.
 **/
@@ -1006,6 +1036,7 @@ CoreInitializeMemoryProtection (
 {
   EFI_STATUS  Status;
   EFI_EVENT   Event;
+  EFI_EVENT   EndOfDxeEvent;
   VOID        *Registration;
 
   mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
@@ -1044,6 +1075,22 @@ CoreInitializeMemoryProtection (
                );
     ASSERT_EFI_ERROR(Status);
   }
+
+  //
+  // Register a callback to disable NULL pointer detection at EndOfDxe
+  //
+  if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == (BIT0|BIT7)) {
+    Status = CoreCreateEventEx (
+                    EVT_NOTIFY_SIGNAL,
+                    TPL_NOTIFY,
+                    DisableNullDetectionAtTheEndOfDxe,
+                    NULL,
+                    &gEfiEndOfDxeEventGroupGuid,
+                    &EndOfDxeEvent
+                    );
+    ASSERT_EFI_ERROR (Status);
+  }
+
   return ;
 }
 
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
index 72d2532f50..104599156c 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
@@ -52,6 +52,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #define STACK_SIZE      0x20000
 #define BSP_STORE_SIZE  0x4000
 
+#define NULL_DETECTION_ENABLED  ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) != 0)
 
 //
 // This PPI is installed to indicate the end of the PEI usage of memory
@@ -240,4 +241,18 @@ 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
+  );
+
 #endif
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index c54afe4aa6..fde70f94bb 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -110,11 +110,12 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplBuildPageTables       ## CONSUMES
 
 [FeaturePcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress         ## CONSUMES
 
 [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..b5f9d92f5b 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
@@ -825,3 +825,56 @@ 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          RscDescHob;
+  EFI_PEI_HOB_POINTERS          MemAllocHob;
+  BOOLEAN                       DoClear;
+
+  RscDescHob.Raw = HobStart;
+  MemAllocHob.Raw = HobStart;
+  DoClear = FALSE;
+
+  //
+  // Check if page 0 exists and free
+  //
+  while ((RscDescHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, RscDescHob.Raw)) != NULL) {
+    if (RscDescHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY && 
+        RscDescHob.ResourceDescriptor->PhysicalStart == 0) {
+      DoClear = TRUE;
+      // 
+      // Make sure memory at 0-4095 has not been allocated.
+      //
+      while ((MemAllocHob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, MemAllocHob.Raw)) != NULL) {
+        if (MemAllocHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress < EFI_PAGE_SIZE) {
+          DoClear = FALSE;
+          break;
+        }
+        MemAllocHob.Raw = GET_NEXT_HOB (MemAllocHob);
+      }
+      break;
+    }
+    RscDescHob.Raw = GET_NEXT_HOB (RscDescHob);
+  }
+
+  if (DoClear) {
+    DEBUG((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
+    SetMem(NULL, EFI_PAGE_SIZE, 0);
+  }
+
+  return;
+}
+
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index 1957326caf..a8aa0d5d1b 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -123,7 +123,8 @@ Create4GPageTablesIa32Pae (
     PageDirectoryPointerEntry->Bits.Present = 1;
 
     for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress += SIZE_2MB) {
-      if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase)) {
+      if ((NULL_DETECTION_ENABLED && PhysicalAddress == 0)
+          || ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
         //
         // Need to split this 2M page that covers stack range.
         //
@@ -240,6 +241,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 +382,8 @@ HandOffToDxeCore (
     TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
 
     PageTables = 0;
-    BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) && IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
+    BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () && IsExecuteDisableBitAvailable ()
+                                        && (PcdGetBool (PcdSetNxForStack) || NULL_DETECTION_ENABLED));
     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..50a8d77a5b 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..ccd6e10cb2 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -90,8 +90,14 @@ Split2MPageTo4K (
     //
     PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
     PageTableEntry->Bits.ReadWrite = 1;
-    PageTableEntry->Bits.Present = 1;
-    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + StackSize)) {
+
+    if (NULL_DETECTION_ENABLED && 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 +143,10 @@ Split1GPageTo2M (
 
   PhysicalAddress2M = PhysicalAddress;
   for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += SIZE_2MB) {
-    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M + SIZE_2MB) > StackBase)) {
+    if ((NULL_DETECTION_ENABLED && 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 +286,8 @@ 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 ((NULL_DETECTION_ENABLED && PageAddress == 0)
+            || (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + StackSize) && ((PageAddress + SIZE_1GB) > StackBase))) {
           Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
         } else {
           //
@@ -308,9 +316,10 @@ 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 ((NULL_DETECTION_ENABLED && 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 {
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 593bff357a..1cc84894af 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -860,6 +860,18 @@
   # @ValidList  0x80000006 | 0x03058002
   gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable|0x03058002|UINT32|0x30001040
 
+  ## Mask to control the NULL address detection in code for different phases.
+  #  If enabled, accessing NULL address in UEFI or SMM code can be caught.<BR><BR>
+  #    BIT0    - Enable NULL pointer detection for UEFI.<BR>
+  #    BIT1    - Enable NULL pointer detection for SMM.<BR>
+  #    BIT2..6 - Reserved for future uses.<BR>
+  #    BIT7    - Disable NULL pointer detection just after EndOfDxe. <BR>
+  #              This is a workaround for those unsolvable NULL access issues in OptionROM, boot loader, etc.
+  #              It can also help to avoid unnecessary exception caused by legacy memory (0-4095) access after 
+  #              EndOfDxe, such as Windows 7 boot on Qemu.<BR>
+  # @Prompt Enable NULL address detection.
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask|0x0|UINT8|0x30001050
+
 [PcdsFixedAtBuild, PcdsPatchableInModule]
   ## Dynamic type PCD can be registered callback function for Pcd setting action.
   #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function
-- 
2.14.1.windows.1

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

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

Re: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

Posted by Jordan Justen 7 years, 3 months ago

On 2017-09-13 02:25:04, Wang, Jian J wrote:
> The mechanism behind is to trigger a page fault exception at address 0. This can be made by disabling page 0 (0-4095) during page table setup. So this feature can only be available on platform with paging enabled. Once this feature is enabled, any code, like CSM, which has to access memory in page 0 needs to enable this page temporarily in advance and disable it afterwards. PcdNullPointerDetectionPropertyMask is used to control and elaborate the use cases.
>

Your commit message line is > 450 columns.

https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format

I think BaseTools/Scripts/PatchCheck.py can help detect this issue.

(more below...)

> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Wolman, Ayellet <ayellet.wolman@intel.com>
> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wang, Jian J <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 +-
>  MdeModulePkg/Core/Dxe/Mem/Page.c                 | 21 ++++++----
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c    | 47 +++++++++++++++++++++
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            | 15 +++++++
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  3 +-
>  MdeModulePkg/Core/DxeIplPeim/DxeLoad.c           | 53 ++++++++++++++++++++++++
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  8 +++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 23 ++++++----
>  MdeModulePkg/MdeModulePkg.dec                    | 12 ++++++
>  10 files changed, 167 insertions(+), 20 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index 30d5984f7c..273b8b7c0e 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -179,7 +179,7 @@
>    gEfiWatchdogTimerArchProtocolGuid             ## CONSUMES
>  
>  [FeaturePcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport         ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport          ## CONSUMES

Why is this line changed?

>  
>  [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber    ## SOMETIMES_CONSUMES
> @@ -192,6 +192,7 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable                   ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy                   ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy             ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask        ## CONSUMES
>  
>  # [Hob]
>  # RESOURCE_DESCRIPTOR   ## CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index a142c79ee2..2e0b72f864 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -170,6 +170,7 @@ CoreAddRange (
>  {
>    LIST_ENTRY        *Link;
>    MEMORY_MAP        *Entry;
> +  EFI_STATUS        Status;
>  
>    ASSERT ((Start & EFI_PAGE_MASK) == 0);
>    ASSERT (End > Start) ;
> @@ -188,7 +189,17 @@ CoreAddRange (
>    // used for other purposes.
>    //  
>    if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 1)) {
> -    SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +    if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
> +      SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +    } else if (gCpu != NULL) {
> +      Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
> +      ASSERT_EFI_ERROR(Status);
> +
> +      SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +
> +      Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, EFI_MEMORY_RP);
> +      ASSERT_EFI_ERROR(Status);
> +    }
>    }
>    
>    //
> @@ -1972,11 +1983,3 @@ Done:
>    return Status;
>  }
>  
> -
> -
> -
> -
> -
> -
> -
> -
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index a73c4ccd64..2367d674e1 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -995,6 +995,36 @@ MemoryProtectionExitBootServicesCallback (
>    }
>  }
>  
> +/**
> +  Disable NULL pointer detection after EndOfDxe. This is a workaround resort in 
> +  order to skip unfixable NULL pointer access issues detected in OptionROM or 
> +  boot loaders.
> +
> +  @param[in]  Event     The Event this notify function registered to.
> +  @param[in]  Context   Pointer to the context data registered to the Event.
> +**/
> +VOID
> +EFIAPI
> +DisableNullDetectionAtTheEndOfDxe (
> +  EFI_EVENT                               Event,
> +  VOID                                    *Context
> +  )
> +{
> +  EFI_STATUS                        Status;
> +
> +  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): start\r\n"));
> +  //
> +  // Disable NULL pointer detection by enabling first 4K page
> +  //
> +  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
> +  ASSERT_EFI_ERROR(Status);
> +
> +  CoreCloseEvent (Event);
> +  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): end\r\n"));
> +
> +  return;
> +}
> +
>  /**
>    Initialize Memory Protection support.
>  **/
> @@ -1006,6 +1036,7 @@ CoreInitializeMemoryProtection (
>  {
>    EFI_STATUS  Status;
>    EFI_EVENT   Event;
> +  EFI_EVENT   EndOfDxeEvent;
>    VOID        *Registration;
>  
>    mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
> @@ -1044,6 +1075,22 @@ CoreInitializeMemoryProtection (
>                 );
>      ASSERT_EFI_ERROR(Status);
>    }
> +
> +  //
> +  // Register a callback to disable NULL pointer detection at EndOfDxe
> +  //
> +  if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == (BIT0|BIT7)) {
> +    Status = CoreCreateEventEx (
> +                    EVT_NOTIFY_SIGNAL,
> +                    TPL_NOTIFY,
> +                    DisableNullDetectionAtTheEndOfDxe,
> +                    NULL,
> +                    &gEfiEndOfDxeEventGroupGuid,
> +                    &EndOfDxeEvent
> +                    );
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
>    return ;
>  }
>  
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> index 72d2532f50..104599156c 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> @@ -52,6 +52,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #define STACK_SIZE      0x20000
>  #define BSP_STORE_SIZE  0x4000
>  
> +#define NULL_DETECTION_ENABLED  ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) != 0)

I think maybe the code style would look better if you defined a
function for this rather than using a macro. What do you think?

>  
>  //
>  // This PPI is installed to indicate the end of the PEI usage of memory
> @@ -240,4 +241,18 @@ 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
> +  );
> +
>  #endif
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index c54afe4aa6..fde70f94bb 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -110,11 +110,12 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplBuildPageTables       ## CONSUMES
>  
>  [FeaturePcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress         ## CONSUMES
>  
>  [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..b5f9d92f5b 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> @@ -825,3 +825,56 @@ 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          RscDescHob;
> +  EFI_PEI_HOB_POINTERS          MemAllocHob;
> +  BOOLEAN                       DoClear;
> +
> +  RscDescHob.Raw = HobStart;
> +  MemAllocHob.Raw = HobStart;
> +  DoClear = FALSE;
> +
> +  //
> +  // Check if page 0 exists and free
> +  //
> +  while ((RscDescHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, RscDescHob.Raw)) != NULL) {

This code line is longer than 80 columns. There are more cases of this
in your patchset.

-Jordan

> +    if (RscDescHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY && 
> +        RscDescHob.ResourceDescriptor->PhysicalStart == 0) {
> +      DoClear = TRUE;
> +      // 
> +      // Make sure memory at 0-4095 has not been allocated.
> +      //
> +      while ((MemAllocHob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, MemAllocHob.Raw)) != NULL) {
> +        if (MemAllocHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress < EFI_PAGE_SIZE) {
> +          DoClear = FALSE;
> +          break;
> +        }
> +        MemAllocHob.Raw = GET_NEXT_HOB (MemAllocHob);
> +      }
> +      break;
> +    }
> +    RscDescHob.Raw = GET_NEXT_HOB (RscDescHob);
> +  }
> +
> +  if (DoClear) {
> +    DEBUG((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
> +    SetMem(NULL, EFI_PAGE_SIZE, 0);
> +  }
> +
> +  return;
> +}
> +
> diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> index 1957326caf..a8aa0d5d1b 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> @@ -123,7 +123,8 @@ Create4GPageTablesIa32Pae (
>      PageDirectoryPointerEntry->Bits.Present = 1;
>  
>      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress += SIZE_2MB) {
> -      if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase)) {
> +      if ((NULL_DETECTION_ENABLED && PhysicalAddress == 0)
> +          || ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
>          //
>          // Need to split this 2M page that covers stack range.
>          //
> @@ -240,6 +241,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 +382,8 @@ HandOffToDxeCore (
>      TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
>  
>      PageTables = 0;
> -    BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) && IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
> +    BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () && IsExecuteDisableBitAvailable ()
> +                                        && (PcdGetBool (PcdSetNxForStack) || NULL_DETECTION_ENABLED));
>      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..50a8d77a5b 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..ccd6e10cb2 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -90,8 +90,14 @@ Split2MPageTo4K (
>      //
>      PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
>      PageTableEntry->Bits.ReadWrite = 1;
> -    PageTableEntry->Bits.Present = 1;
> -    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + StackSize)) {
> +
> +    if (NULL_DETECTION_ENABLED && 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 +143,10 @@ Split1GPageTo2M (
>  
>    PhysicalAddress2M = PhysicalAddress;
>    for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += SIZE_2MB) {
> -    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M + SIZE_2MB) > StackBase)) {
> +    if ((NULL_DETECTION_ENABLED && 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 +286,8 @@ 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 ((NULL_DETECTION_ENABLED && PageAddress == 0)
> +            || (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + StackSize) && ((PageAddress + SIZE_1GB) > StackBase))) {
>            Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
>          } else {
>            //
> @@ -308,9 +316,10 @@ 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 ((NULL_DETECTION_ENABLED && 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 {
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 593bff357a..1cc84894af 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -860,6 +860,18 @@
>    # @ValidList  0x80000006 | 0x03058002
>    gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable|0x03058002|UINT32|0x30001040
>  
> +  ## Mask to control the NULL address detection in code for different phases.
> +  #  If enabled, accessing NULL address in UEFI or SMM code can be caught.<BR><BR>
> +  #    BIT0    - Enable NULL pointer detection for UEFI.<BR>
> +  #    BIT1    - Enable NULL pointer detection for SMM.<BR>
> +  #    BIT2..6 - Reserved for future uses.<BR>
> +  #    BIT7    - Disable NULL pointer detection just after EndOfDxe. <BR>
> +  #              This is a workaround for those unsolvable NULL access issues in OptionROM, boot loader, etc.
> +  #              It can also help to avoid unnecessary exception caused by legacy memory (0-4095) access after 
> +  #              EndOfDxe, such as Windows 7 boot on Qemu.<BR>
> +  # @Prompt Enable NULL address detection.
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask|0x0|UINT8|0x30001050
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule]
>    ## Dynamic type PCD can be registered callback function for Pcd setting action.
>    #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function
> -- 
> 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 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

Posted by Wang, Jian J 7 years, 3 months ago

See my comments start with [Jian] below.

-----Original Message-----
From: Justen, Jordan L 
Sent: Thursday, September 14, 2017 1:28 AM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>
Subject: Re: [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

On 2017-09-13 02:25:04, Wang, Jian J wrote:
> The mechanism behind is to trigger a page fault exception at address 0. This can be made by disabling page 0 (0-4095) during page table setup. So this feature can only be available on platform with paging enabled. Once this feature is enabled, any code, like CSM, which has to access memory in page 0 needs to enable this page temporarily in advance and disable it afterwards. PcdNullPointerDetectionPropertyMask is used to control and elaborate the use cases.
>

Your commit message line is > 450 columns.

https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format

I think BaseTools/Scripts/PatchCheck.py can help detect this issue.

(more below...)

[Jian] Sure.

> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Wolman, Ayellet <ayellet.wolman@intel.com>
> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wang, Jian J <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 +-
>  MdeModulePkg/Core/Dxe/Mem/Page.c                 | 21 ++++++----
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c    | 47 +++++++++++++++++++++
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            | 15 +++++++
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  3 +-
>  MdeModulePkg/Core/DxeIplPeim/DxeLoad.c           | 53 ++++++++++++++++++++++++
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  8 +++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 23 ++++++----
>  MdeModulePkg/MdeModulePkg.dec                    | 12 ++++++
>  10 files changed, 167 insertions(+), 20 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index 30d5984f7c..273b8b7c0e 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -179,7 +179,7 @@
>    gEfiWatchdogTimerArchProtocolGuid             ## CONSUMES
>  
>  [FeaturePcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport         ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport          ## CONSUMES

Why is this line changed?
[Jian] Just align the comment followed because the new one added

>  
>  [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber    ## SOMETIMES_CONSUMES
> @@ -192,6 +192,7 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable                   ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy                   ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy             ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask        ## CONSUMES
>  
>  # [Hob]
>  # RESOURCE_DESCRIPTOR   ## CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index a142c79ee2..2e0b72f864 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -170,6 +170,7 @@ CoreAddRange (
>  {
>    LIST_ENTRY        *Link;
>    MEMORY_MAP        *Entry;
> +  EFI_STATUS        Status;
>  
>    ASSERT ((Start & EFI_PAGE_MASK) == 0);
>    ASSERT (End > Start) ;
> @@ -188,7 +189,17 @@ CoreAddRange (
>    // used for other purposes.
>    //  
>    if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 1)) {
> -    SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +    if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
> +      SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +    } else if (gCpu != NULL) {
> +      Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
> +      ASSERT_EFI_ERROR(Status);
> +
> +      SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +
> +      Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, EFI_MEMORY_RP);
> +      ASSERT_EFI_ERROR(Status);
> +    }
>    }
>    
>    //
> @@ -1972,11 +1983,3 @@ Done:
>    return Status;
>  }
>  
> -
> -
> -
> -
> -
> -
> -
> -
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index a73c4ccd64..2367d674e1 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -995,6 +995,36 @@ MemoryProtectionExitBootServicesCallback (
>    }
>  }
>  
> +/**
> +  Disable NULL pointer detection after EndOfDxe. This is a workaround resort in 
> +  order to skip unfixable NULL pointer access issues detected in OptionROM or 
> +  boot loaders.
> +
> +  @param[in]  Event     The Event this notify function registered to.
> +  @param[in]  Context   Pointer to the context data registered to the Event.
> +**/
> +VOID
> +EFIAPI
> +DisableNullDetectionAtTheEndOfDxe (
> +  EFI_EVENT                               Event,
> +  VOID                                    *Context
> +  )
> +{
> +  EFI_STATUS                        Status;
> +
> +  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): start\r\n"));
> +  //
> +  // Disable NULL pointer detection by enabling first 4K page
> +  //
> +  Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0);
> +  ASSERT_EFI_ERROR(Status);
> +
> +  CoreCloseEvent (Event);
> +  DEBUG((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): end\r\n"));
> +
> +  return;
> +}
> +
>  /**
>    Initialize Memory Protection support.
>  **/
> @@ -1006,6 +1036,7 @@ CoreInitializeMemoryProtection (
>  {
>    EFI_STATUS  Status;
>    EFI_EVENT   Event;
> +  EFI_EVENT   EndOfDxeEvent;
>    VOID        *Registration;
>  
>    mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
> @@ -1044,6 +1075,22 @@ CoreInitializeMemoryProtection (
>                 );
>      ASSERT_EFI_ERROR(Status);
>    }
> +
> +  //
> +  // Register a callback to disable NULL pointer detection at EndOfDxe
> +  //
> +  if ((PcdGet8(PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == (BIT0|BIT7)) {
> +    Status = CoreCreateEventEx (
> +                    EVT_NOTIFY_SIGNAL,
> +                    TPL_NOTIFY,
> +                    DisableNullDetectionAtTheEndOfDxe,
> +                    NULL,
> +                    &gEfiEndOfDxeEventGroupGuid,
> +                    &EndOfDxeEvent
> +                    );
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
>    return ;
>  }
>  
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> index 72d2532f50..104599156c 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
> @@ -52,6 +52,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #define STACK_SIZE      0x20000
>  #define BSP_STORE_SIZE  0x4000
>  
> +#define NULL_DETECTION_ENABLED  ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) != 0)

I think maybe the code style would look better if you defined a
function for this rather than using a macro. What do you think?

[Jian] Ok. I'll add a function instead.

>  
>  //
>  // This PPI is installed to indicate the end of the PEI usage of memory
> @@ -240,4 +241,18 @@ 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
> +  );
> +
>  #endif
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index c54afe4aa6..fde70f94bb 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -110,11 +110,12 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplBuildPageTables       ## CONSUMES
>  
>  [FeaturePcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress         ## CONSUMES
>  
>  [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..b5f9d92f5b 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
> @@ -825,3 +825,56 @@ 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          RscDescHob;
> +  EFI_PEI_HOB_POINTERS          MemAllocHob;
> +  BOOLEAN                       DoClear;
> +
> +  RscDescHob.Raw = HobStart;
> +  MemAllocHob.Raw = HobStart;
> +  DoClear = FALSE;
> +
> +  //
> +  // Check if page 0 exists and free
> +  //
> +  while ((RscDescHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, RscDescHob.Raw)) != NULL) {

This code line is longer than 80 columns. There are more cases of this
in your patchset.

-Jordan

> +    if (RscDescHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY && 
> +        RscDescHob.ResourceDescriptor->PhysicalStart == 0) {
> +      DoClear = TRUE;
> +      // 
> +      // Make sure memory at 0-4095 has not been allocated.
> +      //
> +      while ((MemAllocHob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, MemAllocHob.Raw)) != NULL) {
> +        if (MemAllocHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress < EFI_PAGE_SIZE) {
> +          DoClear = FALSE;
> +          break;
> +        }
> +        MemAllocHob.Raw = GET_NEXT_HOB (MemAllocHob);
> +      }
> +      break;
> +    }
> +    RscDescHob.Raw = GET_NEXT_HOB (RscDescHob);
> +  }
> +
> +  if (DoClear) {
> +    DEBUG((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
> +    SetMem(NULL, EFI_PAGE_SIZE, 0);
> +  }
> +
> +  return;
> +}
> +
> diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> index 1957326caf..a8aa0d5d1b 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> @@ -123,7 +123,8 @@ Create4GPageTablesIa32Pae (
>      PageDirectoryPointerEntry->Bits.Present = 1;
>  
>      for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress += SIZE_2MB) {
> -      if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase)) {
> +      if ((NULL_DETECTION_ENABLED && PhysicalAddress == 0)
> +          || ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
>          //
>          // Need to split this 2M page that covers stack range.
>          //
> @@ -240,6 +241,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 +382,8 @@ HandOffToDxeCore (
>      TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
>  
>      PageTables = 0;
> -    BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) && IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
> +    BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () && IsExecuteDisableBitAvailable ()
> +                                        && (PcdGetBool (PcdSetNxForStack) || NULL_DETECTION_ENABLED));
>      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..50a8d77a5b 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..ccd6e10cb2 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -90,8 +90,14 @@ Split2MPageTo4K (
>      //
>      PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
>      PageTableEntry->Bits.ReadWrite = 1;
> -    PageTableEntry->Bits.Present = 1;
> -    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + StackSize)) {
> +
> +    if (NULL_DETECTION_ENABLED && 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 +143,10 @@ Split1GPageTo2M (
>  
>    PhysicalAddress2M = PhysicalAddress;
>    for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += SIZE_2MB) {
> -    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M + SIZE_2MB) > StackBase)) {
> +    if ((NULL_DETECTION_ENABLED && 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 +286,8 @@ 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 ((NULL_DETECTION_ENABLED && PageAddress == 0)
> +            || (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + StackSize) && ((PageAddress + SIZE_1GB) > StackBase))) {
>            Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
>          } else {
>            //
> @@ -308,9 +316,10 @@ 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 ((NULL_DETECTION_ENABLED && 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 {
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 593bff357a..1cc84894af 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -860,6 +860,18 @@
>    # @ValidList  0x80000006 | 0x03058002
>    gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable|0x03058002|UINT32|0x30001040
>  
> +  ## Mask to control the NULL address detection in code for different phases.
> +  #  If enabled, accessing NULL address in UEFI or SMM code can be caught.<BR><BR>
> +  #    BIT0    - Enable NULL pointer detection for UEFI.<BR>
> +  #    BIT1    - Enable NULL pointer detection for SMM.<BR>
> +  #    BIT2..6 - Reserved for future uses.<BR>
> +  #    BIT7    - Disable NULL pointer detection just after EndOfDxe. <BR>
> +  #              This is a workaround for those unsolvable NULL access issues in OptionROM, boot loader, etc.
> +  #              It can also help to avoid unnecessary exception caused by legacy memory (0-4095) access after 
> +  #              EndOfDxe, such as Windows 7 boot on Qemu.<BR>
> +  # @Prompt Enable NULL address detection.
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask|0x0|UINT8|0x30001050
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule]
>    ## Dynamic type PCD can be registered callback function for Pcd setting action.
>    #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function
> -- 
> 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 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

Posted by Jordan Justen 7 years, 3 months ago

On 2017-09-13 18:25:22, Wang, Jian J wrote:
> See my comments start with [Jian] below.
> 
> -----Original Message-----
> From: Justen, Jordan L 
> Sent: Thursday, September 14, 2017 1:28 AM
> 
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > index 30d5984f7c..273b8b7c0e 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > @@ -179,7 +179,7 @@
> >    gEfiWatchdogTimerArchProtocolGuid             ## CONSUMES
> >  
> >  [FeaturePcd]
> > -  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport         ## CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport          ## CONSUMES
> 
> Why is this line changed?
> [Jian] Just align the comment followed because the new one added

I was looking in the patch, and it didn't look like it was now aligned
with the comments above or below. (It is 1 column less than the
section below, right?)

My opinion is to not change the line since it is in a separate section
of the inf file.

-Jordan

> 
> >  
> >  [Pcd]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber    ## SOMETIMES_CONSUMES
> > @@ -192,6 +192,7 @@
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable                   ## CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy                   ## CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy             ## CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask        ## CONSUMES
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

Posted by Wang, Jian J 7 years, 3 months ago

It's a tab character in old version. It may look like the same if your tab width is to a certain number. In my editor, it looks unaligned. Sometimes I just can't resist the temptation to fix the tab:)   I'll restore it to avoid irrelevant changes in next patch update.

-----Original Message-----
From: Justen, Jordan L 
Sent: Thursday, September 14, 2017 2:34 PM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>
Subject: RE: [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

On 2017-09-13 18:25:22, Wang, Jian J wrote:
> See my comments start with [Jian] below.
> 
> -----Original Message-----
> From: Justen, Jordan L 
> Sent: Thursday, September 14, 2017 1:28 AM
> 
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > index 30d5984f7c..273b8b7c0e 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > @@ -179,7 +179,7 @@
> >    gEfiWatchdogTimerArchProtocolGuid             ## CONSUMES
> >  
> >  [FeaturePcd]
> > -  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport         ## CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport          ## CONSUMES
> 
> Why is this line changed?
> [Jian] Just align the comment followed because the new one added

I was looking in the patch, and it didn't look like it was now aligned
with the comments above or below. (It is 1 column less than the
section below, right?)

My opinion is to not change the line since it is in a separate section
of the inf file.

-Jordan

> 
> >  
> >  [Pcd]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber    ## SOMETIMES_CONSUMES
> > @@ -192,6 +192,7 @@
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable                   ## CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy                   ## CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy             ## CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask        ## CONSUMES
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

Posted by Laszlo Ersek 7 years, 3 months ago

On 09/14/17 08:51, Wang, Jian J wrote:
> It's a tab character in old version. It may look like the same if
> your tab width is to a certain number. In my editor, it looks
> unaligned. Sometimes I just can't resist the temptation to fix the
> tab:)   I'll restore it to avoid irrelevant changes in next patch
> update.

It is perfectly fine (and even recommended, IMO) to fix up such warts
whenever someone comes across them, but all such fixes should be
separated out to their own dedicated patches.

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

[edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.
[edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.
[edk2] [PATCH 3/4] IntelFrameworkModulePkg/Csm: Update CSM code to temporarily bypass NULL pointer detection if enabled.
[edk2] [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.