From: "Johnson, Brian (EXL - Eagan)" <brian.johnson@hpe.com>
To: "Wang, Jian J" <jian.j.wang@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Justen@ml01.01.org" <Justen@ml01.01.org>,
Eric Dong <eric.dong@intel.com>,
"Kinney@ml01.01.org" <Kinney@ml01.01.org>,
Jordan L <jordan.l.justen@intel.com>,
"Wolman@ml01.01.org" <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: Re: [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code.
Date: Wed, 13 Sep 2017 16:33:37 +0000 [thread overview]
Message-ID: <DF4PR84MB015594C395E7133DD8C50559E16E0@DF4PR84MB0155.NAMPRD84.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20170913092507.12504-3-jian.j.wang@intel.com>
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
next prev parent reply other threads:[~2017-09-13 16:30 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
[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 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core Wang, Jian J
2017-09-13 16:33 ` Johnson, Brian (EXL - Eagan)
2017-09-14 1:37 ` Wang, Jian J
2017-09-13 17:28 ` Jordan Justen
2017-09-14 1:25 ` Wang, Jian J
2017-09-14 6:33 ` Jordan Justen
2017-09-14 6:51 ` Wang, Jian J
2017-09-14 8:22 ` Laszlo Ersek
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) [this message]
2017-09-14 1:31 ` Wang, Jian J
2017-09-13 17:31 ` Jordan Justen
2017-09-14 1:20 ` Wang, Jian J
2017-09-13 9:25 ` [PATCH 3/4] IntelFrameworkModulePkg/Csm: Update CSM code to temporarily bypass NULL pointer detection if enabled Wang, Jian J
2017-09-13 16:33 ` Johnson, Brian (EXL - Eagan)
2017-09-13 9:25 ` [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to " Wang, Jian J
2017-09-13 16:33 ` Johnson, Brian (EXL - Eagan)
2017-09-13 23:34 ` Laszlo Ersek
2017-09-14 1:17 ` Wang, Jian J
2017-09-14 3:17 ` Wang, Jian J
2017-09-14 8:30 ` Laszlo Ersek
2017-09-14 8:38 ` Yao, Jiewen
2017-09-14 8:46 ` Wang, Jian J
2017-09-14 8:48 ` Yao, Jiewen
2017-09-14 8:54 ` Laszlo Ersek
2017-09-14 9:39 ` Zeng, Star
2017-09-14 9:55 ` Laszlo Ersek
2017-09-14 10:16 ` Zeng, Star
2017-09-15 0:15 ` Wang, Jian J
2017-09-15 6:05 ` Wang, Jian J
2017-09-15 6:28 ` Zeng, Star
2017-09-14 8:52 ` Laszlo Ersek
2017-09-14 5:50 ` Jordan Justen
2017-09-14 6:52 ` Wang, Jian J
2017-09-14 8:26 ` Laszlo Ersek
2017-09-13 8:07 [PATCH 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM mode code Wang, Jian J
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DF4PR84MB015594C395E7133DD8C50559E16E0@DF4PR84MB0155.NAMPRD84.PROD.OUTLOOK.COM \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox