public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 0/2] Implement NULL pointer detection feature
@ 2017-08-28  2:51 Wang, Jian J
  2017-08-28  2:51 ` [PATCH 1/2] Implement NULL pointer detection for EDK-II Core Wang, Jian J
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Wang, Jian J @ 2017-08-28  2:51 UTC (permalink / raw)
  To: edk2-devel

This patch is the implementation of NULL pointer detection feature,
which is one of the small features of Special Pool.

Wang, Jian J (2):
  Implement NULL pointer detection for EDK-II Core
  Implement NULL pointer detection for EDK-II SMM Core and driver

 MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
 MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26 ++++++++++++++++--------
 MdeModulePkg/MdeModulePkg.dec                    |  7 +++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12 +++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25 ++++++++++++++++++++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12 +++++++++++
 10 files changed, 84 insertions(+), 14 deletions(-)

-- 
2.11.0.windows.1



^ permalink raw reply	[flat|nested] 17+ messages in thread
* [PATCH 1/2] Implement NULL pointer detection for EDK-II Core
  2017-08-28  2:51 [PATCH 0/2] Implement NULL pointer detection feature Wang, Jian J
@ 2017-08-28  2:51 ` Wang, Jian J
  2017-08-28  2:51 ` [PATCH 2/2] Implement NULL pointer detection for EDK-II SMM Core and driver Wang, Jian J
  2017-08-28  3:10 ` [PATCH 0/2] Implement NULL pointer detection feature Yao, Jiewen
  2 siblings, 0 replies; 17+ messages in thread
From: Wang, Jian J @ 2017-08-28  2:51 UTC (permalink / raw)
  To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Eric Dong

This feature is for debug purpose which helps to detect potential
NULL pointer access in code at run-time.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@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                 |  5 +++--
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26 ++++++++++++++++--------
 MdeModulePkg/MdeModulePkg.dec                    |  7 +++++++
 6 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 30d5984f7c..3d75a0014d 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -179,7 +179,8 @@
   gEfiWatchdogTimerArchProtocolGuid             ## CONSUMES
 
 [FeaturePcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport	   ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport          ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection                   ## CONSUMES
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber    ## SOMETIMES_CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index a142c79ee2..3fe77391b7 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -185,9 +185,10 @@ CoreAddRange (
   // compatibility with operating systems that may evaluate memory in this page 
   // for legacy data structures.  If memory of any other type is added starting 
   // at address 0, then do not zero the page at address 0 because the page is being 
-  // used for other purposes.
+  // used for other purposes. But don't do this if NULL pointer detection mechanism 
+  // is used.
   //  
-  if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 1)) {
+  if (!PcdGetBool(PcdNullPointerDetection) && Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 1)) {
     SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
   }
   
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index c54afe4aa6..6b4d68cfa1 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -111,6 +111,7 @@
 
 [FeaturePcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection        ## CONSUMES
 
 [Pcd.IA32,Pcd.X64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable                      ## SOMETIMES_CONSUMES
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index 1957326caf..d4e1b7c858 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 ((PcdGetBool(PcdNullPointerDetection) && PhysicalAddress == 0)
+          || ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
         //
         // Need to split this 2M page that covers stack range.
         //
@@ -379,7 +380,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) || PcdGetBool (PcdNullPointerDetection)));
     if (BuildPageTablesIa32Pae) {
       PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
       EnableExecuteDisableBit ();
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index 48150be4e1..c69f889d9e 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -89,9 +89,16 @@ Split2MPageTo4K (
     // Fill in the Page Table entries
     //
     PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
-    PageTableEntry->Bits.ReadWrite = 1;
-    PageTableEntry->Bits.Present = 1;
-    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + StackSize)) {
+
+    if (PcdGetBool(PcdNullPointerDetection) && PhysicalAddress4K == 0) {
+      PageTableEntry->Bits.ReadWrite = 0;
+      PageTableEntry->Bits.Present = 0;
+    } else {
+      PageTableEntry->Bits.ReadWrite = 1;
+      PageTableEntry->Bits.Present = 1;
+    }
+
+    if (PcdGetBool (PcdSetNxForStack) && (PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + StackSize)) {
       //
       // Set Nx bit for stack.
       //
@@ -137,9 +144,10 @@ Split1GPageTo2M (
 
   PhysicalAddress2M = PhysicalAddress;
   for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += SIZE_2MB) {
-    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M + SIZE_2MB) > StackBase)) {
+    if ((PcdGetBool(PcdNullPointerDetection) && 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 +287,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 ((PcdGetBool (PcdNullPointerDetection) && PageAddress == 0)
+            || (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + StackSize) && ((PageAddress + SIZE_1GB) > StackBase))) {
           Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize);
         } else {
           //
@@ -308,9 +317,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 ((PcdGetBool (PcdNullPointerDetection) && 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..713593dc38 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -802,6 +802,13 @@
   # @Prompt Degrade 64-bit PCI MMIO BARs for legacy BIOS option ROMs
   gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeResourceForOptionRom|TRUE|BOOLEAN|0x0001003a
 
+  ## Indicates if NULL address detection will be enabled.
+  #  If enabled, accessing NULL address in UEFI can be caught.<BR><BR>
+  #   TRUE  - NULL address detection will be enabled.<BR>
+  #   FALSE - NULL address detection will be disabled.<BR>
+  # @Prompt Enable NULL address detection.
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE|BOOLEAN|0x0004003d
+
 [PcdsFeatureFlag.IA32, PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeResourceForOptionRom|FALSE|BOOLEAN|0x0001003a
 
-- 
2.11.0.windows.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH 2/2] Implement NULL pointer detection for EDK-II SMM Core and driver
  2017-08-28  2:51 [PATCH 0/2] Implement NULL pointer detection feature Wang, Jian J
  2017-08-28  2:51 ` [PATCH 1/2] Implement NULL pointer detection for EDK-II Core Wang, Jian J
@ 2017-08-28  2:51 ` Wang, Jian J
  2017-08-28  3:10 ` [PATCH 0/2] Implement NULL pointer detection feature Yao, Jiewen
  2 siblings, 0 replies; 17+ messages in thread
From: Wang, Jian J @ 2017-08-28  2:51 UTC (permalink / raw)
  To: edk2-devel; +Cc: Eric Dong, Star Zeng, Jiewen Yao

This feature is for debug purpose which helps to detect potential
NULL pointer access in code at run-time in SMM mode.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiewen Yao <jiewen.yao@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>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c     | 12 ++++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c        | 25 ++++++++++++++++++++++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |  1 +
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c      | 12 ++++++++++++
 4 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index 32ce5958c5..3ad7e9a10f 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -153,6 +153,18 @@ SmiPFHandler (
     }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (FeaturePcdGet(PcdNullPointerDetection)
+      && (PFAddress >= 0 && PFAddress < EFI_PAGE_SIZE)) {
+    DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
+    DEBUG_CODE (
+      DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Rip);
+    );
+    CpuDeadLoop ();
+  }
+
   if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
     SmmProfilePFHandler (
       SystemContext.SystemContextIa32->Eip,
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index f086b97c30..bba716c66f 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -855,10 +855,10 @@ Gen4GPageTable (
     Pte[Index] = (Index << 21) | mAddressEncMask | IA32_PG_PS | PAGE_ATTRIBUTE_BITS;
   }
 
+  Pdpte = (UINT64*)PageTable;
   if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
     Pages = (UINTN)PageTable + EFI_PAGES_TO_SIZE (5);
     GuardPage = mSmmStackArrayBase + EFI_PAGE_SIZE;
-    Pdpte = (UINT64*)PageTable;
     for (PageIndex = Low2MBoundary; PageIndex <= High2MBoundary; PageIndex += SIZE_2MB) {
       Pte = (UINT64*)(UINTN)(Pdpte[BitFieldRead32 ((UINT32)PageIndex, 30, 31)] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));
       Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] = (UINT64)Pages | mAddressEncMask | PAGE_ATTRIBUTE_BITS;
@@ -886,6 +886,29 @@ Gen4GPageTable (
     }
   }
 
+  if (FeaturePcdGet(PcdNullPointerDetection)) {
+    Pte = (UINT64*)(UINT64)(Pdpte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));
+    if ((Pte[0] & IA32_PG_PS) == 0) {
+      // 4K-page entries are already mapped. Just hide the first one anyway.
+      Pte = (UINT64*)(UINT64)(Pte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));
+      Pte[0] &= ~1; // Hide page 0
+    } else {
+      // Create 4K-page entries
+      Pages = (UINTN)AllocatePageTableMemory (1);
+      ASSERT (Pages != 0);
+
+      Pte[0] = (UINT64)(Pages | mAddressEncMask | PAGE_ATTRIBUTE_BITS);
+
+      Pte = (UINT64*)Pages;
+      PageAddress = 0;
+      Pte[0] = PageAddress | mAddressEncMask; // Hide page 0 but present left
+      for (Index = 1; Index < EFI_PAGE_SIZE / sizeof (*Pte); Index++) {
+        PageAddress += EFI_PAGE_SIZE;
+        Pte[Index] = PageAddress | mAddressEncMask | PAGE_ATTRIBUTE_BITS;
+      }
+    }
+  }
+
   return (UINT32)(UINTN)PageTable;
 }
 
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 099792e6ce..e28b89bce1 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -146,6 +146,7 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable                 ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer             ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock         ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection           ## CONSUMES
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 32385faae4..82427e176c 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -873,6 +873,18 @@ SmiPFHandler (
     }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (FeaturePcdGet(PcdNullPointerDetection)
+      && (PFAddress >= 0 && PFAddress < EFI_PAGE_SIZE)) {
+    DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
+    DEBUG_CODE (
+      DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip);
+    );
+    CpuDeadLoop ();
+  }
+
   if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
     SmmProfilePFHandler (
       SystemContext.SystemContextX64->Rip,
-- 
2.11.0.windows.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-08-28  2:51 [PATCH 0/2] Implement NULL pointer detection feature Wang, Jian J
  2017-08-28  2:51 ` [PATCH 1/2] Implement NULL pointer detection for EDK-II Core Wang, Jian J
  2017-08-28  2:51 ` [PATCH 2/2] Implement NULL pointer detection for EDK-II SMM Core and driver Wang, Jian J
@ 2017-08-28  3:10 ` Yao, Jiewen
  2017-08-28  3:24   ` Wang, Jian J
  2 siblings, 1 reply; 17+ messages in thread
From: Yao, Jiewen @ 2017-08-28  3:10 UTC (permalink / raw)
  To: Wang, Jian J, edk2-devel@lists.01.org

Thank you  to enable this feature.

I have 2 comments, after a very quick review.


1)      I notice it is enabled by default "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".

Would you please provide the information on how many open source platforms are validated?
Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
Or do we need validate any ARM platform?



2)      I am thinking about CSM platform. Do you think we can make it dynamic, as such, a platform may set the validate based upon CSM enable/disable?


Or if we need update the CSM module to patch the page table automatically? Once this is feature is ON.


Thank you
Yao Jiewen


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Monday, August 28, 2017 10:51 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> This patch is the implementation of NULL pointer detection feature,
> which is one of the small features of Special Pool.
>
> Wang, Jian J (2):
>   Implement NULL pointer detection for EDK-II Core
>   Implement NULL pointer detection for EDK-II SMM Core and driver
>
>  MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
>  MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
> ++++++++++++++++--------
>  MdeModulePkg/MdeModulePkg.dec                    |  7 +++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12 +++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
> ++++++++++++++++++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12 +++++++++++
>  10 files changed, 84 insertions(+), 14 deletions(-)
>
> --
> 2.11.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-08-28  3:10 ` [PATCH 0/2] Implement NULL pointer detection feature Yao, Jiewen
@ 2017-08-28  3:24   ` Wang, Jian J
  2017-08-28  3:39     ` Yao, Jiewen
  0 siblings, 1 reply; 17+ messages in thread
From: Wang, Jian J @ 2017-08-28  3:24 UTC (permalink / raw)
  To: Yao, Jiewen, edk2-devel@lists.01.org

1)      I think this feature should be 'FALSE' by default. I forgot to reset its default value. This feature makes use of page mechanism to detect NULL pointer. So any ARCH doesn't support paging in EDK-II can't use it. Currently validated platform includes VLV2 and Denlow. Let me know if all platform must be validated or not.

2)      It's hard to make it a dynamic feature because we need to setup page table for physical address 0-4095 in advance. If there's no memory alloc/free action after enabling this feature, there's no chance to make those change in page table. Then the usage of feature will be limited in such case.

From: Yao, Jiewen
Sent: Monday, August 28, 2017 11:10 AM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Thank you  to enable this feature.

I have 2 comments, after a very quick review.


1)      I notice it is enabled by default "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".

Would you please provide the information on how many open source platforms are validated?
Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
Or do we need validate any ARM platform?



2)      I am thinking about CSM platform. Do you think we can make it dynamic, as such, a platform may set the validate based upon CSM enable/disable?


