[v1] Implement NULL pointer detection feature for special pool

[edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.

Posted by Wang, Jian J 8 years, 1 month ago

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

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

Re: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.

Posted by Johnson, Brian (EXL - Eagan) 8 years, 1 month ago

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

Re: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.

Posted by Wang, Jian J 8 years, 1 month ago

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

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

Re: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.

Posted by Jordan Justen 8 years, 1 month ago

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

Re: [edk2] [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.

Posted by Wang, Jian J 8 years, 1 month ago

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

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