> 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.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ayellet Wolman <ayellet.wolman@intel.com>
Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@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
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.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@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.zeng@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Michael Kinney <michael.d.kinney@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Ayellet Wolman <ayellet.wolman@intel.com> Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jian J Wang <jian.j.wang@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
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.wang@intel.com>; edk2-devel@lists.01.org Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@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.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@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.zeng@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Michael Kinney <michael.d.kinney@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Ayellet Wolman <ayellet.wolman@intel.com> Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jian J Wang <jian.j.wang@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
Clearing this block of memory has nothing to do with NULL pointer detection. I'm not sure the extra check is necessary. > -----Original Message----- > From: Zeng, Star > Sent: Thursday, September 28, 2017 11:31 AM > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, > Jiewen <jiewen.yao@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; > Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer > detection > > 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.wang@intel.com>; edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, > Jiewen <jiewen.yao@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; > Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@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.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo > Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, > Michael D <michael.d.kinney@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@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.zeng@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Michael Kinney <michael.d.kinney@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Ayellet Wolman <ayellet.wolman@intel.com> > Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <jian.j.wang@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
If NULL pointer detection is disabled, the code should be behave same with before, and ClearLegacyMemory() is not needed to be called because DxeCore will behave same with before to handle the first 4K page clear , right? Thanks, Star -----Original Message----- From: Wang, Jian J Sent: Thursday, September 28, 2017 11:55 AM To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com> Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection Clearing this block of memory has nothing to do with NULL pointer detection. I'm not sure the extra check is necessary. > -----Original Message----- > From: Zeng, Star > Sent: Thursday, September 28, 2017 11:31 AM > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek > <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, > Michael D <michael.d.kinney@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; Wolman, Ayellet > <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL > pointer detection > > 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.wang@intel.com>; edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek > <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, > Michael D <michael.d.kinney@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; Wolman, Ayellet > <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@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.zeng@intel.com>; Dong, Eric > <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen > <jiewen.yao@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; Wolman, Ayellet > <ayellet.wolman@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.zeng@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Michael Kinney <michael.d.kinney@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Ayellet Wolman <ayellet.wolman@intel.com> > Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <jian.j.wang@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 > IndexOfPageDirectoryEntries+++= > 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 > IndexOfPageDirectoryEntries+++= > 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
From this perspective, you're right. > -----Original Message----- > From: Zeng, Star > Sent: Thursday, September 28, 2017 1:10 PM > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, > Jiewen <jiewen.yao@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; > Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer > detection > > If NULL pointer detection is disabled, the code should be behave same with > before, and ClearLegacyMemory() is not needed to be called because DxeCore > will behave same with before to handle the first 4K page clear , right? > > > Thanks, > Star > -----Original Message----- > From: Wang, Jian J > Sent: Thursday, September 28, 2017 11:55 AM > To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, > Jiewen <jiewen.yao@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; > Wolman, Ayellet <ayellet.wolman@intel.com> > Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer > detection > > Clearing this block of memory has nothing to do with NULL pointer detection. > I'm not sure the extra check is necessary. > > > -----Original Message----- > > From: Zeng, Star > > Sent: Thursday, September 28, 2017 11:31 AM > > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org > > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek > > <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, > > Michael D <michael.d.kinney@intel.com>; Justen, Jordan L > > <jordan.l.justen@intel.com>; Wolman, Ayellet > > <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com> > > Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL > > pointer detection > > > > 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.wang@intel.com>; edk2-devel@lists.01.org > > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek > > <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, > > Michael D <michael.d.kinney@intel.com>; Justen, Jordan L > > <jordan.l.justen@intel.com>; Wolman, Ayellet > > <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@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.zeng@intel.com>; Dong, Eric > > <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen > > <jiewen.yao@intel.com>; Kinney, Michael D > > <michael.d.kinney@intel.com>; Justen, Jordan L > > <jordan.l.justen@intel.com>; Wolman, Ayellet > > <ayellet.wolman@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.zeng@intel.com> > > Cc: Eric Dong <eric.dong@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Michael Kinney <michael.d.kinney@intel.com> > > Cc: Jordan Justen <jordan.l.justen@intel.com> > > Cc: Ayellet Wolman <ayellet.wolman@intel.com> > > Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jian J Wang <jian.j.wang@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 > > IndexOfPageDirectoryEntries+++= > > 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 > > IndexOfPageDirectoryEntries+++= > > 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
Please see my comments inline below. > -----Original Message----- > From: Zeng, Star > Sent: Thursday, September 28, 2017 11:24 AM > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, > Jiewen <jiewen.yao@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; > Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@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. > You're right. The PCD should not be under this section. > 2. How about using name "ClearFirst4KPage" instead of "ClearLegacyMemory" > to be more clear? > I prefer a bit more general name in case of potential future change. > > 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.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo > Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, > Michael D <michael.d.kinney@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@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.zeng@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Michael Kinney <michael.d.kinney@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Ayellet Wolman <ayellet.wolman@intel.com> > Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <jian.j.wang@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
I am curious about what is potential future change. Thanks, Star -----Original Message----- From: Wang, Jian J Sent: Thursday, September 28, 2017 11:50 AM To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com> Subject: RE: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection Please see my comments inline below. > -----Original Message----- > From: Zeng, Star > Sent: Thursday, September 28, 2017 11:24 AM > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek > <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, > Michael D <michael.d.kinney@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; Wolman, Ayellet > <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@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. > You're right. The PCD should not be under this section. > 2. How about using name "ClearFirst4KPage" instead of "ClearLegacyMemory" > to be more clear? > I prefer a bit more general name in case of potential future change. > > 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.zeng@intel.com>; Dong, Eric > <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen > <jiewen.yao@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com>; Justen, Jordan L > <jordan.l.justen@intel.com>; Wolman, Ayellet > <ayellet.wolman@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.zeng@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Michael Kinney <michael.d.kinney@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Ayellet Wolman <ayellet.wolman@intel.com> > Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <jian.j.wang@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 > IndexOfPageDirectoryEntries+++= > 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 > IndexOfPageDirectoryEntries+++= > 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
© 2016 - 2024 Red Hat, Inc.