Or if we need update the CSM module to patch the page table automatically? Once this is feature is ON.


Thank you
Yao Jiewen


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Monday, August 28, 2017 10:51 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> This patch is the implementation of NULL pointer detection feature,
> which is one of the small features of Special Pool.
>
> Wang, Jian J (2):
>   Implement NULL pointer detection for EDK-II Core
>   Implement NULL pointer detection for EDK-II SMM Core and driver
>
>  MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
>  MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
> ++++++++++++++++--------
>  MdeModulePkg/MdeModulePkg.dec                    |  7 +++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12 +++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
> ++++++++++++++++++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12 +++++++++++
>  10 files changed, 84 insertions(+), 14 deletions(-)
>
> --
> 2.11.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-08-28  3:24   ` Wang, Jian J
@ 2017-08-28  3:39     ` Yao, Jiewen
  2017-08-29 15:16       ` Johnson, Brian (EXL - Eagan)
  0 siblings, 1 reply; 17+ messages in thread
From: Yao, Jiewen @ 2017-08-28  3:39 UTC (permalink / raw)
  To: Wang, Jian J, edk2-devel@lists.01.org

Comment in line.

From: Wang, Jian J
Sent: Monday, August 28, 2017 11:24 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature


1)      I think this feature should be 'FALSE' by default. I forgot to reset its default value. This feature makes use of page mechanism to detect NULL pointer. So any ARCH doesn't support paging in EDK-II can't use it. Currently validated platform includes VLV2 and Denlow. Let me know if all platform must be validated or not.

[Jiewen] Yes, I am OK to set to be FALSE to provide best compatibility.
I suggest we validate all open source IA platforms, such as Quark, and OVMF with TRUE.


2)      It's hard to make it a dynamic feature because we need to setup page table for physical address 0-4095 in advance. If there's no memory alloc/free action after enabling this feature, there's no chance to make those change in page table. Then the usage of feature will be limited in such case.

[Jiewen] Dynamic means the initial value is dynamic. I am not saying we need modify the page table.

An implementation could be:
A platform can set this PCD in PEI phase based upon a setup variable to choose CSM enable or disable.

The IPL sets page table based upon this PCD value. The DXE Core cannot consume PCD directly, because it might be dynamic. But we can pass the information from IPL via HOB. All the DXE module just checks the value based upon HOB.




From: Yao, Jiewen
Sent: Monday, August 28, 2017 11:10 AM
To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Thank you  to enable this feature.

I have 2 comments, after a very quick review.


1)      I notice it is enabled by default "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".

Would you please provide the information on how many open source platforms are validated?
Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
Or do we need validate any ARM platform?



2)      I am thinking about CSM platform. Do you think we can make it dynamic, as such, a platform may set the validate based upon CSM enable/disable?


Or if we need update the CSM module to patch the page table automatically? Once this is feature is ON.


Thank you
Yao Jiewen


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Monday, August 28, 2017 10:51 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> This patch is the implementation of NULL pointer detection feature,
> which is one of the small features of Special Pool.
>
> Wang, Jian J (2):
>   Implement NULL pointer detection for EDK-II Core
>   Implement NULL pointer detection for EDK-II SMM Core and driver
>
>  MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
>  MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
> ++++++++++++++++--------
>  MdeModulePkg/MdeModulePkg.dec                    |  7 +++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12 +++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
> ++++++++++++++++++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12 +++++++++++
>  10 files changed, 84 insertions(+), 14 deletions(-)
>
> --
> 2.11.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-08-28  3:39     ` Yao, Jiewen
@ 2017-08-29 15:16       ` Johnson, Brian (EXL - Eagan)
  2017-08-29 16:02         ` Kinney, Michael D
  2017-08-30 14:55         ` Yao, Jiewen
  0 siblings, 2 replies; 17+ messages in thread
From: Johnson, Brian (EXL - Eagan) @ 2017-08-29 15:16 UTC (permalink / raw)
  To: Yao, Jiewen, Wang, Jian J, edk2-devel@lists.01.org

Thank you for implementing this feature!  It is very  helpful for catching pointer-related problems.  We have used a similar scheme on our systems for years, and caught several important bugs.  Some comments:

* It's possible to implement similar protections in PEI (IA32) by modifying the GDT descriptors.

* For flexibility, I'd like NULL pointer protection to be controlled independently in PEI, DXE, and SMM, using separate PCDs.

* We have seen various option ROMs and OS boot loaders which have NULL pointer issues, but are outside of our control.  It is useful to enable NULL pointer protection during as much of the boot as possible, but disable it before running these other executables.  So I'd suggest adding another PCD, perhaps PcdNullPointerDetectionPostDxe, to control NULL pointer protection late in boot.  If PcdNullPointerDetection != PcdNullPointerDetectionPostDxe, install an end-of-DXE event (gEfiEndOfDxeEventGroupGuid) which changes the protection of page 0 using a call to EFI_CPU_ARCH_PROTOCOL. SetMemoryAttributes(CpuArch, 0, EFI_PAGE_SIZE, 0).

So ideally I'd like to have 4 PCDs:

PcdNullPointerDetectionPei
PcdNullPointerDetectionDxe
PcdNullPointerDetectionSmm
PcdNullPointerDetectionPostDxe

Thanks,
Brian Johnson
HPE

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Sunday, August 27, 2017 10:39 PM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Comment in line.

From: Wang, Jian J
Sent: Monday, August 28, 2017 11:24 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature


1)      I think this feature should be 'FALSE' by default. I forgot to reset its default value. This feature makes use of page mechanism to detect NULL pointer. So any ARCH doesn't support paging in EDK-II can't use it. Currently validated platform includes VLV2 and Denlow. Let me know if all platform must be validated or not.

[Jiewen] Yes, I am OK to set to be FALSE to provide best compatibility.
I suggest we validate all open source IA platforms, such as Quark, and OVMF with TRUE.


2)      It's hard to make it a dynamic feature because we need to setup page table for physical address 0-4095 in advance. If there's no memory alloc/free action after enabling this feature, there's no chance to make those change in page table. Then the usage of feature will be limited in such case.

[Jiewen] Dynamic means the initial value is dynamic. I am not saying we need modify the page table.

An implementation could be:
A platform can set this PCD in PEI phase based upon a setup variable to choose CSM enable or disable.

The IPL sets page table based upon this PCD value. The DXE Core cannot consume PCD directly, because it might be dynamic. But we can pass the information from IPL via HOB. All the DXE module just checks the value based upon HOB.




From: Yao, Jiewen
Sent: Monday, August 28, 2017 11:10 AM
To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Thank you  to enable this feature.

I have 2 comments, after a very quick review.


1)      I notice it is enabled by default "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".

Would you please provide the information on how many open source platforms are validated?
Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
Or do we need validate any ARM platform?



2)      I am thinking about CSM platform. Do you think we can make it dynamic, as such, a platform may set the validate based upon CSM enable/disable?


Or if we need update the CSM module to patch the page table automatically? Once this is feature is ON.


Thank you
Yao Jiewen


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Monday, August 28, 2017 10:51 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> This patch is the implementation of NULL pointer detection feature,
> which is one of the small features of Special Pool.
>
> Wang, Jian J (2):
>   Implement NULL pointer detection for EDK-II Core
>   Implement NULL pointer detection for EDK-II SMM Core and driver
>
>  MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
>  MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
> ++++++++++++++++--------
>  MdeModulePkg/MdeModulePkg.dec                    |  7 +++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12 +++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
> ++++++++++++++++++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12 +++++++++++
>  10 files changed, 84 insertions(+), 14 deletions(-)
>
> --
> 2.11.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto: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


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-08-29 15:16       ` Johnson, Brian (EXL - Eagan)
@ 2017-08-29 16:02         ` Kinney, Michael D
  2017-08-29 17:12           ` Johnson, Brian (EXL - Eagan)
  2017-08-30 14:55         ` Yao, Jiewen
  1 sibling, 1 reply; 17+ messages in thread
From: Kinney, Michael D @ 2017-08-29 16:02 UTC (permalink / raw)
  To: Johnson, Brian (EXL - Eagan), Yao, Jiewen, Wang, Jian J,
	edk2-devel@lists.01.org, Kinney, Michael D

Brian,

Do you prefer 4 new PCDs or one new PCD with a bitmask to enable
detection in different phases?

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of Johnson, Brian (EXL - Eagan)
> Sent: Tuesday, August 29, 2017 8:17 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer
> detection feature
> 
> Thank you for implementing this feature!  It is very  helpful
> for catching pointer-related problems.  We have used a similar
> scheme on our systems for years, and caught several important
> bugs.  Some comments:
> 
> * It's possible to implement similar protections in PEI (IA32)
> by modifying the GDT descriptors.
> 
> * For flexibility, I'd like NULL pointer protection to be
> controlled independently in PEI, DXE, and SMM, using separate
> PCDs.
> 
> * We have seen various option ROMs and OS boot loaders which
> have NULL pointer issues, but are outside of our control.  It
> is useful to enable NULL pointer protection during as much of
> the boot as possible, but disable it before running these other
> executables.  So I'd suggest adding another PCD, perhaps
> PcdNullPointerDetectionPostDxe, to control NULL pointer
> protection late in boot.  If PcdNullPointerDetection !=
> PcdNullPointerDetectionPostDxe, install an end-of-DXE event
> (gEfiEndOfDxeEventGroupGuid) which changes the protection of
> page 0 using a call to EFI_CPU_ARCH_PROTOCOL.
> SetMemoryAttributes(CpuArch, 0, EFI_PAGE_SIZE, 0).
> 
> So ideally I'd like to have 4 PCDs:
> 
> PcdNullPointerDetectionPei
> PcdNullPointerDetectionDxe
> PcdNullPointerDetectionSmm
> PcdNullPointerDetectionPostDxe
> 
> Thanks,
> Brian Johnson
> HPE
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of Yao, Jiewen
> Sent: Sunday, August 27, 2017 10:39 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-
> devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer
> detection feature
> 
> Comment in line.
> 
> From: Wang, Jian J
> Sent: Monday, August 28, 2017 11:24 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer
> detection feature
> 
> 
> 1)      I think this feature should be 'FALSE' by default. I
> forgot to reset its default value. This feature makes use of
> page mechanism to detect NULL pointer. So any ARCH doesn't
> support paging in EDK-II can't use it. Currently validated
> platform includes VLV2 and Denlow. Let me know if all platform
> must be validated or not.
> 
> [Jiewen] Yes, I am OK to set to be FALSE to provide best
> compatibility.
> I suggest we validate all open source IA platforms, such as
> Quark, and OVMF with TRUE.
> 
> 
> 2)      It's hard to make it a dynamic feature because we need
> to setup page table for physical address 0-4095 in advance. If
> there's no memory alloc/free action after enabling this
> feature, there's no chance to make those change in page table.
> Then the usage of feature will be limited in such case.
> 
> [Jiewen] Dynamic means the initial value is dynamic. I am not
> saying we need modify the page table.
> 
> An implementation could be:
> A platform can set this PCD in PEI phase based upon a setup
> variable to choose CSM enable or disable.
> 
> The IPL sets page table based upon this PCD value. The DXE Core
> cannot consume PCD directly, because it might be dynamic. But
> we can pass the information from IPL via HOB. All the DXE
> module just checks the value based upon HOB.
> 
> 
> 
> 
> From: Yao, Jiewen
> Sent: Monday, August 28, 2017 11:10 AM
> To: Wang, Jian J
> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-
> devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer
> detection feature
> 
> Thank you  to enable this feature.
> 
> I have 2 comments, after a very quick review.
> 
> 
> 1)      I notice it is enabled by default
> "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".
> 
> Would you please provide the information on how many open
> source platforms are validated?
> Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
> Or do we need validate any ARM platform?
> 
> 
> 
> 2)      I am thinking about CSM platform. Do you think we can
> make it dynamic, as such, a platform may set the validate based
> upon CSM enable/disable?
> 
> 
> Or if we need update the CSM module to patch the page table
> automatically? Once this is feature is ON.
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of Wang,
> > Jian J
> > Sent: Monday, August 28, 2017 10:51 AM
> > To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection
> feature
> >
> > This patch is the implementation of NULL pointer detection
> feature,
> > which is one of the small features of Special Pool.
> >
> > Wang, Jian J (2):
> >   Implement NULL pointer detection for EDK-II Core
> >   Implement NULL pointer detection for EDK-II SMM Core and
> driver
> >
> >  MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
> >  MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
> >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
> >  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
> >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
> > ++++++++++++++++--------
> >  MdeModulePkg/MdeModulePkg.dec                    |  7
> +++++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12
> +++++++++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
> > ++++++++++++++++++++++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12
> +++++++++++
> >  10 files changed, 84 insertions(+), 14 deletions(-)
> >
> > --
> > 2.11.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org<mailto: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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-08-29 16:02         ` Kinney, Michael D
@ 2017-08-29 17:12           ` Johnson, Brian (EXL - Eagan)
  2017-08-29 17:14             ` Tim Lewis
  0 siblings, 1 reply; 17+ messages in thread
From: Johnson, Brian (EXL - Eagan) @ 2017-08-29 17:12 UTC (permalink / raw)
  To: Kinney, Michael D, Yao, Jiewen, Wang, Jian J,
	edk2-devel@lists.01.org

It makes no difference to me.  But it sounds more flexible and less cumbersome to use 4 PCDs.  That way you don't have to define the meanings of individual bits, in the code or in the .dec file.  And you could use different PCD types for the different PCDs, eg. FeatureFlag or FixedAtBuild for PcdNullPointerDetectionPei (since the GCDs are built at compile time, and it would take quite a bit of recoding to change that) but Dynamic for PcdNullPointerDetectionDxe, as Jiewen requested.

But I'm good either way.

Brian Johnson

-----Original Message-----
From: Kinney, Michael D [mailto:michael.d.kinney@intel.com] 
Sent: Tuesday, August 29, 2017 11:02 AM
To: Johnson, Brian (EXL - Eagan) <brian.johnson@hpe.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Brian,

Do you prefer 4 new PCDs or one new PCD with a bitmask to enable
detection in different phases?

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of Johnson, Brian (EXL - Eagan)
> Sent: Tuesday, August 29, 2017 8:17 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer
> detection feature
> 
> Thank you for implementing this feature!  It is very  helpful
> for catching pointer-related problems.  We have used a similar
> scheme on our systems for years, and caught several important
> bugs.  Some comments:
> 
> * It's possible to implement similar protections in PEI (IA32)
> by modifying the GDT descriptors.
> 
> * For flexibility, I'd like NULL pointer protection to be
> controlled independently in PEI, DXE, and SMM, using separate
> PCDs.
> 
> * We have seen various option ROMs and OS boot loaders which
> have NULL pointer issues, but are outside of our control.  It
> is useful to enable NULL pointer protection during as much of
> the boot as possible, but disable it before running these other
> executables.  So I'd suggest adding another PCD, perhaps
> PcdNullPointerDetectionPostDxe, to control NULL pointer
> protection late in boot.  If PcdNullPointerDetection !=
> PcdNullPointerDetectionPostDxe, install an end-of-DXE event
> (gEfiEndOfDxeEventGroupGuid) which changes the protection of
> page 0 using a call to EFI_CPU_ARCH_PROTOCOL.
> SetMemoryAttributes(CpuArch, 0, EFI_PAGE_SIZE, 0).
> 
> So ideally I'd like to have 4 PCDs:
> 
> PcdNullPointerDetectionPei
> PcdNullPointerDetectionDxe
> PcdNullPointerDetectionSmm
> PcdNullPointerDetectionPostDxe
> 
> Thanks,
> Brian Johnson
> HPE
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of Yao, Jiewen
> Sent: Sunday, August 27, 2017 10:39 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-
> devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer
> detection feature
> 
> Comment in line.
> 
> From: Wang, Jian J
> Sent: Monday, August 28, 2017 11:24 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer
> detection feature
> 
> 
> 1)      I think this feature should be 'FALSE' by default. I
> forgot to reset its default value. This feature makes use of
> page mechanism to detect NULL pointer. So any ARCH doesn't
> support paging in EDK-II can't use it. Currently validated
> platform includes VLV2 and Denlow. Let me know if all platform
> must be validated or not.
> 
> [Jiewen] Yes, I am OK to set to be FALSE to provide best
> compatibility.
> I suggest we validate all open source IA platforms, such as
> Quark, and OVMF with TRUE.
> 
> 
> 2)      It's hard to make it a dynamic feature because we need
> to setup page table for physical address 0-4095 in advance. If
> there's no memory alloc/free action after enabling this
> feature, there's no chance to make those change in page table.
> Then the usage of feature will be limited in such case.
> 
> [Jiewen] Dynamic means the initial value is dynamic. I am not
> saying we need modify the page table.
> 
> An implementation could be:
> A platform can set this PCD in PEI phase based upon a setup
> variable to choose CSM enable or disable.
> 
> The IPL sets page table based upon this PCD value. The DXE Core
> cannot consume PCD directly, because it might be dynamic. But
> we can pass the information from IPL via HOB. All the DXE
> module just checks the value based upon HOB.
> 
> 
> 
> 
> From: Yao, Jiewen
> Sent: Monday, August 28, 2017 11:10 AM
> To: Wang, Jian J
> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-
> devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer
> detection feature
> 
> Thank you  to enable this feature.
> 
> I have 2 comments, after a very quick review.
> 
> 
> 1)      I notice it is enabled by default
> "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".
> 
> Would you please provide the information on how many open
> source platforms are validated?
> Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
> Or do we need validate any ARM platform?
> 
> 
> 
> 2)      I am thinking about CSM platform. Do you think we can
> make it dynamic, as such, a platform may set the validate based
> upon CSM enable/disable?
> 
> 
> Or if we need update the CSM module to patch the page table
> automatically? Once this is feature is ON.
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of Wang,
> > Jian J
> > Sent: Monday, August 28, 2017 10:51 AM
> > To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection
> feature
> >
> > This patch is the implementation of NULL pointer detection
> feature,
> > which is one of the small features of Special Pool.
> >
> > Wang, Jian J (2):
> >   Implement NULL pointer detection for EDK-II Core
> >   Implement NULL pointer detection for EDK-II SMM Core and
> driver
> >
> >  MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
> >  MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
> >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
> >  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
> >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
> > ++++++++++++++++--------
> >  MdeModulePkg/MdeModulePkg.dec                    |  7
> +++++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12
> +++++++++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
> > ++++++++++++++++++++++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12
> +++++++++++
> >  10 files changed, 84 insertions(+), 14 deletions(-)
> >
> > --
> > 2.11.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org<mailto: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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-08-29 17:12           ` Johnson, Brian (EXL - Eagan)
@ 2017-08-29 17:14             ` Tim Lewis
  0 siblings, 0 replies; 17+ messages in thread
From: Tim Lewis @ 2017-08-29 17:14 UTC (permalink / raw)
  To: Johnson, Brian (EXL - Eagan), Kinney, Michael D, Yao, Jiewen,
	Wang, Jian J, edk2-devel@lists.01.org

Just a +1 for 4 separate PCDs.

Tim

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Johnson, Brian (EXL - Eagan)
Sent: Tuesday, August 29, 2017 10:12 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

It makes no difference to me.  But it sounds more flexible and less cumbersome to use 4 PCDs.  That way you don't have to define the meanings of individual bits, in the code or in the .dec file.  And you could use different PCD types for the different PCDs, eg. FeatureFlag or FixedAtBuild for PcdNullPointerDetectionPei (since the GCDs are built at compile time, and it would take quite a bit of recoding to change that) but Dynamic for PcdNullPointerDetectionDxe, as Jiewen requested.

But I'm good either way.

Brian Johnson

-----Original Message-----
From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
Sent: Tuesday, August 29, 2017 11:02 AM
To: Johnson, Brian (EXL - Eagan) <brian.johnson@hpe.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Brian,

Do you prefer 4 new PCDs or one new PCD with a bitmask to enable detection in different phases?

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Johnson, Brian (EXL - Eagan)
> Sent: Tuesday, August 29, 2017 8:17 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J 
> <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection 
> feature
> 
> Thank you for implementing this feature!  It is very  helpful for 
> catching pointer-related problems.  We have used a similar scheme on 
> our systems for years, and caught several important bugs.  Some 
> comments:
> 
> * It's possible to implement similar protections in PEI (IA32) by 
> modifying the GDT descriptors.
> 
> * For flexibility, I'd like NULL pointer protection to be controlled 
> independently in PEI, DXE, and SMM, using separate PCDs.
> 
> * We have seen various option ROMs and OS boot loaders which have NULL 
> pointer issues, but are outside of our control.  It is useful to 
> enable NULL pointer protection during as much of the boot as possible, 
> but disable it before running these other executables.  So I'd suggest 
> adding another PCD, perhaps PcdNullPointerDetectionPostDxe, to control 
> NULL pointer protection late in boot.  If PcdNullPointerDetection != 
> PcdNullPointerDetectionPostDxe, install an end-of-DXE event
> (gEfiEndOfDxeEventGroupGuid) which changes the protection of page 0 
> using a call to EFI_CPU_ARCH_PROTOCOL.
> SetMemoryAttributes(CpuArch, 0, EFI_PAGE_SIZE, 0).
> 
> So ideally I'd like to have 4 PCDs:
> 
> PcdNullPointerDetectionPei
> PcdNullPointerDetectionDxe
> PcdNullPointerDetectionSmm
> PcdNullPointerDetectionPostDxe
> 
> Thanks,
> Brian Johnson
> HPE
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Yao, Jiewen
> Sent: Sunday, August 27, 2017 10:39 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2- devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection 
> feature
> 
> Comment in line.
> 
> From: Wang, Jian J
> Sent: Monday, August 28, 2017 11:24 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection 
> feature
> 
> 
> 1)      I think this feature should be 'FALSE' by default. I
> forgot to reset its default value. This feature makes use of page 
> mechanism to detect NULL pointer. So any ARCH doesn't support paging 
> in EDK-II can't use it. Currently validated platform includes VLV2 and 
> Denlow. Let me know if all platform must be validated or not.
> 
> [Jiewen] Yes, I am OK to set to be FALSE to provide best 
> compatibility.
> I suggest we validate all open source IA platforms, such as Quark, and 
> OVMF with TRUE.
> 
> 
> 2)      It's hard to make it a dynamic feature because we need
> to setup page table for physical address 0-4095 in advance. If there's 
> no memory alloc/free action after enabling this feature, there's no 
> chance to make those change in page table.
> Then the usage of feature will be limited in such case.
> 
> [Jiewen] Dynamic means the initial value is dynamic. I am not saying 
> we need modify the page table.
> 
> An implementation could be:
> A platform can set this PCD in PEI phase based upon a setup variable 
> to choose CSM enable or disable.
> 
> The IPL sets page table based upon this PCD value. The DXE Core cannot 
> consume PCD directly, because it might be dynamic. But we can pass the 
> information from IPL via HOB. All the DXE module just checks the value 
> based upon HOB.
> 
> 
> 
> 
> From: Yao, Jiewen
> Sent: Monday, August 28, 2017 11:10 AM
> To: Wang, Jian J
> <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2- 
> devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection 
> feature
> 
> Thank you  to enable this feature.
> 
> I have 2 comments, after a very quick review.
> 
> 
> 1)      I notice it is enabled by default
> "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".
> 
> Would you please provide the information on how many open source 
> platforms are validated?
> Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
> Or do we need validate any ARM platform?
> 
> 
> 
> 2)      I am thinking about CSM platform. Do you think we can
> make it dynamic, as such, a platform may set the validate based upon 
> CSM enable/disable?
> 
> 
> Or if we need update the CSM module to patch the page table 
> automatically? Once this is feature is ON.
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of Wang,
> > Jian J
> > Sent: Monday, August 28, 2017 10:51 AM
> > To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> > Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection
> feature
> >
> > This patch is the implementation of NULL pointer detection
> feature,
> > which is one of the small features of Special Pool.
> >
> > Wang, Jian J (2):
> >   Implement NULL pointer detection for EDK-II Core
> >   Implement NULL pointer detection for EDK-II SMM Core and
> driver
> >
> >  MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
> >  MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
> >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
> >  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--  
> > MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
> > ++++++++++++++++--------
> >  MdeModulePkg/MdeModulePkg.dec                    |  7
> +++++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12
> +++++++++++
> >  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
> > ++++++++++++++++++++++-
> >  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12
> +++++++++++
> >  10 files changed, 84 insertions(+), 14 deletions(-)
> >
> > --
> > 2.11.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org<mailto: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
> _______________________________________________
> 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


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-08-29 15:16       ` Johnson, Brian (EXL - Eagan)
  2017-08-29 16:02         ` Kinney, Michael D
