public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.
@ 2017-09-13  8:07 Wang, Jian J
  0 siblings, 0 replies; 6+ messages in thread
From: Wang, Jian J @ 2017-09-13  8:07 UTC (permalink / raw)
  To: edk2-devel

The mechanism behind is the same as NULL pointer detection enabled in EDK-II core. SMM has its own page table and we have to disable page 0 again in SMM mode.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Justen, Jordan L <jordan.l.justen@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Wolman, Ayellet <ayellet.wolman@intel.com>
Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J <jian.j.wang@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c     | 11 +++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c        | 25 ++++++++++++++++++++++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  2 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +++++++++--------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c      | 11 +++++++++++
 5 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index f295c2ebf2..d423958783 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -155,6 +155,17 @@ SmiPFHandler (
     }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (NULL_DETECTION_ENABLED && (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..81c5ac9d11 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 (NULL_DETECTION_ENABLED) {
+    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.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 1cf85c1481..bcb3032db8 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -153,6 +153,8 @@ typedef UINT32                              SMM_CPU_ARRIVAL_EXCEPTIONS;
 #define ARRIVAL_EXCEPTION_DELAYED           0x2
 #define ARRIVAL_EXCEPTION_SMI_DISABLED      0x4
 
+#define NULL_DETECTION_ENABLED    ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT1) != 0)
+
 //
 // Private structure for the SMM CPU module that is stored in DXE Runtime memory
 // Contains the SMM Configuration Protocols that is produced.
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 099792e6ce..57a14d9f24 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -138,14 +138,14 @@
   gEdkiiPiSmmMemoryAttributesTableGuid     ## CONSUMES ## SystemTable
 
 [FeaturePcd]
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                         ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp            ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection             ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport                   ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable                 ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer             ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock         ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                           ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp              ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection               ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport                     ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                      ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable                   ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer               ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock           ## CONSUMES
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
@@ -159,6 +159,7 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable               ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask    ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
 
 [Depex]
   gEfiMpServiceProtocolGuid
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 3dde80f9ba..e67bcfe0f6 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -872,6 +872,17 @@ SmiPFHandler (
     }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (NULL_DETECTION_ENABLED && (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.14.1.windows.1




^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.
  2017-09-13  9:25 ` [PATCH 0/4] Implement NULL pointer detection feature for special pool Wang, Jian J
@ 2017-09-13  9:25   ` Wang, Jian J
  2017-09-13 16:33     ` Johnson, Brian (EXL - Eagan)
  2017-09-13 17:31     ` Jordan Justen
  0 siblings, 2 replies; 6+ messages in thread
From: Wang, Jian J @ 2017-09-13  9:25 UTC (permalink / raw)
  To: edk2-devel
  Cc: Jiewen Yao, Eric Dong, Star Zeng, Laszlo Ersek, Justen, Jordan L,
	Kinney, Michael D, Wolman, Ayellet

The mechanism behind is the same as NULL pointer detection enabled in EDK-II core. SMM has its own page table and we have to disable page 0 again in SMM mode.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Justen, Jordan L <jordan.l.justen@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Wolman, Ayellet <ayellet.wolman@intel.com>
Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J <jian.j.wang@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c     | 11 +++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c        | 25 ++++++++++++++++++++++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  2 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +++++++++--------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c      | 11 +++++++++++
 5 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index f295c2ebf2..d423958783 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -155,6 +155,17 @@ SmiPFHandler (
     }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (NULL_DETECTION_ENABLED && (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..81c5ac9d11 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 (NULL_DETECTION_ENABLED) {
+    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.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 1cf85c1481..bcb3032db8 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -153,6 +153,8 @@ typedef UINT32                              SMM_CPU_ARRIVAL_EXCEPTIONS;
 #define ARRIVAL_EXCEPTION_DELAYED           0x2
 #define ARRIVAL_EXCEPTION_SMI_DISABLED      0x4
 
+#define NULL_DETECTION_ENABLED    ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT1) != 0)
+
 //
 // Private structure for the SMM CPU module that is stored in DXE Runtime memory
 // Contains the SMM Configuration Protocols that is produced.
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 099792e6ce..57a14d9f24 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -138,14 +138,14 @@
   gEdkiiPiSmmMemoryAttributesTableGuid     ## CONSUMES ## SystemTable
 
 [FeaturePcd]
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                         ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp            ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection             ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport                   ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable                 ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer             ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock         ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                           ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp              ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection               ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport                     ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                      ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable                   ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer               ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock           ## CONSUMES
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
@@ -159,6 +159,7 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable               ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask    ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
 
 [Depex]
   gEfiMpServiceProtocolGuid
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 3dde80f9ba..e67bcfe0f6 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -872,6 +872,17 @@ SmiPFHandler (
     }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (NULL_DETECTION_ENABLED && (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.14.1.windows.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.
  2017-09-13  9:25   ` [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code Wang, Jian J
@ 2017-09-13 16:33     ` Johnson, Brian (EXL - Eagan)
  2017-09-14  1:31       ` Wang, Jian J
  2017-09-13 17:31     ` Jordan Justen
  1 sibling, 1 reply; 6+ messages in thread
From: Johnson, Brian (EXL - Eagan) @ 2017-09-13 16:33 UTC (permalink / raw)
  To: Wang, Jian J, edk2-devel@lists.01.org
  Cc: Justen@ml01.01.org, Eric Dong, Kinney@ml01.01.org, Jordan L,
	Wolman@ml01.01.org, Jiewen Yao, Ayellet, Michael D, Laszlo Ersek,
	Star Zeng

Comments below.

Brian

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

The mechanism behind is the same as NULL pointer detection enabled in EDK-II core. SMM has its own page table and we have to disable page 0 again in SMM mode.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Justen, Jordan L <jordan.l.justen@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Wolman, Ayellet <ayellet.wolman@intel.com>
Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J <jian.j.wang@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c     | 11 +++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c        | 25 ++++++++++++++++++++++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  2 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +++++++++--------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c      | 11 +++++++++++
 5 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index f295c2ebf2..d423958783 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -155,6 +155,17 @@ SmiPFHandler (
     }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (NULL_DETECTION_ENABLED && (PFAddress >= 0 && PFAddress < EFI_PAGE_SIZE)) {

[Brian] PFAddress is unsigned, so it will always be >= 0.  Some compilers complain about this....  Should probably remove that part of the test.

+    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..81c5ac9d11 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 (NULL_DETECTION_ENABLED) {
+    Pte = (UINT64*)(UINT64)(Pdpte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));

[Brian] Shouldn't the inner cast be (UINTN), not (UINT64)?  That would match the PcdCpuSmmStackGuard section above.

+    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));

[Brian] Same comment re. the inner cast.

+      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.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 1cf85c1481..bcb3032db8 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -153,6 +153,8 @@ typedef UINT32                              SMM_CPU_ARRIVAL_EXCEPTIONS;
 #define ARRIVAL_EXCEPTION_DELAYED           0x2
 #define ARRIVAL_EXCEPTION_SMI_DISABLED      0x4
 
+#define NULL_DETECTION_ENABLED    ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT1) != 0)
+
 //
 // Private structure for the SMM CPU module that is stored in DXE Runtime memory
 // Contains the SMM Configuration Protocols that is produced.
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 099792e6ce..57a14d9f24 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -138,14 +138,14 @@
   gEdkiiPiSmmMemoryAttributesTableGuid     ## CONSUMES ## SystemTable
 
 [FeaturePcd]
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                         ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp            ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection             ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport                   ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable                 ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer             ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock         ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                           ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp              ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection               ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport                     ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                      ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable                   ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer               ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock           ## CONSUMES
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
@@ -159,6 +159,7 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable               ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask    ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
 
 [Depex]
   gEfiMpServiceProtocolGuid
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 3dde80f9ba..e67bcfe0f6 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -872,6 +872,17 @@ SmiPFHandler (
     }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (NULL_DETECTION_ENABLED && (PFAddress >= 0 && PFAddress < EFI_PAGE_SIZE)) {

[Brian] PFAddress is unsigned, so it will always be >= 0.  Some compilers complain about this....  Should probably remove that part of the test.

+    DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
+    DEBUG_CODE (
+      DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip);
+    );
+    CpuDeadLoop ();
+  }
+
   if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
     SmmProfilePFHandler (
       SystemContext.SystemContextX64->Rip,
-- 
2.14.1.windows.1

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.
  2017-09-13  9:25   ` [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code Wang, Jian J
  2017-09-13 16:33     ` Johnson, Brian (EXL - Eagan)
@ 2017-09-13 17:31     ` Jordan Justen
  2017-09-14  1:20       ` Wang, Jian J
  1 sibling, 1 reply; 6+ messages in thread
From: Jordan Justen @ 2017-09-13 17:31 UTC (permalink / raw)
  To: Wang, Jian J, edk2-devel
  Cc: Jiewen Yao, Eric Dong, Star Zeng, Laszlo Ersek, Justen, Kinney,
	Michael D, Wolman, Ayellet

On 2017-09-13 02:25:05, Wang, Jian J wrote:
> The mechanism behind is the same as NULL pointer detection enabled in EDK-II core. SMM has its own page table and we have to disable page 0 again in SMM mode.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Wolman, Ayellet <ayellet.wolman@intel.com>
> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wang, Jian J <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c     | 11 +++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c        | 25 ++++++++++++++++++++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  2 ++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +++++++++--------
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c      | 11 +++++++++++
>  5 files changed, 57 insertions(+), 9 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index f295c2ebf2..d423958783 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -155,6 +155,17 @@ SmiPFHandler (
>      }
>    }
>  
> +  //
> +  // If NULL pointer was just accessed
> +  //
> +  if (NULL_DETECTION_ENABLED && (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..81c5ac9d11 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 (NULL_DETECTION_ENABLED) {
> +    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.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 1cf85c1481..bcb3032db8 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -153,6 +153,8 @@ typedef UINT32                              SMM_CPU_ARRIVAL_EXCEPTIONS;
>  #define ARRIVAL_EXCEPTION_DELAYED           0x2
>  #define ARRIVAL_EXCEPTION_SMI_DISABLED      0x4
>  
> +#define NULL_DETECTION_ENABLED    ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT1) != 0)
> +

Again, I think the NULL_DETECTION_ENABLED macro code style looks odd.
Maybe it is just better to include the duplicated code directly in the
3 places?

The commit message for this patch has a long line length.

-Jordan

>  //
>  // Private structure for the SMM CPU module that is stored in DXE Runtime memory
>  // Contains the SMM Configuration Protocols that is produced.
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> index 099792e6ce..57a14d9f24 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> @@ -138,14 +138,14 @@
>    gEdkiiPiSmmMemoryAttributesTableGuid     ## CONSUMES ## SystemTable
>  
>  [FeaturePcd]
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                         ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp            ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection             ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport                   ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable                 ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer             ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock         ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                           ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp              ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection               ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport                     ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                      ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable                   ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer               ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock           ## CONSUMES
>  
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
> @@ -159,6 +159,7 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable               ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                   ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask    ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
>  
>  [Depex]
>    gEfiMpServiceProtocolGuid
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 3dde80f9ba..e67bcfe0f6 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -872,6 +872,17 @@ SmiPFHandler (
>      }
>    }
>  
> +  //
> +  // If NULL pointer was just accessed
> +  //
> +  if (NULL_DETECTION_ENABLED && (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.14.1.windows.1
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.
  2017-09-13 17:31     ` Jordan Justen
@ 2017-09-14  1:20       ` Wang, Jian J
  0 siblings, 0 replies; 6+ messages in thread
From: Wang, Jian J @ 2017-09-14  1:20 UTC (permalink / raw)
  To: Justen, Jordan L, edk2-devel@lists.01.org
  Cc: Yao, Jiewen, Dong, Eric, Zeng, Star, Laszlo Ersek,
	Kinney, Michael D, Wolman, Ayellet, Wolman, Ayellet

I'll use the tool to check the format. For the macro, it's for readability purpose. How's the library replacement suggestion from Laszlo? 

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

On 2017-09-13 02:25:05, Wang, Jian J wrote:
> The mechanism behind is the same as NULL pointer detection enabled in EDK-II core. SMM has its own page table and we have to disable page 0 again in SMM mode.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Wolman, Ayellet <ayellet.wolman@intel.com>
> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wang, Jian J <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c     | 11 +++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c        | 25 ++++++++++++++++++++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  2 ++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +++++++++--------
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c      | 11 +++++++++++
>  5 files changed, 57 insertions(+), 9 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index f295c2ebf2..d423958783 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -155,6 +155,17 @@ SmiPFHandler (
>      }
>    }
>  
> +  //
> +  // If NULL pointer was just accessed
> +  //
> +  if (NULL_DETECTION_ENABLED && (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..81c5ac9d11 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 (NULL_DETECTION_ENABLED) {
> +    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.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 1cf85c1481..bcb3032db8 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -153,6 +153,8 @@ typedef UINT32                              SMM_CPU_ARRIVAL_EXCEPTIONS;
>  #define ARRIVAL_EXCEPTION_DELAYED           0x2
>  #define ARRIVAL_EXCEPTION_SMI_DISABLED      0x4
>  
> +#define NULL_DETECTION_ENABLED    ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT1) != 0)
> +

Again, I think the NULL_DETECTION_ENABLED macro code style looks odd.
Maybe it is just better to include the duplicated code directly in the
3 places?

The commit message for this patch has a long line length.

-Jordan

>  //
>  // Private structure for the SMM CPU module that is stored in DXE Runtime memory
>  // Contains the SMM Configuration Protocols that is produced.
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> index 099792e6ce..57a14d9f24 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> @@ -138,14 +138,14 @@
>    gEdkiiPiSmmMemoryAttributesTableGuid     ## CONSUMES ## SystemTable
>  
>  [FeaturePcd]
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                         ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp            ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection             ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport                   ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable                 ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer             ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock         ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                           ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp              ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection               ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport                     ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                      ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable                   ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer               ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock           ## CONSUMES
>  
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
> @@ -159,6 +159,7 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable               ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                   ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask    ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
>  
>  [Depex]
>    gEfiMpServiceProtocolGuid
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 3dde80f9ba..e67bcfe0f6 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -872,6 +872,17 @@ SmiPFHandler (
>      }
>    }
>  
> +  //
> +  // If NULL pointer was just accessed
> +  //
> +  if (NULL_DETECTION_ENABLED && (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.14.1.windows.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.
  2017-09-13 16:33     ` Johnson, Brian (EXL - Eagan)
@ 2017-09-14  1:31       ` Wang, Jian J
  0 siblings, 0 replies; 6+ messages in thread
From: Wang, Jian J @ 2017-09-14  1:31 UTC (permalink / raw)
  To: Johnson, Brian (EXL - Eagan), edk2-devel@lists.01.org
  Cc: Justen@ml01.01.org, Dong, Eric, Kinney@ml01.01.org,
	Justen, Jordan L, Wolman@ml01.01.org, Yao, Jiewen,
	Wolman, Ayellet, Kinney, Michael D, Laszlo Ersek, Zeng, Star

Thanks for the comments. See my comment start with [Jian] below.

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

Comments below.

Brian

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

The mechanism behind is the same as NULL pointer detection enabled in EDK-II core. SMM has its own page table and we have to disable page 0 again in SMM mode.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Justen, Jordan L <jordan.l.justen@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Wolman, Ayellet <ayellet.wolman@intel.com>
Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wang, Jian J <jian.j.wang@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c     | 11 +++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c        | 25 ++++++++++++++++++++++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   |  2 ++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 17 +++++++++--------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c      | 11 +++++++++++
 5 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index f295c2ebf2..d423958783 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -155,6 +155,17 @@ SmiPFHandler (
     }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (NULL_DETECTION_ENABLED && (PFAddress >= 0 && PFAddress < EFI_PAGE_SIZE)) {

[Brian] PFAddress is unsigned, so it will always be >= 0.  Some compilers complain about this....  Should probably remove that part of the test.
[Jian] You're right. The first test is not necessary.

+    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..81c5ac9d11 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 (NULL_DETECTION_ENABLED) {
+    Pte = (UINT64*)(UINT64)(Pdpte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));

[Brian] Shouldn't the inner cast be (UINTN), not (UINT64)?  That would match the PcdCpuSmmStackGuard section above.
[Jian] You're right. Inner cast should be UINTN.

+    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));

[Brian] Same comment re. the inner cast.

+      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.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 1cf85c1481..bcb3032db8 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -153,6 +153,8 @@ typedef UINT32                              SMM_CPU_ARRIVAL_EXCEPTIONS;
 #define ARRIVAL_EXCEPTION_DELAYED           0x2
 #define ARRIVAL_EXCEPTION_SMI_DISABLED      0x4
 
+#define NULL_DETECTION_ENABLED    ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT1) != 0)
+
 //
 // Private structure for the SMM CPU module that is stored in DXE Runtime memory
 // Contains the SMM Configuration Protocols that is produced.
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 099792e6ce..57a14d9f24 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -138,14 +138,14 @@
   gEdkiiPiSmmMemoryAttributesTableGuid     ## CONSUMES ## SystemTable
 
 [FeaturePcd]
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                         ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp            ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection             ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport                   ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable                 ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer             ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock         ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                           ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp              ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection               ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport                     ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                      ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable                   ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer               ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock           ## CONSUMES
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## SOMETIMES_CONSUMES
@@ -159,6 +159,7 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable               ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask    ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
 
 [Depex]
   gEfiMpServiceProtocolGuid
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 3dde80f9ba..e67bcfe0f6 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -872,6 +872,17 @@ SmiPFHandler (
     }
   }
 
+  //
+  // If NULL pointer was just accessed
+  //
+  if (NULL_DETECTION_ENABLED && (PFAddress >= 0 && PFAddress < EFI_PAGE_SIZE)) {

[Brian] PFAddress is unsigned, so it will always be >= 0.  Some compilers complain about this....  Should probably remove that part of the test.

+    DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
+    DEBUG_CODE (
+      DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip);
+    );
+    CpuDeadLoop ();
+  }
+
   if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
     SmmProfilePFHandler (
       SystemContext.SystemContextX64->Rip,
-- 
2.14.1.windows.1

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



^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-09-14  1:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-13  8:07 [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code Wang, Jian J
     [not found] <Implement NULL pointer detection feature>
2017-09-13  9:25 ` [PATCH 0/4] Implement NULL pointer detection feature for special pool Wang, Jian J
2017-09-13  9:25   ` [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code Wang, Jian J
2017-09-13 16:33     ` Johnson, Brian (EXL - Eagan)
2017-09-14  1:31       ` Wang, Jian J
2017-09-13 17:31     ` Jordan Justen
2017-09-14  1:20       ` Wang, Jian J

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