Another comment.
Should IsNullDetectionEnabled() be checked before calling ClearLegacyMemory()?
Thanks,
Star
-----Original Message-----
From: Zeng, Star
Sent: Thursday, September 28, 2017 11:24 AM
To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.d...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Yao,
Jiewen <jiewen....@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>;
Justen, Jordan L <jordan.l.jus...@intel.com>; Wolman, Ayellet
<ayellet.wol...@intel.com>; Zeng, Star <star.z...@intel.com>
Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer
detection
Minor comments to this patch.
1. IsNullDetectionEnabled() function is put in DxeLoad.c that is shared by all
ARCHs, and the function is consuming PcdNullPointerDetectionPropertyMask, but
PcdNullPointerDetectionPropertyMask is only declared in "[Pcd.IA32,Pcd.X64]" in
inf.
I am not sure whether it will cause build failure or not for non-IA32/X64 ARCHs.
2. How about using name "ClearFirst4KPage" instead of "ClearLegacyMemory" to be
more clear?
Thanks,
Star
-----Original Message-----
From: Wang, Jian J
Sent: Thursday, September 28, 2017 9:04 AM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.z...@intel.com>; Dong, Eric <eric.d...@intel.com>; Laszlo
Ersek <ler...@redhat.com>; Yao, Jiewen <jiewen....@intel.com>; Kinney, Michael
D <michael.d.kin...@intel.com>; Justen, Jordan L <jordan.l.jus...@intel.com>;
Wolman, Ayellet <ayellet.wol...@intel.com>
Subject: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
> According to Jiewen's feedback, change the page split condition for
> NULL pointer detection to exclude IsExecuteDisableBitAvailable()
> (Ia32/DxeLoadFunc.c)
NULL pointer detection is done by making use of paging mechanism of CPU.
During page table setup, if enabled, the first 4-K page (0-4095) will be marked
as NOT PRESENT. Any code which unintentionally access memory between
0-4095 will trigger a Page Fault exception which warns users that there's
potential illegal code in BIOS.
This also means that legacy code which has to access memory between 0-4095
should be cautious to temporarily disable this feature before the access and
re-enable it afterwards; or disalbe this feature at all.
Cc: Star Zeng <star.z...@intel.com>
Cc: Eric Dong <eric.d...@intel.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Jiewen Yao <jiewen....@intel.com>
Cc: Michael Kinney <michael.d.kin...@intel.com>
Cc: Jordan Justen <jordan.l.jus...@intel.com>
Cc: Ayellet Wolman <ayellet.wol...@intel.com>
Suggested-by: Ayellet Wolman <ayellet.wol...@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.w...@intel.com>
---
MdeModulePkg/Core/DxeIplPeim/DxeIpl.h | 25 +++++++++
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 1 +
MdeModulePkg/Core/DxeIplPeim/DxeLoad.c | 65 ++++++++++++++++++++++++
MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c | 11 +++-
MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c | 2 +
MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 31 ++++++++---
6 files changed, 126 insertions(+), 9 deletions(-)
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
index 72d2532f50..1654bcd2dc 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
@@ -240,4 +240,29 @@ Decompress (
OUT UINTN *OutputSize
);
+/**
+ Clear legacy memory located at the first 4K-page.
+
+ This function traverses the whole HOB list to check if memory from 0 to 4095
+ exists and has not been allocated, and then clear it if so.
+
+ @param HoStart The start of HobList passed to DxeCore.
+
+**/
+VOID
+ClearLegacyMemory (
+ IN VOID *HobStart
+ );
+
+/**
+ Return configure status of NULL pointer detection feature
+
+ @return TRUE NULL pointer detection feature is enabled
+ @return FALSE NULL pointer detection feature is disabled **/
+BOOLEAN IsNullDetectionEnabled (
+ VOID
+ );
+
#endif
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index c54afe4aa6..9d0e76a293 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -115,6 +115,7 @@
[Pcd.IA32,Pcd.X64]
gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable ##
SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask ##
CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ##
CONSUMES
[Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
SOMETIMES_CONSUMES
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
index 50b5440d15..0a71b1f3de 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
@@ -825,3 +825,68 @@ UpdateStackHob (
Hob.Raw = GET_NEXT_HOB (Hob);
}
}
+
+/**
+ Clear legacy memory located at the first 4K-page, if available.
+
+ This function traverses the whole HOB list to check if memory from 0 to 4095
+ exists and has not been allocated, and then clear it if so.
+
+ @param HoStart The start of HobList passed to DxeCore.
+
+**/
+VOID
+ClearLegacyMemory (
+ IN VOID *HobStart
+ )
+{
+ EFI_PEI_HOB_POINTERS RscHob;
+ EFI_PEI_HOB_POINTERS MemHob;
+ BOOLEAN DoClear;
+
+ RscHob.Raw = HobStart;
+ MemHob.Raw = HobStart;
+ DoClear = FALSE;
+
+ //
+ // Check if page 0 exists and free
+ //
+ while ((RscHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR,
+ RscHob.Raw)) != NULL) {
+ if (RscHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY
&&
+ RscHob.ResourceDescriptor->PhysicalStart == 0) {
+ DoClear = TRUE;
+ //
+ // Make sure memory at 0-4095 has not been allocated.
+ //
+ while ((MemHob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION,
+ MemHob.Raw)) != NULL) {
+ if (MemHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress
+ < EFI_PAGE_SIZE) {
+ DoClear = FALSE;
+ break;
+ }
+ MemHob.Raw = GET_NEXT_HOB (MemHob);
+ }
+ break;
+ }
+ RscHob.Raw = GET_NEXT_HOB (RscHob); }
+
+ if (DoClear) {
+ DEBUG ((DEBUG_INFO, "Clearing first 4K-page!\r\n"));
+ SetMem (NULL, EFI_PAGE_SIZE, 0);
+ }
+
+ return;
+}
+
+BOOLEAN
+IsNullDetectionEnabled (
+ VOID
+ )
+{
+ return (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0) ?
+ TRUE : FALSE);
+}
+
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index 1957326caf..7a8421dd16 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -123,7 +123,9 @@ Create4GPageTablesIa32Pae (
PageDirectoryPointerEntry->Bits.Present = 1;
for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress +=
SIZE_2MB) {
- if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress +
SIZE_2MB) > StackBase)) {
+ if ((IsNullDetectionEnabled () && PhysicalAddress == 0)
+ || ((PhysicalAddress < StackBase + StackSize)
+ && ((PhysicalAddress + SIZE_2MB) > StackBase))) {
//
// Need to split this 2M page that covers stack range.
//
@@ -240,6 +242,8 @@ HandOffToDxeCore (
EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
BOOLEAN BuildPageTablesIa32Pae;
+ ClearLegacyMemory (HobList.Raw);
+
Status = PeiServicesAllocatePages (EfiBootServicesData, EFI_SIZE_TO_PAGES
(STACK_SIZE), &BaseOfStack);
ASSERT_EFI_ERROR (Status);
@@ -379,7 +383,10 @@ HandOffToDxeCore (
TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER (TopOfStack,
CPU_STACK_ALIGNMENT);
PageTables = 0;
- BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) &&
IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
+ BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () &&
+ (IsNullDetectionEnabled () ||
+ (PcdGetBool (PcdSetNxForStack) &&
+ IsExecuteDisableBitAvailable
+ ())));
if (BuildPageTablesIa32Pae) {
PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
EnableExecuteDisableBit ();
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
index 6488880eab..d93a9c5a2d 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
@@ -42,6 +42,8 @@ HandOffToDxeCore (
EFI_VECTOR_HANDOFF_INFO *VectorInfo;
EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
+ ClearLegacyMemory (HobList.Raw);
+
//
// Get Vector Hand-off Info PPI and build Guided HOB
//
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index 48150be4e1..80c1821eca 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -90,8 +90,16 @@ Split2MPageTo4K (
//
PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
PageTableEntry->Bits.ReadWrite = 1;
- PageTableEntry->Bits.Present = 1;
- if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase +
StackSize)) {
+
+ if (IsNullDetectionEnabled () && PhysicalAddress4K == 0) {
+ PageTableEntry->Bits.Present = 0;
+ } else {
+ PageTableEntry->Bits.Present = 1;
+ }
+
+ if (PcdGetBool (PcdSetNxForStack)
+ && (PhysicalAddress4K >= StackBase)
+ && (PhysicalAddress4K < StackBase + StackSize)) {
//
// Set Nx bit for stack.
//
@@ -137,9 +145,12 @@ Split1GPageTo2M (
PhysicalAddress2M = PhysicalAddress;
for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M +=
SIZE_2MB) {
- if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M +
SIZE_2MB) > StackBase)) {
+ if ((IsNullDetectionEnabled () && PhysicalAddress2M == 0)
+ || (PcdGetBool (PcdSetNxForStack)
+ && (PhysicalAddress2M < StackBase + StackSize)
+ && ((PhysicalAddress2M + SIZE_2MB) > StackBase))) {
//
- // Need to split this 2M page that covers stack range.
+ // Need to split this 2M page that covers NULL or stack range.
//
Split2MPageTo4K (PhysicalAddress2M, (UINT64 *) PageDirectoryEntry,
StackBase, StackSize);
} else {
@@ -279,7 +290,10 @@ CreateIdentityMappingPageTables (
PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress += SIZE_1GB)
{
- if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase +
StackSize) && ((PageAddress + SIZE_1GB) > StackBase)) {
+ if ((IsNullDetectionEnabled () && PageAddress == 0)
+ || (PcdGetBool (PcdSetNxForStack)
+ && (PageAddress < StackBase + StackSize)
+ && ((PageAddress + SIZE_1GB) > StackBase))) {
Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry,
StackBase, StackSize);
} else {
//
@@ -308,9 +322,12 @@ CreateIdentityMappingPageTables (
PageDirectoryPointerEntry->Bits.Present = 1;
for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
SIZE_2MB) {
- if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase +
StackSize) && ((PageAddress + SIZE_2MB) > StackBase)) {
+ if ((IsNullDetectionEnabled () && PageAddress == 0)
+ || (PcdGetBool (PcdSetNxForStack)
+ && (PageAddress < StackBase + StackSize)
+ && ((PageAddress + SIZE_2MB) > StackBase))) {
//
- // Need to split this 2M page that covers stack range.
+ // Need to split this 2M page that covers NULL or stack range.
//
Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry,
StackBase, StackSize);
} else {
--
2.14.1.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