@ 2017-08-30 14:55         ` Yao, Jiewen
  2017-08-30 16:27           ` Andrew Fish
  1 sibling, 1 reply; 17+ messages in thread
From: Yao, Jiewen @ 2017-08-30 14:55 UTC (permalink / raw)
  To: Johnson, Brian (EXL - Eagan), Wang, Jian J,
	edk2-devel@lists.01.org

Hi Brian
Good feedback.
Comment in line.

From: Johnson, Brian (EXL - Eagan) [mailto:brian.johnson@hpe.com]
Sent: Tuesday, August 29, 2017 11:17 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Thank you for implementing this feature!  It is very  helpful for catching pointer-related problems.  We have used a similar scheme on our systems for years, and caught several important bugs.  Some comments:

* It's possible to implement similar protections in PEI (IA32) by modifying the GDT descriptors.
[Jiewen] Enabling PEI is interesting.
Since we are working on DXE and SMM as our first priority, can we treat the PEI enabling as a separate task?
If you can help to create a patch for PEI and submit, that will be even better.



* For flexibility, I'd like NULL pointer protection to be controlled independently in PEI, DXE, and SMM, using separate PCDs.
[Jiewen] I think we can existing example on using one PCD to control all phase or using one PCD to control different phase.
For example, we use below PCD to control all phases, PEI/DXE/SMM
  ## The mask is used to control PerformanceLib behavior.<BR><BR>
  #  BIT0 - Enable Performance Measurement.<BR>
  # @Prompt Performance Measurement Property.
  # @Expression  0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask & 0xFE) == 0
  gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask|0|UINT8|0x00000009

And we use below PCD to control different phase, DXE/SMM
  ## The mask is used to control memory profile behavior.<BR><BR>
  #  BIT0 - Enable UEFI memory profile.<BR>
  #  BIT1 - Enable SMRAM profile.<BR>
  #  BIT7 - Disable recording at the start.<BR>
  # @Prompt Memory Profile Property.
  # @Expression  0x80000002 | (gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask & 0x7C) == 0
  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask|0x0|UINT8|0x30001041

In order to make the code consistent with the existing PCD, I am thinking to use one PCD named PcdNullPointerDetectionPropertyMask
  #  BIT0 - Enable NULL pointer detection for UEFI.<BR>
  #  BIT1 - Enable NULL pointer detection for SMM.<BR>
  #  BIT2 - Enable NULL pointer detection for PEI.<BR>
  #  BIT7 - Disable NULL pointer detection after EndOfDxe.<BR>

I am not so worried about pre-memory initialization PEI phase, because page 0 is always invalid.
For post-memory initialization PEI phase, we can modify the page table in memory, and dynamic PCD can be used at that time.


* We have seen various option ROMs and OS boot loaders which have NULL pointer issues, but are outside of our control.  It is useful to enable NULL pointer protection during as much of the boot as possible, but disable it before running these other executables.  So I'd suggest adding another PCD, perhaps PcdNullPointerDetectionPostDxe, to control NULL pointer protection late in boot.  If PcdNullPointerDetection != PcdNullPointerDetectionPostDxe, install an end-of-DXE event (gEfiEndOfDxeEventGroupGuid) which changes the protection of page 0 using a call to EFI_CPU_ARCH_PROTOCOL. SetMemoryAttributes(CpuArch, 0, EFI_PAGE_SIZE, 0).
[Jiewen] Good point for the compatibility consideration.
Another thing is that CSM module may need access address 0. We have 2 choices:
1) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0, access page 0, then disable page 0.
2) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0 at the beginning, and always keep it enabled.
Any though on that?


So ideally I'd like to have 4 PCDs:

PcdNullPointerDetectionPei
PcdNullPointerDetectionDxe
PcdNullPointerDetectionSmm
PcdNullPointerDetectionPostDxe

Thanks,
Brian Johnson
HPE

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Sunday, August 27, 2017 10:39 PM
To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Comment in line.

From: Wang, Jian J
Sent: Monday, August 28, 2017 11:24 AM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature


1)      I think this feature should be 'FALSE' by default. I forgot to reset its default value. This feature makes use of page mechanism to detect NULL pointer. So any ARCH doesn't support paging in EDK-II can't use it. Currently validated platform includes VLV2 and Denlow. Let me know if all platform must be validated or not.

[Jiewen] Yes, I am OK to set to be FALSE to provide best compatibility.
I suggest we validate all open source IA platforms, such as Quark, and OVMF with TRUE.


2)      It's hard to make it a dynamic feature because we need to setup page table for physical address 0-4095 in advance. If there's no memory alloc/free action after enabling this feature, there's no chance to make those change in page table. Then the usage of feature will be limited in such case.

[Jiewen] Dynamic means the initial value is dynamic. I am not saying we need modify the page table.

An implementation could be:
A platform can set this PCD in PEI phase based upon a setup variable to choose CSM enable or disable.

The IPL sets page table based upon this PCD value. The DXE Core cannot consume PCD directly, because it might be dynamic. But we can pass the information from IPL via HOB. All the DXE module just checks the value based upon HOB.




From: Yao, Jiewen
Sent: Monday, August 28, 2017 11:10 AM
To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Thank you  to enable this feature.

I have 2 comments, after a very quick review.


1)      I notice it is enabled by default "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".

Would you please provide the information on how many open source platforms are validated?
Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
Or do we need validate any ARM platform?



2)      I am thinking about CSM platform. Do you think we can make it dynamic, as such, a platform may set the validate based upon CSM enable/disable?


Or if we need update the CSM module to patch the page table automatically? Once this is feature is ON.


Thank you
Yao Jiewen


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Monday, August 28, 2017 10:51 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> This patch is the implementation of NULL pointer detection feature,
> which is one of the small features of Special Pool.
>
> Wang, Jian J (2):
>   Implement NULL pointer detection for EDK-II Core
>   Implement NULL pointer detection for EDK-II SMM Core and driver
>
>  MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
>  MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
> ++++++++++++++++--------
>  MdeModulePkg/MdeModulePkg.dec                    |  7 +++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12 +++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
> ++++++++++++++++++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12 +++++++++++
>  10 files changed, 84 insertions(+), 14 deletions(-)
>
> --
> 2.11.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-08-30 14:55         ` Yao, Jiewen
@ 2017-08-30 16:27           ` Andrew Fish
  2017-08-30 21:36             ` Johnson, Brian (EXL - Eagan)
  2017-08-31  0:55             ` Yao, Jiewen
  0 siblings, 2 replies; 17+ messages in thread
From: Andrew Fish @ 2017-08-30 16:27 UTC (permalink / raw)
  To: Yao, Jiewen
  Cc: Johnson, Brian (EXL - Eagan), Wang, Jian J,
	edk2-devel@lists.01.org


> On Aug 30, 2017, at 7:55 AM, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> 
> Hi Brian
> Good feedback.
> Comment in line.
> 
> From: Johnson, Brian (EXL - Eagan) [mailto:brian.johnson@hpe.com]
> Sent: Tuesday, August 29, 2017 11:17 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
> 
> Thank you for implementing this feature!  It is very  helpful for catching pointer-related problems.  We have used a similar scheme on our systems for years, and caught several important bugs.  Some comments:
> 
> * It's possible to implement similar protections in PEI (IA32) by modifying the GDT descriptors.
> [Jiewen] Enabling PEI is interesting.
> Since we are working on DXE and SMM as our first priority, can we treat the PEI enabling as a separate task?
> If you can help to create a patch for PEI and submit, that will be even better.
> 

Jiewen,

This is slightly off topic but I've noticed in early PEI the issue on some x86 chipsets is not that a NULL pointer reference is bad, but any reference to an address space that is not decoded will triple bus fault the processor and that is really hard to debug. If the GDT trick could be used to cause an exception that drops into the debugger vs. a triple bus fault that would be a real win for debugging. While the memory ranges would be platform specific it would be awesome to have some plumbing in the edk2 to make this easier to implement. 

Thanks,

Andrew Fish

> 
> 
> * For flexibility, I'd like NULL pointer protection to be controlled independently in PEI, DXE, and SMM, using separate PCDs.
> [Jiewen] I think we can existing example on using one PCD to control all phase or using one PCD to control different phase.
> For example, we use below PCD to control all phases, PEI/DXE/SMM
>  ## The mask is used to control PerformanceLib behavior.<BR><BR>
>  #  BIT0 - Enable Performance Measurement.<BR>
>  # @Prompt Performance Measurement Property.
>  # @Expression  0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask & 0xFE) == 0
>  gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask|0|UINT8|0x00000009
> 
> And we use below PCD to control different phase, DXE/SMM
>  ## The mask is used to control memory profile behavior.<BR><BR>
>  #  BIT0 - Enable UEFI memory profile.<BR>
>  #  BIT1 - Enable SMRAM profile.<BR>
>  #  BIT7 - Disable recording at the start.<BR>
>  # @Prompt Memory Profile Property.
>  # @Expression  0x80000002 | (gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask & 0x7C) == 0
>  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask|0x0|UINT8|0x30001041
> 
> In order to make the code consistent with the existing PCD, I am thinking to use one PCD named PcdNullPointerDetectionPropertyMask
>  #  BIT0 - Enable NULL pointer detection for UEFI.<BR>
>  #  BIT1 - Enable NULL pointer detection for SMM.<BR>
>  #  BIT2 - Enable NULL pointer detection for PEI.<BR>
>  #  BIT7 - Disable NULL pointer detection after EndOfDxe.<BR>
> 
> I am not so worried about pre-memory initialization PEI phase, because page 0 is always invalid.
> For post-memory initialization PEI phase, we can modify the page table in memory, and dynamic PCD can be used at that time.
> 
> 
> * We have seen various option ROMs and OS boot loaders which have NULL pointer issues, but are outside of our control.  It is useful to enable NULL pointer protection during as much of the boot as possible, but disable it before running these other executables.  So I'd suggest adding another PCD, perhaps PcdNullPointerDetectionPostDxe, to control NULL pointer protection late in boot.  If PcdNullPointerDetection != PcdNullPointerDetectionPostDxe, install an end-of-DXE event (gEfiEndOfDxeEventGroupGuid) which changes the protection of page 0 using a call to EFI_CPU_ARCH_PROTOCOL. SetMemoryAttributes(CpuArch, 0, EFI_PAGE_SIZE, 0).
> [Jiewen] Good point for the compatibility consideration.
> Another thing is that CSM module may need access address 0. We have 2 choices:
> 1) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0, access page 0, then disable page 0.
> 2) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0 at the beginning, and always keep it enabled.
> Any though on that?
> 
> 
> So ideally I'd like to have 4 PCDs:
> 
> PcdNullPointerDetectionPei
> PcdNullPointerDetectionDxe
> PcdNullPointerDetectionSmm
> PcdNullPointerDetectionPostDxe
> 
> Thanks,
> Brian Johnson
> HPE
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
> Sent: Sunday, August 27, 2017 10:39 PM
> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
> 
> Comment in line.
> 
> From: Wang, Jian J
> Sent: Monday, August 28, 2017 11:24 AM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
> 
> 
> 1)      I think this feature should be 'FALSE' by default. I forgot to reset its default value. This feature makes use of page mechanism to detect NULL pointer. So any ARCH doesn't support paging in EDK-II can't use it. Currently validated platform includes VLV2 and Denlow. Let me know if all platform must be validated or not.
> 
> [Jiewen] Yes, I am OK to set to be FALSE to provide best compatibility.
> I suggest we validate all open source IA platforms, such as Quark, and OVMF with TRUE.
> 
> 
> 2)      It's hard to make it a dynamic feature because we need to setup page table for physical address 0-4095 in advance. If there's no memory alloc/free action after enabling this feature, there's no chance to make those change in page table. Then the usage of feature will be limited in such case.
> 
> [Jiewen] Dynamic means the initial value is dynamic. I am not saying we need modify the page table.
> 
> An implementation could be:
> A platform can set this PCD in PEI phase based upon a setup variable to choose CSM enable or disable.
> 
> The IPL sets page table based upon this PCD value. The DXE Core cannot consume PCD directly, because it might be dynamic. But we can pass the information from IPL via HOB. All the DXE module just checks the value based upon HOB.
> 
> 
> 
> 
> From: Yao, Jiewen
> Sent: Monday, August 28, 2017 11:10 AM
> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
> 
> Thank you  to enable this feature.
> 
> I have 2 comments, after a very quick review.
> 
> 
> 1)      I notice it is enabled by default "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".
> 
> Would you please provide the information on how many open source platforms are validated?
> Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
> Or do we need validate any ARM platform?
> 
> 
> 
> 2)      I am thinking about CSM platform. Do you think we can make it dynamic, as such, a platform may set the validate based upon CSM enable/disable?
> 
> 
> Or if we need update the CSM module to patch the page table automatically? Once this is feature is ON.
> 
> 
> Thank you
> Yao Jiewen
> 
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
>> Jian J
>> Sent: Monday, August 28, 2017 10:51 AM
>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
>> Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>> 
>> This patch is the implementation of NULL pointer detection feature,
>> which is one of the small features of Special Pool.
>> 
>> Wang, Jian J (2):
>>  Implement NULL pointer detection for EDK-II Core
>>  Implement NULL pointer detection for EDK-II SMM Core and driver
>> 
>> MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
>> MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
>> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>> MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
>> ++++++++++++++++--------
>> MdeModulePkg/MdeModulePkg.dec                    |  7 +++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12 +++++++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
>> ++++++++++++++++++++++-
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12 +++++++++++
>> 10 files changed, 84 insertions(+), 14 deletions(-)
>> 
>> --
>> 2.11.0.windows.1
>> 
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto: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



^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-08-30 16:27           ` Andrew Fish
@ 2017-08-30 21:36             ` Johnson, Brian (EXL - Eagan)
  2017-08-31  1:16               ` Yao, Jiewen
  2017-08-31  0:55             ` Yao, Jiewen
  1 sibling, 1 reply; 17+ messages in thread
From: Johnson, Brian (EXL - Eagan) @ 2017-08-30 21:36 UTC (permalink / raw)
  To: afish@apple.com, Yao, Jiewen; +Cc: Wang, Jian J, edk2-devel@lists.01.org

Jiewen,

Certainly PEI could be done later.  There's no need to get all the code in at once, and you have the DXE/SMM part pretty much ready to go.

I'm willing to go with a single PCD with a bitmap, or multiple PCDs.  Whatever seems best.

IIRC protecting page 0 in PEI involves modifying the main GCD descriptor to be a grows-downward variety, with a limit above page 0.  That way you get an exception if page 0 is accessed.  I'd have to check on the steps needed to release the actual code, which may take quite a while.  You may be better off just doing it yourself.  I'll take another look at the code to make sure I'm explaining this properly, though.

CSM is one of the few pieces of BIOS which has a legitimate need to access page 0.  We get the best protection by having it enable, access, and then disable page 0.  But if that affects performance too badly, we may need to have it leave page 0 enabled.  I don't enable CSM on the platforms I work on, so it's not something I have much to say about.  Those who use CSM would want to weigh in.

Brian Johnson

-----Original Message-----
From: afish@apple.com [mailto:afish@apple.com] 
Sent: Wednesday, August 30, 2017 11:27 AM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: Johnson, Brian (EXL - Eagan) <brian.johnson@hpe.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature


> On Aug 30, 2017, at 7:55 AM, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> 
> Hi Brian
> Good feedback.
> Comment in line.
> 
> From: Johnson, Brian (EXL - Eagan) [mailto:brian.johnson@hpe.com]
> Sent: Tuesday, August 29, 2017 11:17 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
> 
> Thank you for implementing this feature!  It is very  helpful for catching pointer-related problems.  We have used a similar scheme on our systems for years, and caught several important bugs.  Some comments:
> 
> * It's possible to implement similar protections in PEI (IA32) by modifying the GDT descriptors.
> [Jiewen] Enabling PEI is interesting.
> Since we are working on DXE and SMM as our first priority, can we treat the PEI enabling as a separate task?
> If you can help to create a patch for PEI and submit, that will be even better.
> 

Jiewen,

This is slightly off topic but I've noticed in early PEI the issue on some x86 chipsets is not that a NULL pointer reference is bad, but any reference to an address space that is not decoded will triple bus fault the processor and that is really hard to debug. If the GDT trick could be used to cause an exception that drops into the debugger vs. a triple bus fault that would be a real win for debugging. While the memory ranges would be platform specific it would be awesome to have some plumbing in the edk2 to make this easier to implement. 

Thanks,

Andrew Fish

> 
> 
> * For flexibility, I'd like NULL pointer protection to be controlled independently in PEI, DXE, and SMM, using separate PCDs.
> [Jiewen] I think we can existing example on using one PCD to control all phase or using one PCD to control different phase.
> For example, we use below PCD to control all phases, PEI/DXE/SMM
>  ## The mask is used to control PerformanceLib behavior.<BR><BR>
>  #  BIT0 - Enable Performance Measurement.<BR>
>  # @Prompt Performance Measurement Property.
>  # @Expression  0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask & 0xFE) == 0
>  gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask|0|UINT8|0x00000009
> 
> And we use below PCD to control different phase, DXE/SMM
>  ## The mask is used to control memory profile behavior.<BR><BR>
>  #  BIT0 - Enable UEFI memory profile.<BR>
>  #  BIT1 - Enable SMRAM profile.<BR>
>  #  BIT7 - Disable recording at the start.<BR>
>  # @Prompt Memory Profile Property.
>  # @Expression  0x80000002 | (gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask & 0x7C) == 0
>  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask|0x0|UINT8|0x30001041
> 
> In order to make the code consistent with the existing PCD, I am thinking to use one PCD named PcdNullPointerDetectionPropertyMask
>  #  BIT0 - Enable NULL pointer detection for UEFI.<BR>
>  #  BIT1 - Enable NULL pointer detection for SMM.<BR>
>  #  BIT2 - Enable NULL pointer detection for PEI.<BR>
>  #  BIT7 - Disable NULL pointer detection after EndOfDxe.<BR>
> 
> I am not so worried about pre-memory initialization PEI phase, because page 0 is always invalid.
> For post-memory initialization PEI phase, we can modify the page table in memory, and dynamic PCD can be used at that time.
> 
> 
> * We have seen various option ROMs and OS boot loaders which have NULL pointer issues, but are outside of our control.  It is useful to enable NULL pointer protection during as much of the boot as possible, but disable it before running these other executables.  So I'd suggest adding another PCD, perhaps PcdNullPointerDetectionPostDxe, to control NULL pointer protection late in boot.  If PcdNullPointerDetection != PcdNullPointerDetectionPostDxe, install an end-of-DXE event (gEfiEndOfDxeEventGroupGuid) which changes the protection of page 0 using a call to EFI_CPU_ARCH_PROTOCOL. SetMemoryAttributes(CpuArch, 0, EFI_PAGE_SIZE, 0).
> [Jiewen] Good point for the compatibility consideration.
> Another thing is that CSM module may need access address 0. We have 2 choices:
> 1) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0, access page 0, then disable page 0.
> 2) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0 at the beginning, and always keep it enabled.
> Any though on that?
> 
> 
> So ideally I'd like to have 4 PCDs:
> 
> PcdNullPointerDetectionPei
> PcdNullPointerDetectionDxe
> PcdNullPointerDetectionSmm
> PcdNullPointerDetectionPostDxe
> 
> Thanks,
> Brian Johnson
> HPE
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
> Sent: Sunday, August 27, 2017 10:39 PM
> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
> 
> Comment in line.
> 
> From: Wang, Jian J
> Sent: Monday, August 28, 2017 11:24 AM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
> 
> 
> 1)      I think this feature should be 'FALSE' by default. I forgot to reset its default value. This feature makes use of page mechanism to detect NULL pointer. So any ARCH doesn't support paging in EDK-II can't use it. Currently validated platform includes VLV2 and Denlow. Let me know if all platform must be validated or not.
> 
> [Jiewen] Yes, I am OK to set to be FALSE to provide best compatibility.
> I suggest we validate all open source IA platforms, such as Quark, and OVMF with TRUE.
> 
> 
> 2)      It's hard to make it a dynamic feature because we need to setup page table for physical address 0-4095 in advance. If there's no memory alloc/free action after enabling this feature, there's no chance to make those change in page table. Then the usage of feature will be limited in such case.
> 
> [Jiewen] Dynamic means the initial value is dynamic. I am not saying we need modify the page table.
> 
> An implementation could be:
> A platform can set this PCD in PEI phase based upon a setup variable to choose CSM enable or disable.
> 
> The IPL sets page table based upon this PCD value. The DXE Core cannot consume PCD directly, because it might be dynamic. But we can pass the information from IPL via HOB. All the DXE module just checks the value based upon HOB.
> 
> 
> 
> 
> From: Yao, Jiewen
> Sent: Monday, August 28, 2017 11:10 AM
> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
> 
> Thank you  to enable this feature.
> 
> I have 2 comments, after a very quick review.
> 
> 
> 1)      I notice it is enabled by default "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".
> 
> Would you please provide the information on how many open source platforms are validated?
> Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
> Or do we need validate any ARM platform?
> 
> 
> 
> 2)      I am thinking about CSM platform. Do you think we can make it dynamic, as such, a platform may set the validate based upon CSM enable/disable?
> 
> 
> Or if we need update the CSM module to patch the page table automatically? Once this is feature is ON.
> 
> 
> Thank you
> Yao Jiewen
> 
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
>> Jian J
>> Sent: Monday, August 28, 2017 10:51 AM
>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
>> Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>> 
>> This patch is the implementation of NULL pointer detection feature,
>> which is one of the small features of Special Pool.
>> 
>> Wang, Jian J (2):
>>  Implement NULL pointer detection for EDK-II Core
>>  Implement NULL pointer detection for EDK-II SMM Core and driver
>> 
>> MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
>> MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
>> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>> MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
>> ++++++++++++++++--------
>> MdeModulePkg/MdeModulePkg.dec                    |  7 +++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12 +++++++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
>> ++++++++++++++++++++++-
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12 +++++++++++
>> 10 files changed, 84 insertions(+), 14 deletions(-)
>> 
>> --
>> 2.11.0.windows.1
>> 
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto: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



^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-08-30 16:27           ` Andrew Fish
  2017-08-30 21:36             ` Johnson, Brian (EXL - Eagan)
@ 2017-08-31  0:55             ` Yao, Jiewen
  1 sibling, 0 replies; 17+ messages in thread
From: Yao, Jiewen @ 2017-08-31  0:55 UTC (permalink / raw)
  To: Andrew Fish; +Cc: edk2-devel@lists.01.org

Hi Andrew
You raised a good point.

I have seen a system reset when the below 1MiB address is access before memory initialization. I think NULL pointer access before MRC should never happen in a production BIOS.

The debug ability is a big problem. Fully agree.

I think we may extend that topic to separate PEI to pre-mem and post-mem by using different strategy.

I will file a bugzillar to record this.

Thank you
Yao Jiewen


From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Andrew Fish
Sent: Thursday, August 31, 2017 12:27 AM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature


> On Aug 30, 2017, at 7:55 AM, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
>
> Hi Brian
> Good feedback.
> Comment in line.
>
> From: Johnson, Brian (EXL - Eagan) [mailto:brian.johnson@hpe.com]
> Sent: Tuesday, August 29, 2017 11:17 PM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> Thank you for implementing this feature!  It is very  helpful for catching pointer-related problems.  We have used a similar scheme on our systems for years, and caught several important bugs.  Some comments:
>
> * It's possible to implement similar protections in PEI (IA32) by modifying the GDT descriptors.
> [Jiewen] Enabling PEI is interesting.
> Since we are working on DXE and SMM as our first priority, can we treat the PEI enabling as a separate task?
> If you can help to create a patch for PEI and submit, that will be even better.
>

Jiewen,

This is slightly off topic but I've noticed in early PEI the issue on some x86 chipsets is not that a NULL pointer reference is bad, but any reference to an address space that is not decoded will triple bus fault the processor and that is really hard to debug. If the GDT trick could be used to cause an exception that drops into the debugger vs. a triple bus fault that would be a real win for debugging. While the memory ranges would be platform specific it would be awesome to have some plumbing in the edk2 to make this easier to implement.

Thanks,

Andrew Fish

>
>
> * For flexibility, I'd like NULL pointer protection to be controlled independently in PEI, DXE, and SMM, using separate PCDs.
> [Jiewen] I think we can existing example on using one PCD to control all phase or using one PCD to control different phase.
> For example, we use below PCD to control all phases, PEI/DXE/SMM
>  ## The mask is used to control PerformanceLib behavior.<BR><BR>
>  #  BIT0 - Enable Performance Measurement.<BR>
>  # @Prompt Performance Measurement Property.
>  # @Expression  0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask & 0xFE) == 0
>  gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask|0|UINT8|0x00000009
>
> And we use below PCD to control different phase, DXE/SMM
>  ## The mask is used to control memory profile behavior.<BR><BR>
>  #  BIT0 - Enable UEFI memory profile.<BR>
>  #  BIT1 - Enable SMRAM profile.<BR>
>  #  BIT7 - Disable recording at the start.<BR>
>  # @Prompt Memory Profile Property.
>  # @Expression  0x80000002 | (gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask & 0x7C) == 0
>  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask|0x0|UINT8|0x30001041
>
> In order to make the code consistent with the existing PCD, I am thinking to use one PCD named PcdNullPointerDetectionPropertyMask
>  #  BIT0 - Enable NULL pointer detection for UEFI.<BR>
>  #  BIT1 - Enable NULL pointer detection for SMM.<BR>
>  #  BIT2 - Enable NULL pointer detection for PEI.<BR>
>  #  BIT7 - Disable NULL pointer detection after EndOfDxe.<BR>
>
> I am not so worried about pre-memory initialization PEI phase, because page 0 is always invalid.
> For post-memory initialization PEI phase, we can modify the page table in memory, and dynamic PCD can be used at that time.
>
>
> * We have seen various option ROMs and OS boot loaders which have NULL pointer issues, but are outside of our control.  It is useful to enable NULL pointer protection during as much of the boot as possible, but disable it before running these other executables.  So I'd suggest adding another PCD, perhaps PcdNullPointerDetectionPostDxe, to control NULL pointer protection late in boot.  If PcdNullPointerDetection != PcdNullPointerDetectionPostDxe, install an end-of-DXE event (gEfiEndOfDxeEventGroupGuid) which changes the protection of page 0 using a call to EFI_CPU_ARCH_PROTOCOL. SetMemoryAttributes(CpuArch, 0, EFI_PAGE_SIZE, 0).
> [Jiewen] Good point for the compatibility consideration.
> Another thing is that CSM module may need access address 0. We have 2 choices:
> 1) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0, access page 0, then disable page 0.
> 2) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0 at the beginning, and always keep it enabled.
> Any though on that?
>
>
> So ideally I'd like to have 4 PCDs:
>
> PcdNullPointerDetectionPei
> PcdNullPointerDetectionDxe
> PcdNullPointerDetectionSmm
> PcdNullPointerDetectionPostDxe
>
> Thanks,
> Brian Johnson
> HPE
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
> Sent: Sunday, August 27, 2017 10:39 PM
> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> Comment in line.
>
> From: Wang, Jian J
> Sent: Monday, August 28, 2017 11:24 AM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
>
> 1)      I think this feature should be 'FALSE' by default. I forgot to reset its default value. This feature makes use of page mechanism to detect NULL pointer. So any ARCH doesn't support paging in EDK-II can't use it. Currently validated platform includes VLV2 and Denlow. Let me know if all platform must be validated or not.
>
> [Jiewen] Yes, I am OK to set to be FALSE to provide best compatibility.
> I suggest we validate all open source IA platforms, such as Quark, and OVMF with TRUE.
>
>
> 2)      It's hard to make it a dynamic feature because we need to setup page table for physical address 0-4095 in advance. If there's no memory alloc/free action after enabling this feature, there's no chance to make those change in page table. Then the usage of feature will be limited in such case.
>
> [Jiewen] Dynamic means the initial value is dynamic. I am not saying we need modify the page table.
>
> An implementation could be:
> A platform can set this PCD in PEI phase based upon a setup variable to choose CSM enable or disable.
>
> The IPL sets page table based upon this PCD value. The DXE Core cannot consume PCD directly, because it might be dynamic. But we can pass the information from IPL via HOB. All the DXE module just checks the value based upon HOB.
>
>
>
>
> From: Yao, Jiewen
> Sent: Monday, August 28, 2017 11:10 AM
> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com>>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> Thank you  to enable this feature.
>
> I have 2 comments, after a very quick review.
>
>
> 1)      I notice it is enabled by default "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".
>
> Would you please provide the information on how many open source platforms are validated?
> Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
> Or do we need validate any ARM platform?
>
>
>
> 2)      I am thinking about CSM platform. Do you think we can make it dynamic, as such, a platform may set the validate based upon CSM enable/disable?
>
>
> Or if we need update the CSM module to patch the page table automatically? Once this is feature is ON.
>
>
> Thank you
> Yao Jiewen
>
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
>> Jian J
>> Sent: Monday, August 28, 2017 10:51 AM
>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
>> Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>>
>> This patch is the implementation of NULL pointer detection feature,
>> which is one of the small features of Special Pool.
>>
>> Wang, Jian J (2):
>>  Implement NULL pointer detection for EDK-II Core
>>  Implement NULL pointer detection for EDK-II SMM Core and driver
>>
>> MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
>> MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
>> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>> MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
>> ++++++++++++++++--------
>> MdeModulePkg/MdeModulePkg.dec                    |  7 +++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12 +++++++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
>> ++++++++++++++++++++++-
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12 +++++++++++
>> 10 files changed, 84 insertions(+), 14 deletions(-)
>>
>> --
>> 2.11.0.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel

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


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-08-30 21:36             ` Johnson, Brian (EXL - Eagan)
@ 2017-08-31  1:16               ` Yao, Jiewen
  2017-09-01  2:27                 ` Wang, Jian J
  0 siblings, 1 reply; 17+ messages in thread
From: Yao, Jiewen @ 2017-08-31  1:16 UTC (permalink / raw)
  To: Johnson, Brian (EXL - Eagan), afish@apple.com; +Cc: edk2-devel@lists.01.org

Thanks.
I have filed https://bugzilla.tianocore.org/show_bug.cgi?id=687 for PEI phase, with suggestion from Brian Johnson and Andrew Fish.

If you share some of your PEI code to elimination the duplicated effort, that would be great.

Yes, we can calculate the perf data in CSM to see what is the best way to enable this feature. Good suggestion.

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Johnson, Brian (EXL - Eagan)
Sent: Thursday, August 31, 2017 5:36 AM
To: afish@apple.com; Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Jiewen,

Certainly PEI could be done later.  There's no need to get all the code in at once, and you have the DXE/SMM part pretty much ready to go.

I'm willing to go with a single PCD with a bitmap, or multiple PCDs.  Whatever seems best.

IIRC protecting page 0 in PEI involves modifying the main GCD descriptor to be a grows-downward variety, with a limit above page 0.  That way you get an exception if page 0 is accessed.  I'd have to check on the steps needed to release the actual code, which may take quite a while.  You may be better off just doing it yourself.  I'll take another look at the code to make sure I'm explaining this properly, though.

CSM is one of the few pieces of BIOS which has a legitimate need to access page 0.  We get the best protection by having it enable, access, and then disable page 0.  But if that affects performance too badly, we may need to have it leave page 0 enabled.  I don't enable CSM on the platforms I work on, so it's not something I have much to say about.  Those who use CSM would want to weigh in.

Brian Johnson

-----Original Message-----
From: afish@apple.com<mailto:afish@apple.com> [mailto:afish@apple.com]
Sent: Wednesday, August 30, 2017 11:27 AM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Cc: Johnson, Brian (EXL - Eagan) <brian.johnson@hpe.com<mailto:brian.johnson@hpe.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature


> On Aug 30, 2017, at 7:55 AM, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
>
> Hi Brian
> Good feedback.
> Comment in line.
>
> From: Johnson, Brian (EXL - Eagan) [mailto:brian.johnson@hpe.com]
> Sent: Tuesday, August 29, 2017 11:17 PM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> Thank you for implementing this feature!  It is very  helpful for catching pointer-related problems.  We have used a similar scheme on our systems for years, and caught several important bugs.  Some comments:
>
> * It's possible to implement similar protections in PEI (IA32) by modifying the GDT descriptors.
> [Jiewen] Enabling PEI is interesting.
> Since we are working on DXE and SMM as our first priority, can we treat the PEI enabling as a separate task?
> If you can help to create a patch for PEI and submit, that will be even better.
>

Jiewen,

This is slightly off topic but I've noticed in early PEI the issue on some x86 chipsets is not that a NULL pointer reference is bad, but any reference to an address space that is not decoded will triple bus fault the processor and that is really hard to debug. If the GDT trick could be used to cause an exception that drops into the debugger vs. a triple bus fault that would be a real win for debugging. While the memory ranges would be platform specific it would be awesome to have some plumbing in the edk2 to make this easier to implement.

Thanks,

Andrew Fish

>
>
> * For flexibility, I'd like NULL pointer protection to be controlled independently in PEI, DXE, and SMM, using separate PCDs.
> [Jiewen] I think we can existing example on using one PCD to control all phase or using one PCD to control different phase.
> For example, we use below PCD to control all phases, PEI/DXE/SMM
>  ## The mask is used to control PerformanceLib behavior.<BR><BR>
>  #  BIT0 - Enable Performance Measurement.<BR>
>  # @Prompt Performance Measurement Property.
>  # @Expression  0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask & 0xFE) == 0
>  gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask|0|UINT8|0x00000009
>
> And we use below PCD to control different phase, DXE/SMM
>  ## The mask is used to control memory profile behavior.<BR><BR>
>  #  BIT0 - Enable UEFI memory profile.<BR>
>  #  BIT1 - Enable SMRAM profile.<BR>
>  #  BIT7 - Disable recording at the start.<BR>
>  # @Prompt Memory Profile Property.
>  # @Expression  0x80000002 | (gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask & 0x7C) == 0
>  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask|0x0|UINT8|0x30001041
>
> In order to make the code consistent with the existing PCD, I am thinking to use one PCD named PcdNullPointerDetectionPropertyMask
>  #  BIT0 - Enable NULL pointer detection for UEFI.<BR>
>  #  BIT1 - Enable NULL pointer detection for SMM.<BR>
>  #  BIT2 - Enable NULL pointer detection for PEI.<BR>
>  #  BIT7 - Disable NULL pointer detection after EndOfDxe.<BR>
>
> I am not so worried about pre-memory initialization PEI phase, because page 0 is always invalid.
> For post-memory initialization PEI phase, we can modify the page table in memory, and dynamic PCD can be used at that time.
>
>
> * We have seen various option ROMs and OS boot loaders which have NULL pointer issues, but are outside of our control.  It is useful to enable NULL pointer protection during as much of the boot as possible, but disable it before running these other executables.  So I'd suggest adding another PCD, perhaps PcdNullPointerDetectionPostDxe, to control NULL pointer protection late in boot.  If PcdNullPointerDetection != PcdNullPointerDetectionPostDxe, install an end-of-DXE event (gEfiEndOfDxeEventGroupGuid) which changes the protection of page 0 using a call to EFI_CPU_ARCH_PROTOCOL. SetMemoryAttributes(CpuArch, 0, EFI_PAGE_SIZE, 0).
> [Jiewen] Good point for the compatibility consideration.
> Another thing is that CSM module may need access address 0. We have 2 choices:
> 1) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0, access page 0, then disable page 0.
> 2) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0 at the beginning, and always keep it enabled.
> Any though on that?
>
>
> So ideally I'd like to have 4 PCDs:
>
> PcdNullPointerDetectionPei
> PcdNullPointerDetectionDxe
> PcdNullPointerDetectionSmm
> PcdNullPointerDetectionPostDxe
>
> Thanks,
> Brian Johnson
> HPE
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
> Sent: Sunday, August 27, 2017 10:39 PM
> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> Comment in line.
>
> From: Wang, Jian J
> Sent: Monday, August 28, 2017 11:24 AM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
>
> 1)      I think this feature should be 'FALSE' by default. I forgot to reset its default value. This feature makes use of page mechanism to detect NULL pointer. So any ARCH doesn't support paging in EDK-II can't use it. Currently validated platform includes VLV2 and Denlow. Let me know if all platform must be validated or not.
>
> [Jiewen] Yes, I am OK to set to be FALSE to provide best compatibility.
> I suggest we validate all open source IA platforms, such as Quark, and OVMF with TRUE.
>
>
> 2)      It's hard to make it a dynamic feature because we need to setup page table for physical address 0-4095 in advance. If there's no memory alloc/free action after enabling this feature, there's no chance to make those change in page table. Then the usage of feature will be limited in such case.
>
> [Jiewen] Dynamic means the initial value is dynamic. I am not saying we need modify the page table.
>
> An implementation could be:
> A platform can set this PCD in PEI phase based upon a setup variable to choose CSM enable or disable.
>
> The IPL sets page table based upon this PCD value. The DXE Core cannot consume PCD directly, because it might be dynamic. But we can pass the information from IPL via HOB. All the DXE module just checks the value based upon HOB.
>
>
>
>
> From: Yao, Jiewen
> Sent: Monday, August 28, 2017 11:10 AM
> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com>>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> Thank you  to enable this feature.
>
> I have 2 comments, after a very quick review.
>
>
> 1)      I notice it is enabled by default "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".
>
> Would you please provide the information on how many open source platforms are validated?
> Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
> Or do we need validate any ARM platform?
>
>
>
> 2)      I am thinking about CSM platform. Do you think we can make it dynamic, as such, a platform may set the validate based upon CSM enable/disable?
>
>
> Or if we need update the CSM module to patch the page table automatically? Once this is feature is ON.
>
>
> Thank you
> Yao Jiewen
>
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
>> Jian J
>> Sent: Monday, August 28, 2017 10:51 AM
>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
>> Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>>
>> This patch is the implementation of NULL pointer detection feature,
>> which is one of the small features of Special Pool.
>>
>> Wang, Jian J (2):
>>  Implement NULL pointer detection for EDK-II Core
>>  Implement NULL pointer detection for EDK-II SMM Core and driver
>>
>> MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
>> MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
>> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>> MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
>> ++++++++++++++++--------
>> MdeModulePkg/MdeModulePkg.dec                    |  7 +++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12 +++++++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
>> ++++++++++++++++++++++-
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12 +++++++++++
>> 10 files changed, 84 insertions(+), 14 deletions(-)
>>
>> --
>> 2.11.0.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel

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


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-08-31  1:16               ` Yao, Jiewen
@ 2017-09-01  2:27                 ` Wang, Jian J
  2017-09-01  5:22                   ` Wang, Jian J
  0 siblings, 1 reply; 17+ messages in thread
From: Wang, Jian J @ 2017-09-01  2:27 UTC (permalink / raw)
  To: Yao, Jiewen, Johnson, Brian (EXL - Eagan), afish@apple.com
  Cc: edk2-devel@lists.01.org

Let me summarize the changes to this feature. If no new comments, I'll start to make changes to current implementation.

a) Use one PCD with bit set/clear to enable/disable this feature for different boot phases. To make it easier to use, I'd suggest to use the bit sequence to follow the boot phase sequence (bit0->bit1->bit2......bit7 ===> PEI->DXE->PostDxe......SMM), just like below. SMM is a little bit special so it consumes the MSB.

     gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
     #  BIT0 - Enable NULL pointer detection for PEI.<BR>
     #  BIT1 - Enable NULL pointer detection for DXE.<BR>
     #  BIT2 - Enable NULL pointer detection after EndOfDxe.<BR>
     #  BIT7 - Disable NULL pointer detection for SMM.<BR>

    This PCD is a "fixed" PCD and all bits are cleared by default. Since this is a feature for debug purpose, I see no reason to make it dynamic. 

b) Add PEI and post DXE phase support. Since PEI phase requirement has been covered by following new bug tracker, it won't be included by current patch update but a separate one. Changes for Post DXE phase will be included.

    https://bugzilla.tianocore.org/show_bug.cgi?id=687

Thanks,
Wang, Jian J

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Thursday, August 31, 2017 9:16 AM
To: Johnson, Brian (EXL - Eagan) <brian.johnson@hpe.com>; afish@apple.com
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Thanks.
I have filed https://bugzilla.tianocore.org/show_bug.cgi?id=687 for PEI phase, with suggestion from Brian Johnson and Andrew Fish.

If you share some of your PEI code to elimination the duplicated effort, that would be great.

Yes, we can calculate the perf data in CSM to see what is the best way to enable this feature. Good suggestion.

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Johnson, Brian (EXL - Eagan)
Sent: Thursday, August 31, 2017 5:36 AM
To: afish@apple.com; Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Jiewen,

Certainly PEI could be done later.  There's no need to get all the code in at once, and you have the DXE/SMM part pretty much ready to go.

I'm willing to go with a single PCD with a bitmap, or multiple PCDs.  Whatever seems best.

IIRC protecting page 0 in PEI involves modifying the main GCD descriptor to be a grows-downward variety, with a limit above page 0.  That way you get an exception if page 0 is accessed.  I'd have to check on the steps needed to release the actual code, which may take quite a while.  You may be better off just doing it yourself.  I'll take another look at the code to make sure I'm explaining this properly, though.

CSM is one of the few pieces of BIOS which has a legitimate need to access page 0.  We get the best protection by having it enable, access, and then disable page 0.  But if that affects performance too badly, we may need to have it leave page 0 enabled.  I don't enable CSM on the platforms I work on, so it's not something I have much to say about.  Those who use CSM would want to weigh in.

Brian Johnson

-----Original Message-----
From: afish@apple.com<mailto:afish@apple.com> [mailto:afish@apple.com]
Sent: Wednesday, August 30, 2017 11:27 AM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Cc: Johnson, Brian (EXL - Eagan) <brian.johnson@hpe.com<mailto:brian.johnson@hpe.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature


> On Aug 30, 2017, at 7:55 AM, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
>
> Hi Brian
> Good feedback.
> Comment in line.
>
> From: Johnson, Brian (EXL - Eagan) [mailto:brian.johnson@hpe.com]
> Sent: Tuesday, August 29, 2017 11:17 PM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> Thank you for implementing this feature!  It is very  helpful for catching pointer-related problems.  We have used a similar scheme on our systems for years, and caught several important bugs.  Some comments:
>
> * It's possible to implement similar protections in PEI (IA32) by modifying the GDT descriptors.
> [Jiewen] Enabling PEI is interesting.
> Since we are working on DXE and SMM as our first priority, can we treat the PEI enabling as a separate task?
> If you can help to create a patch for PEI and submit, that will be even better.
>

Jiewen,

This is slightly off topic but I've noticed in early PEI the issue on some x86 chipsets is not that a NULL pointer reference is bad, but any reference to an address space that is not decoded will triple bus fault the processor and that is really hard to debug. If the GDT trick could be used to cause an exception that drops into the debugger vs. a triple bus fault that would be a real win for debugging. While the memory ranges would be platform specific it would be awesome to have some plumbing in the edk2 to make this easier to implement.

Thanks,

Andrew Fish

>
>
> * For flexibility, I'd like NULL pointer protection to be controlled independently in PEI, DXE, and SMM, using separate PCDs.
> [Jiewen] I think we can existing example on using one PCD to control all phase or using one PCD to control different phase.
> For example, we use below PCD to control all phases, PEI/DXE/SMM
>  ## The mask is used to control PerformanceLib behavior.<BR><BR>
>  #  BIT0 - Enable Performance Measurement.<BR>
>  # @Prompt Performance Measurement Property.
>  # @Expression  0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask & 0xFE) == 0
>  gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask|0|UINT8|0x00000009
>
> And we use below PCD to control different phase, DXE/SMM
>  ## The mask is used to control memory profile behavior.<BR><BR>
>  #  BIT0 - Enable UEFI memory profile.<BR>
>  #  BIT1 - Enable SMRAM profile.<BR>
>  #  BIT7 - Disable recording at the start.<BR>
>  # @Prompt Memory Profile Property.
>  # @Expression  0x80000002 | (gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask & 0x7C) == 0
>  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask|0x0|UINT8|0x30001041
>
> In order to make the code consistent with the existing PCD, I am thinking to use one PCD named PcdNullPointerDetectionPropertyMask
>  #  BIT0 - Enable NULL pointer detection for UEFI.<BR>
>  #  BIT1 - Enable NULL pointer detection for SMM.<BR>
>  #  BIT2 - Enable NULL pointer detection for PEI.<BR>
>  #  BIT7 - Disable NULL pointer detection after EndOfDxe.<BR>
>
> I am not so worried about pre-memory initialization PEI phase, because page 0 is always invalid.
> For post-memory initialization PEI phase, we can modify the page table in memory, and dynamic PCD can be used at that time.
>
>
> * We have seen various option ROMs and OS boot loaders which have NULL pointer issues, but are outside of our control.  It is useful to enable NULL pointer protection during as much of the boot as possible, but disable it before running these other executables.  So I'd suggest adding another PCD, perhaps PcdNullPointerDetectionPostDxe, to control NULL pointer protection late in boot.  If PcdNullPointerDetection != PcdNullPointerDetectionPostDxe, install an end-of-DXE event (gEfiEndOfDxeEventGroupGuid) which changes the protection of page 0 using a call to EFI_CPU_ARCH_PROTOCOL. SetMemoryAttributes(CpuArch, 0, EFI_PAGE_SIZE, 0).
> [Jiewen] Good point for the compatibility consideration.
> Another thing is that CSM module may need access address 0. We have 2 choices:
> 1) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0, access page 0, then disable page 0.
> 2) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0 at the beginning, and always keep it enabled.
> Any though on that?
>
>
> So ideally I'd like to have 4 PCDs:
>
> PcdNullPointerDetectionPei
> PcdNullPointerDetectionDxe
> PcdNullPointerDetectionSmm
> PcdNullPointerDetectionPostDxe
>
> Thanks,
> Brian Johnson
> HPE
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
> Sent: Sunday, August 27, 2017 10:39 PM
> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> Comment in line.
>
> From: Wang, Jian J
> Sent: Monday, August 28, 2017 11:24 AM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
>
> 1)      I think this feature should be 'FALSE' by default. I forgot to reset its default value. This feature makes use of page mechanism to detect NULL pointer. So any ARCH doesn't support paging in EDK-II can't use it. Currently validated platform includes VLV2 and Denlow. Let me know if all platform must be validated or not.
>
> [Jiewen] Yes, I am OK to set to be FALSE to provide best compatibility.
> I suggest we validate all open source IA platforms, such as Quark, and OVMF with TRUE.
>
>
> 2)      It's hard to make it a dynamic feature because we need to setup page table for physical address 0-4095 in advance. If there's no memory alloc/free action after enabling this feature, there's no chance to make those change in page table. Then the usage of feature will be limited in such case.
>
> [Jiewen] Dynamic means the initial value is dynamic. I am not saying we need modify the page table.
>
> An implementation could be:
> A platform can set this PCD in PEI phase based upon a setup variable to choose CSM enable or disable.
>
> The IPL sets page table based upon this PCD value. The DXE Core cannot consume PCD directly, because it might be dynamic. But we can pass the information from IPL via HOB. All the DXE module just checks the value based upon HOB.
>
>
>
>
> From: Yao, Jiewen
> Sent: Monday, August 28, 2017 11:10 AM
> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com>>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> Thank you  to enable this feature.
>
> I have 2 comments, after a very quick review.
>
>
> 1)      I notice it is enabled by default "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".
>
> Would you please provide the information on how many open source platforms are validated?
> Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
> Or do we need validate any ARM platform?
>
>
>
> 2)      I am thinking about CSM platform. Do you think we can make it dynamic, as such, a platform may set the validate based upon CSM enable/disable?
>
>
> Or if we need update the CSM module to patch the page table automatically? Once this is feature is ON.
>
>
> Thank you
> Yao Jiewen
>
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
>> Jian J
>> Sent: Monday, August 28, 2017 10:51 AM
>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
>> Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>>
>> This patch is the implementation of NULL pointer detection feature,
>> which is one of the small features of Special Pool.
>>
>> Wang, Jian J (2):
>>  Implement NULL pointer detection for EDK-II Core
>>  Implement NULL pointer detection for EDK-II SMM Core and driver
>>
>> MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
>> MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
>> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>> MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
>> ++++++++++++++++--------
>> MdeModulePkg/MdeModulePkg.dec                    |  7 +++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12 +++++++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
>> ++++++++++++++++++++++-
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12 +++++++++++
>> 10 files changed, 84 insertions(+), 14 deletions(-)
>>
>> --
>> 2.11.0.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto: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


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] Implement NULL pointer detection feature
  2017-09-01  2:27                 ` Wang, Jian J
@ 2017-09-01  5:22                   ` Wang, Jian J
  0 siblings, 0 replies; 17+ messages in thread
From: Wang, Jian J @ 2017-09-01  5:22 UTC (permalink / raw)
  To: Wang, Jian J, Yao, Jiewen, Johnson, Brian (EXL - Eagan),
	afish@apple.com
  Cc: edk2-devel@lists.01.org

Updated version according to some internal discussions:

a) Use one PCD with bitmask to enable/disable this feature for different boot phases. This PCD is a "fixed" PCD with type of UINT8, and all bits are cleared by default. Please note that setting BIT7 with BIT0 set is used to "DISABLE" NULL pointer detection for code after EndOfDxe as a workaround for non-fixable NULL access in OpROM or boot loaders.

     gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
     #  BIT0 - Enable NULL pointer detection for UEFI.<BR>
     #  BIT1 - Enable NULL pointer detection for SMM.<BR>
     #  BIT2-6 - Reserved for PEI and other possible uses.<BR>
     #  BIT7 - Disable NULL pointer detection after EndOfDxe.<BR>

b) Add PEI support. Since PEI requirement has been covered by following new bug tracker, it won't be included by current patch update but a separate one.

    https://bugzilla.tianocore.org/show_bug.cgi?id=687

c) CSM code lines which access page 0 will be enclosed by <enable_page_0> and then <disable_page_0> code. This is subject to change if critical performance issue is found.


-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Jian J
Sent: Friday, September 01, 2017 10:27 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; Johnson, Brian (EXL - Eagan) <brian.johnson@hpe.com>; afish@apple.com
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Let me summarize the changes to this feature. If no new comments, I'll start to make changes to current implementation.

a) Use one PCD with bit set/clear to enable/disable this feature for different boot phases. To make it easier to use, I'd suggest to use the bit sequence to follow the boot phase sequence (bit0->bit1->bit2......bit7 ===> PEI->DXE->PostDxe......SMM), just like below. SMM is a little bit special so it consumes the MSB.

     gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
     #  BIT0 - Enable NULL pointer detection for PEI.<BR>
     #  BIT1 - Enable NULL pointer detection for DXE.<BR>
     #  BIT2 - Enable NULL pointer detection after EndOfDxe.<BR>
     #  BIT7 - Disable NULL pointer detection for SMM.<BR>

    This PCD is a "fixed" PCD and all bits are cleared by default. Since this is a feature for debug purpose, I see no reason to make it dynamic. 

b) Add PEI and post DXE phase support. Since PEI phase requirement has been covered by following new bug tracker, it won't be included by current patch update but a separate one. Changes for Post DXE phase will be included.

    https://bugzilla.tianocore.org/show_bug.cgi?id=687

Thanks,
Wang, Jian J

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Thursday, August 31, 2017 9:16 AM
To: Johnson, Brian (EXL - Eagan) <brian.johnson@hpe.com>; afish@apple.com
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Thanks.
I have filed https://bugzilla.tianocore.org/show_bug.cgi?id=687 for PEI phase, with suggestion from Brian Johnson and Andrew Fish.

If you share some of your PEI code to elimination the duplicated effort, that would be great.

Yes, we can calculate the perf data in CSM to see what is the best way to enable this feature. Good suggestion.

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Johnson, Brian (EXL - Eagan)
Sent: Thursday, August 31, 2017 5:36 AM
To: afish@apple.com; Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature

Jiewen,

Certainly PEI could be done later.  There's no need to get all the code in at once, and you have the DXE/SMM part pretty much ready to go.

I'm willing to go with a single PCD with a bitmap, or multiple PCDs.  Whatever seems best.

IIRC protecting page 0 in PEI involves modifying the main GCD descriptor to be a grows-downward variety, with a limit above page 0.  That way you get an exception if page 0 is accessed.  I'd have to check on the steps needed to release the actual code, which may take quite a while.  You may be better off just doing it yourself.  I'll take another look at the code to make sure I'm explaining this properly, though.

CSM is one of the few pieces of BIOS which has a legitimate need to access page 0.  We get the best protection by having it enable, access, and then disable page 0.  But if that affects performance too badly, we may need to have it leave page 0 enabled.  I don't enable CSM on the platforms I work on, so it's not something I have much to say about.  Those who use CSM would want to weigh in.

Brian Johnson

-----Original Message-----
From: afish@apple.com<mailto:afish@apple.com> [mailto:afish@apple.com]
Sent: Wednesday, August 30, 2017 11:27 AM
To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Cc: Johnson, Brian (EXL - Eagan) <brian.johnson@hpe.com<mailto:brian.johnson@hpe.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature


> On Aug 30, 2017, at 7:55 AM, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
>
> Hi Brian
> Good feedback.
> Comment in line.
>
> From: Johnson, Brian (EXL - Eagan) [mailto:brian.johnson@hpe.com]
> Sent: Tuesday, August 29, 2017 11:17 PM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> Thank you for implementing this feature!  It is very  helpful for catching pointer-related problems.  We have used a similar scheme on our systems for years, and caught several important bugs.  Some comments:
>
> * It's possible to implement similar protections in PEI (IA32) by modifying the GDT descriptors.
> [Jiewen] Enabling PEI is interesting.
> Since we are working on DXE and SMM as our first priority, can we treat the PEI enabling as a separate task?
> If you can help to create a patch for PEI and submit, that will be even better.
>

Jiewen,

This is slightly off topic but I've noticed in early PEI the issue on some x86 chipsets is not that a NULL pointer reference is bad, but any reference to an address space that is not decoded will triple bus fault the processor and that is really hard to debug. If the GDT trick could be used to cause an exception that drops into the debugger vs. a triple bus fault that would be a real win for debugging. While the memory ranges would be platform specific it would be awesome to have some plumbing in the edk2 to make this easier to implement.

Thanks,

Andrew Fish

>
>
> * For flexibility, I'd like NULL pointer protection to be controlled independently in PEI, DXE, and SMM, using separate PCDs.
> [Jiewen] I think we can existing example on using one PCD to control all phase or using one PCD to control different phase.
> For example, we use below PCD to control all phases, PEI/DXE/SMM
>  ## The mask is used to control PerformanceLib behavior.<BR><BR>
>  #  BIT0 - Enable Performance Measurement.<BR>
>  # @Prompt Performance Measurement Property.
>  # @Expression  0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask & 0xFE) == 0
>  gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask|0|UINT8|0x00000009
>
> And we use below PCD to control different phase, DXE/SMM
>  ## The mask is used to control memory profile behavior.<BR><BR>
>  #  BIT0 - Enable UEFI memory profile.<BR>
>  #  BIT1 - Enable SMRAM profile.<BR>
>  #  BIT7 - Disable recording at the start.<BR>
>  # @Prompt Memory Profile Property.
>  # @Expression  0x80000002 | (gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask & 0x7C) == 0
>  gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfilePropertyMask|0x0|UINT8|0x30001041
>
> In order to make the code consistent with the existing PCD, I am thinking to use one PCD named PcdNullPointerDetectionPropertyMask
>  #  BIT0 - Enable NULL pointer detection for UEFI.<BR>
>  #  BIT1 - Enable NULL pointer detection for SMM.<BR>
>  #  BIT2 - Enable NULL pointer detection for PEI.<BR>
>  #  BIT7 - Disable NULL pointer detection after EndOfDxe.<BR>
>
> I am not so worried about pre-memory initialization PEI phase, because page 0 is always invalid.
> For post-memory initialization PEI phase, we can modify the page table in memory, and dynamic PCD can be used at that time.
>
>
> * We have seen various option ROMs and OS boot loaders which have NULL pointer issues, but are outside of our control.  It is useful to enable NULL pointer protection during as much of the boot as possible, but disable it before running these other executables.  So I'd suggest adding another PCD, perhaps PcdNullPointerDetectionPostDxe, to control NULL pointer protection late in boot.  If PcdNullPointerDetection != PcdNullPointerDetectionPostDxe, install an end-of-DXE event (gEfiEndOfDxeEventGroupGuid) which changes the protection of page 0 using a call to EFI_CPU_ARCH_PROTOCOL. SetMemoryAttributes(CpuArch, 0, EFI_PAGE_SIZE, 0).
> [Jiewen] Good point for the compatibility consideration.
> Another thing is that CSM module may need access address 0. We have 2 choices:
> 1) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0, access page 0, then disable page 0.
> 2) CSM32 can call CpuArch->SetMemoryAttribute() to enable page 0 at the beginning, and always keep it enabled.
> Any though on that?
>
>
> So ideally I'd like to have 4 PCDs:
>
> PcdNullPointerDetectionPei
> PcdNullPointerDetectionDxe
> PcdNullPointerDetectionSmm
> PcdNullPointerDetectionPostDxe
>
> Thanks,
> Brian Johnson
> HPE
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
> Sent: Sunday, August 27, 2017 10:39 PM
> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Subject: Re: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> Comment in line.
>
> From: Wang, Jian J
> Sent: Monday, August 28, 2017 11:24 AM
> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
>
> 1)      I think this feature should be 'FALSE' by default. I forgot to reset its default value. This feature makes use of page mechanism to detect NULL pointer. So any ARCH doesn't support paging in EDK-II can't use it. Currently validated platform includes VLV2 and Denlow. Let me know if all platform must be validated or not.
>
> [Jiewen] Yes, I am OK to set to be FALSE to provide best compatibility.
> I suggest we validate all open source IA platforms, such as Quark, and OVMF with TRUE.
>
>
> 2)      It's hard to make it a dynamic feature because we need to setup page table for physical address 0-4095 in advance. If there's no memory alloc/free action after enabling this feature, there's no chance to make those change in page table. Then the usage of feature will be limited in such case.
>
> [Jiewen] Dynamic means the initial value is dynamic. I am not saying we need modify the page table.
>
> An implementation could be:
> A platform can set this PCD in PEI phase based upon a setup variable to choose CSM enable or disable.
>
> The IPL sets page table based upon this PCD value. The DXE Core cannot consume PCD directly, because it might be dynamic. But we can pass the information from IPL via HOB. All the DXE module just checks the value based upon HOB.
>
>
>
>
> From: Yao, Jiewen
> Sent: Monday, August 28, 2017 11:10 AM
> To: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com<mailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com%3cmailto:jian.j.wang@intel.com>>>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
> Subject: RE: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>
> Thank you  to enable this feature.
>
> I have 2 comments, after a very quick review.
>
>
> 1)      I notice it is enabled by default "gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetection|TRUE".
>
> Would you please provide the information on how many open source platforms are validated?
> Such as, IA platform (VLV2, Quark), emu platform (OVMF, NT32)?
> Or do we need validate any ARM platform?
>
>
>
> 2)      I am thinking about CSM platform. Do you think we can make it dynamic, as such, a platform may set the validate based upon CSM enable/disable?
>
>
> Or if we need update the CSM module to patch the page table automatically? Once this is feature is ON.
>
>
> Thank you
> Yao Jiewen
>
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
>> Jian J
>> Sent: Monday, August 28, 2017 10:51 AM
>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
>> Subject: [edk2] [PATCH 0/2] Implement NULL pointer detection feature
>>
>> This patch is the implementation of NULL pointer detection feature,
>> which is one of the small features of Special Pool.
>>
>> Wang, Jian J (2):
>>  Implement NULL pointer detection for EDK-II Core
>>  Implement NULL pointer detection for EDK-II SMM Core and driver
>>
>> MdeModulePkg/Core/Dxe/DxeMain.inf                |  3 ++-
>> MdeModulePkg/Core/Dxe/Mem/Page.c                 |  5 +++--
>> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  1 +
>> MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  6 ++++--
>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 26
>> ++++++++++++++++--------
>> MdeModulePkg/MdeModulePkg.dec                    |  7 +++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c         | 12 +++++++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c            | 25
>> ++++++++++++++++++++++-
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf     |  1 +
>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c          | 12 +++++++++++
>> 10 files changed, 84 insertions(+), 14 deletions(-)
>>
>> --
>> 2.11.0.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto: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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-09-01  5:19 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-28  2:51 [PATCH 0/2] Implement NULL pointer detection feature Wang, Jian J
2017-08-28  2:51 ` [PATCH 1/2] Implement NULL pointer detection for EDK-II Core Wang, Jian J
2017-08-28  2:51 ` [PATCH 2/2] Implement NULL pointer detection for EDK-II SMM Core and driver Wang, Jian J
2017-08-28  3:10 ` [PATCH 0/2] Implement NULL pointer detection feature Yao, Jiewen
2017-08-28  3:24   ` Wang, Jian J
2017-08-28  3:39     ` Yao, Jiewen
2017-08-29 15:16       ` Johnson, Brian (EXL - Eagan)
2017-08-29 16:02         ` Kinney, Michael D
2017-08-29 17:12           ` Johnson, Brian (EXL - Eagan)
2017-08-29 17:14             ` Tim Lewis
2017-08-30 14:55         ` Yao, Jiewen
2017-08-30 16:27           ` Andrew Fish
2017-08-30 21:36             ` Johnson, Brian (EXL - Eagan)
2017-08-31  1:16               ` Yao, Jiewen
2017-09-01  2:27                 ` Wang, Jian J
2017-09-01  5:22                   ` Wang, Jian J
2017-08-31  0:55             ` Yao, Jiewen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox