* [PATCH v3 1/6] MdeModulePkg/MdeModulePkg.dec, .uni: Add NULL pointer detection PCD
2017-09-28 1:03 [PATCH v3 0/6] Add NULL pointer detection feature Jian J Wang
@ 2017-09-28 1:03 ` Jian J Wang
2017-09-28 3:35 ` Zeng, Star
2017-09-28 1:03 ` [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection Jian J Wang
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Jian J Wang @ 2017-09-28 1:03 UTC (permalink / raw)
To: edk2-devel
Cc: Wang, Jian J, Star Zeng, Eric Dong, Laszlo Ersek, Jiewen Yao,
Michael Kinney, Jordan Justen, Ayellet Wolman
From: "Wang, Jian J" <jian.j.wang@intel.com>
> According to Star's feedback, add prompt and help string in uni file
PCD PcdNullPointerDetectionPropertyMask is a bitmask used to control the
NULL address detection functionality in code for different phases.
If enabled, accessing NULL address in UEFI or SMM code can be caught
as a page fault exception.
BIT0 - Enable NULL pointer detection for UEFI.
BIT1 - Enable NULL pointer detection for SMM.
BIT2..6 - Reserved for future uses.
BIT7 - Disable NULL pointer detection just after EndOfDxe. This is a
workaround for those unsolvable NULL access issues in
OptionROM, boot loader, etc. It can also help to avoid
unnecessary exception caused by legacy memory (0-4095) access
after EndOfDxe, such as Windows 7 boot on Qemu.
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/MdeModulePkg.dec | 13 +++++++++++++
MdeModulePkg/MdeModulePkg.uni | 13 +++++++++++++
2 files changed, 26 insertions(+)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index a3c0633ee1..9248d10da8 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -867,6 +867,19 @@
# @ValidList 0x80000006 | 0x03058002
gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable|0x03058002|UINT32|0x30001040
+ ## Mask to control the NULL address detection in code for different phases.
+ # If enabled, accessing NULL address in UEFI or SMM code can be caught.<BR><BR>
+ # BIT0 - Enable NULL pointer detection for UEFI.<BR>
+ # BIT1 - Enable NULL pointer detection for SMM.<BR>
+ # BIT2..6 - Reserved for future uses.<BR>
+ # BIT7 - Disable NULL pointer detection just after EndOfDxe. <BR>
+ # This is a workaround for those unsolvable NULL access issues in
+ # OptionROM, boot loader, etc. It can also help to avoid unnecessary
+ # exception caused by legacy memory (0-4095) access after EndOfDxe,
+ # such as Windows 7 boot on Qemu.<BR>
+ # @Prompt Enable NULL address detection.
+ gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask|0x0|UINT8|0x30001050
+
[PcdsFixedAtBuild, PcdsPatchableInModule]
## Dynamic type PCD can be registered callback function for Pcd setting action.
# PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function
diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni
index d6015de75f..f8b31694ba 100644
--- a/MdeModulePkg/MdeModulePkg.uni
+++ b/MdeModulePkg/MdeModulePkg.uni
@@ -1127,3 +1127,16 @@
"enabled on AMD processors supporting the Secure Encrypted Virtualization (SEV) feature.\n"
"This mask should be applied when creating 1:1 virtual to physical mapping tables."
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdNullPointerDetectionPropertyMask_PROMPT #language en-US "Enable NULL pointer detection"
+
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdNullPointerDetectionPropertyMask_HELP #language en-US "Mask to control the NULL address detection in code for different phases.\n"
+ " If enabled, accessing NULL address in UEFI or SMM code can be caught.\n\n"
+ " BIT0 - Enable NULL pointer detection for UEFI.\n"
+ " BIT1 - Enable NULL pointer detection for SMM.\n"
+ " BIT2..6 - Reserved for future uses.\n"
+ " BIT7 - Disable NULL pointer detection just after EndOfDxe."
+ " This is a workaround for those unsolvable NULL access issues in"
+ " OptionROM, boot loader, etc. It can also help to avoid unnecessary"
+ " exception caused by legacy memory (0-4095) access after EndOfDxe,"
+ " such as Windows 7 boot on Qemu.\n"
+
--
2.14.1.windows.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 1/6] MdeModulePkg/MdeModulePkg.dec, .uni: Add NULL pointer detection PCD
2017-09-28 1:03 ` [PATCH v3 1/6] MdeModulePkg/MdeModulePkg.dec, .uni: Add NULL pointer detection PCD Jian J Wang
@ 2017-09-28 3:35 ` Zeng, Star
0 siblings, 0 replies; 19+ messages in thread
From: Zeng, Star @ 2017-09-28 3:35 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Dong, Eric, Laszlo Ersek, Yao, Jiewen, Kinney, Michael D,
Justen, Jordan L, Wolman, Ayellet, Zeng, Star
Reviewed-by: Star Zeng <star.zeng@intel.com>
-----Original Message-----
From: Wang, Jian J
Sent: Thursday, September 28, 2017 9:04 AM
To: edk2-devel@lists.01.org
Cc: Wang, Jian J <jian.j.wang@intel.com>; 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 1/6] MdeModulePkg/MdeModulePkg.dec,.uni: Add NULL pointer detection PCD
From: "Wang, Jian J" <jian.j.wang@intel.com>
> According to Star's feedback, add prompt and help string in uni file
PCD PcdNullPointerDetectionPropertyMask is a bitmask used to control the NULL address detection functionality in code for different phases.
If enabled, accessing NULL address in UEFI or SMM code can be caught as a page fault exception.
BIT0 - Enable NULL pointer detection for UEFI.
BIT1 - Enable NULL pointer detection for SMM.
BIT2..6 - Reserved for future uses.
BIT7 - Disable NULL pointer detection just after EndOfDxe. This is a
workaround for those unsolvable NULL access issues in
OptionROM, boot loader, etc. It can also help to avoid
unnecessary exception caused by legacy memory (0-4095) access
after EndOfDxe, such as Windows 7 boot on Qemu.
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/MdeModulePkg.dec | 13 +++++++++++++ MdeModulePkg/MdeModulePkg.uni | 13 +++++++++++++
2 files changed, 26 insertions(+)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index a3c0633ee1..9248d10da8 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -867,6 +867,19 @@
# @ValidList 0x80000006 | 0x03058002
gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable|0x03058002|UINT32|0x30001040
+ ## Mask to control the NULL address detection in code for different phases.
+ # If enabled, accessing NULL address in UEFI or SMM code can be caught.<BR><BR>
+ # BIT0 - Enable NULL pointer detection for UEFI.<BR>
+ # BIT1 - Enable NULL pointer detection for SMM.<BR>
+ # BIT2..6 - Reserved for future uses.<BR>
+ # BIT7 - Disable NULL pointer detection just after EndOfDxe. <BR>
+ # This is a workaround for those unsolvable NULL access issues in
+ # OptionROM, boot loader, etc. It can also help to avoid unnecessary
+ # exception caused by legacy memory (0-4095) access after EndOfDxe,
+ # such as Windows 7 boot on Qemu.<BR>
+ # @Prompt Enable NULL address detection.
+
+ gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask|0x0
+ |UINT8|0x30001050
+
[PcdsFixedAtBuild, PcdsPatchableInModule]
## Dynamic type PCD can be registered callback function for Pcd setting action.
# PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni index d6015de75f..f8b31694ba 100644
--- a/MdeModulePkg/MdeModulePkg.uni
+++ b/MdeModulePkg/MdeModulePkg.uni
@@ -1127,3 +1127,16 @@
"enabled on AMD processors supporting the Secure Encrypted Virtualization (SEV) feature.\n"
"This mask should be applied when creating 1:1 virtual to physical mapping tables."
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdNullPointerDetectionPropertyMask_PROMPT #language en-US "Enable NULL pointer detection"
+
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdNullPointerDetectionPropertyMask_HELP #language en-US "Mask to control the NULL address detection in code for different phases.\n"
+ " If enabled, accessing NULL address in UEFI or SMM code can be caught.\n\n"
+ " BIT0 - Enable NULL pointer detection for UEFI.\n"
+ " BIT1 - Enable NULL pointer detection for SMM.\n"
+ " BIT2..6 - Reserved for future uses.\n"
+ " BIT7 - Disable NULL pointer detection just after EndOfDxe."
+ " This is a workaround for those unsolvable NULL access issues in"
+ " OptionROM, boot loader, etc. It can also help to avoid unnecessary"
+ " exception caused by legacy memory (0-4095) access after EndOfDxe,"
+ " such as Windows 7 boot on Qemu.\n"
+
--
2.14.1.windows.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
2017-09-28 1:03 [PATCH v3 0/6] Add NULL pointer detection feature Jian J Wang
2017-09-28 1:03 ` [PATCH v3 1/6] MdeModulePkg/MdeModulePkg.dec, .uni: Add NULL pointer detection PCD Jian J Wang
@ 2017-09-28 1:03 ` Jian J Wang
2017-09-28 3:23 ` Zeng, Star
2017-09-28 1:03 ` [PATCH v3 3/6] MdeModulePkg/Core/Dxe: Add EndOfDxe workaround Jian J Wang
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Jian J Wang @ 2017-09-28 1:03 UTC (permalink / raw)
To: edk2-devel
Cc: Star Zeng, Eric Dong, Laszlo Ersek, Jiewen Yao, Michael Kinney,
Jordan Justen, Ayellet Wolman
> 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
2017-09-28 1:03 ` [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection Jian J Wang
@ 2017-09-28 3:23 ` Zeng, Star
2017-09-28 3:31 ` Zeng, Star
2017-09-28 3:50 ` Wang, Jian J
0 siblings, 2 replies; 19+ messages in thread
From: Zeng, Star @ 2017-09-28 3:23 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Dong, Eric, Laszlo Ersek, Yao, Jiewen, Kinney, Michael D,
Justen, Jordan L, Wolman, Ayellet, Zeng, Star
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
2017-09-28 3:23 ` Zeng, Star
@ 2017-09-28 3:31 ` Zeng, Star
2017-09-28 3:55 ` Wang, Jian J
2017-09-28 3:50 ` Wang, Jian J
1 sibling, 1 reply; 19+ messages in thread
From: Zeng, Star @ 2017-09-28 3:31 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Dong, Eric, Laszlo Ersek, Yao, Jiewen, Kinney, Michael D,
Justen, Jordan L, Wolman, Ayellet, Zeng, Star
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
2017-09-28 3:31 ` Zeng, Star
@ 2017-09-28 3:55 ` Wang, Jian J
2017-09-28 5:09 ` Zeng, Star
0 siblings, 1 reply; 19+ messages in thread
From: Wang, Jian J @ 2017-09-28 3:55 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org
Cc: Dong, Eric, Laszlo Ersek, Yao, Jiewen, Kinney, Michael D,
Justen, Jordan L, Wolman, Ayellet
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
2017-09-28 3:55 ` Wang, Jian J
@ 2017-09-28 5:09 ` Zeng, Star
2017-09-28 5:33 ` Wang, Jian J
0 siblings, 1 reply; 19+ messages in thread
From: Zeng, Star @ 2017-09-28 5:09 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Dong, Eric, Laszlo Ersek, Yao, Jiewen, Kinney, Michael D,
Justen, Jordan L, Wolman, Ayellet, Zeng, Star
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
2017-09-28 5:09 ` Zeng, Star
@ 2017-09-28 5:33 ` Wang, Jian J
0 siblings, 0 replies; 19+ messages in thread
From: Wang, Jian J @ 2017-09-28 5:33 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org
Cc: Dong, Eric, Laszlo Ersek, Yao, Jiewen, Kinney, Michael D,
Justen, Jordan L, Wolman, Ayellet
>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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
2017-09-28 3:23 ` Zeng, Star
2017-09-28 3:31 ` Zeng, Star
@ 2017-09-28 3:50 ` Wang, Jian J
2017-09-28 5:11 ` Zeng, Star
1 sibling, 1 reply; 19+ messages in thread
From: Wang, Jian J @ 2017-09-28 3:50 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org
Cc: Dong, Eric, Laszlo Ersek, Yao, Jiewen, Kinney, Michael D,
Justen, Jordan L, Wolman, Ayellet
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection
2017-09-28 3:50 ` Wang, Jian J
@ 2017-09-28 5:11 ` Zeng, Star
0 siblings, 0 replies; 19+ messages in thread
From: Zeng, Star @ 2017-09-28 5:11 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Dong, Eric, Laszlo Ersek, Yao, Jiewen, Kinney, Michael D,
Justen, Jordan L, Wolman, Ayellet, Zeng, Star
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 3/6] MdeModulePkg/Core/Dxe: Add EndOfDxe workaround
2017-09-28 1:03 [PATCH v3 0/6] Add NULL pointer detection feature Jian J Wang
2017-09-28 1:03 ` [PATCH v3 1/6] MdeModulePkg/MdeModulePkg.dec, .uni: Add NULL pointer detection PCD Jian J Wang
2017-09-28 1:03 ` [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection Jian J Wang
@ 2017-09-28 1:03 ` Jian J Wang
2017-09-28 3:34 ` Zeng, Star
2017-09-28 1:03 ` [PATCH v3 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM code Jian J Wang
` (2 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Jian J Wang @ 2017-09-28 1:03 UTC (permalink / raw)
To: edk2-devel
Cc: Star Zeng, Eric Dong, Laszlo Ersek, Jiewen Yao, Michael Kinney,
Jordan Justen, Ayellet Wolman
One of issue caused by enabling NULL pointer detection is that some PCI
device OptionROM, binary drivers and binary OS boot loaders may have NULL
pointer access bugs, which will prevent BIOS from booting and is almost
impossible to fix. BIT7 of PCD PcdNullPointerDetectionPropertyMask is used
as a workaround to indicate BIOS to disable NULL pointer detection right
after event gEfiEndOfDxeEventGroupGuid, and then let boot continue.
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/Dxe/DxeMain.inf | 1 +
MdeModulePkg/Core/Dxe/Mem/Page.c | 4 ++-
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 48 +++++++++++++++++++++++++++
3 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 30d5984f7c..0a161ffd71 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -192,6 +192,7 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ## CONSUMES
# [Hob]
# RESOURCE_DESCRIPTOR ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index a142c79ee2..0468df3171 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -188,7 +188,9 @@ CoreAddRange (
// used for other purposes.
//
if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 1)) {
- SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+ if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+ SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+ }
}
//
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index a73c4ccd64..73e3b269f3 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -995,6 +995,36 @@ MemoryProtectionExitBootServicesCallback (
}
}
+/**
+ Disable NULL pointer detection after EndOfDxe. This is a workaround resort in
+ order to skip unfixable NULL pointer access issues detected in OptionROM or
+ boot loaders.
+
+ @param[in] Event The Event this notify function registered to.
+ @param[in] Context Pointer to the context data registered to the Event.
+**/
+VOID
+EFIAPI
+DisableNullDetectionAtTheEndOfDxe (
+ EFI_EVENT Event,
+ VOID *Context
+ )
+{
+ EFI_STATUS Status;
+
+ DEBUG ((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): start\r\n"));
+ //
+ // Disable NULL pointer detection by enabling first 4K page
+ //
+ Status = gCpu->SetMemoryAttributes (gCpu, 0, EFI_PAGE_SIZE, 0);
+ ASSERT_EFI_ERROR (Status);
+
+ CoreCloseEvent (Event);
+ DEBUG ((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): end\r\n"));
+
+ return;
+}
+
/**
Initialize Memory Protection support.
**/
@@ -1006,6 +1036,7 @@ CoreInitializeMemoryProtection (
{
EFI_STATUS Status;
EFI_EVENT Event;
+ EFI_EVENT EndOfDxeEvent;
VOID *Registration;
mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
@@ -1044,6 +1075,23 @@ CoreInitializeMemoryProtection (
);
ASSERT_EFI_ERROR(Status);
}
+
+ //
+ // Register a callback to disable NULL pointer detection at EndOfDxe
+ //
+ if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7))
+ == (BIT0|BIT7)) {
+ Status = CoreCreateEventEx (
+ EVT_NOTIFY_SIGNAL,
+ TPL_NOTIFY,
+ DisableNullDetectionAtTheEndOfDxe,
+ NULL,
+ &gEfiEndOfDxeEventGroupGuid,
+ &EndOfDxeEvent
+ );
+ ASSERT_EFI_ERROR (Status);
+ }
+
return ;
}
--
2.14.1.windows.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/6] MdeModulePkg/Core/Dxe: Add EndOfDxe workaround
2017-09-28 1:03 ` [PATCH v3 3/6] MdeModulePkg/Core/Dxe: Add EndOfDxe workaround Jian J Wang
@ 2017-09-28 3:34 ` Zeng, Star
2017-09-28 5:08 ` Wang, Jian J
0 siblings, 1 reply; 19+ messages in thread
From: Zeng, Star @ 2017-09-28 3:34 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Dong, Eric, Laszlo Ersek, Yao, Jiewen, Kinney, Michael D,
Justen, Jordan L, Wolman, Ayellet, Zeng, Star
Some comments to this patch.
1. How about using lower TPL TPL_CALLBACK instead of TPL_NOTIFY for the notification?
2. Should GCD SetMemorySpaceCapabilities + SetMemorySpaceAttributes be used instead of gCpu->SetMemoryAttributes()?
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 3/6] MdeModulePkg/Core/Dxe: Add EndOfDxe workaround
One of issue caused by enabling NULL pointer detection is that some PCI device OptionROM, binary drivers and binary OS boot loaders may have NULL pointer access bugs, which will prevent BIOS from booting and is almost impossible to fix. BIT7 of PCD PcdNullPointerDetectionPropertyMask is used as a workaround to indicate BIOS to disable NULL pointer detection right after event gEfiEndOfDxeEventGroupGuid, and then let boot continue.
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/Dxe/DxeMain.inf | 1 +
MdeModulePkg/Core/Dxe/Mem/Page.c | 4 ++-
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 48 +++++++++++++++++++++++++++
3 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 30d5984f7c..0a161ffd71 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -192,6 +192,7 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ## CONSUMES
# [Hob]
# RESOURCE_DESCRIPTOR ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index a142c79ee2..0468df3171 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -188,7 +188,9 @@ CoreAddRange (
// used for other purposes.
//
if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE - 1)) {
- SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+ if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+ SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
+ }
}
//
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index a73c4ccd64..73e3b269f3 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -995,6 +995,36 @@ MemoryProtectionExitBootServicesCallback (
}
}
+/**
+ Disable NULL pointer detection after EndOfDxe. This is a workaround
+resort in
+ order to skip unfixable NULL pointer access issues detected in
+OptionROM or
+ boot loaders.
+
+ @param[in] Event The Event this notify function registered to.
+ @param[in] Context Pointer to the context data registered to the Event.
+**/
+VOID
+EFIAPI
+DisableNullDetectionAtTheEndOfDxe (
+ EFI_EVENT Event,
+ VOID *Context
+ )
+{
+ EFI_STATUS Status;
+
+ DEBUG ((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe():
+ start\r\n")); // // Disable NULL pointer detection by enabling first
+ 4K page // Status = gCpu->SetMemoryAttributes (gCpu, 0,
+ EFI_PAGE_SIZE, 0); ASSERT_EFI_ERROR (Status);
+
+ CoreCloseEvent (Event);
+ DEBUG ((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): end\r\n"));
+
+ return;
+}
+
/**
Initialize Memory Protection support.
**/
@@ -1006,6 +1036,7 @@ CoreInitializeMemoryProtection ( {
EFI_STATUS Status;
EFI_EVENT Event;
+ EFI_EVENT EndOfDxeEvent;
VOID *Registration;
mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
@@ -1044,6 +1075,23 @@ CoreInitializeMemoryProtection (
);
ASSERT_EFI_ERROR(Status);
}
+
+ //
+ // Register a callback to disable NULL pointer detection at EndOfDxe
+ // if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7))
+ == (BIT0|BIT7)) {
+ Status = CoreCreateEventEx (
+ EVT_NOTIFY_SIGNAL,
+ TPL_NOTIFY,
+ DisableNullDetectionAtTheEndOfDxe,
+ NULL,
+ &gEfiEndOfDxeEventGroupGuid,
+ &EndOfDxeEvent
+ );
+ ASSERT_EFI_ERROR (Status);
+ }
+
return ;
}
--
2.14.1.windows.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/6] MdeModulePkg/Core/Dxe: Add EndOfDxe workaround
2017-09-28 3:34 ` Zeng, Star
@ 2017-09-28 5:08 ` Wang, Jian J
0 siblings, 0 replies; 19+ messages in thread
From: Wang, Jian J @ 2017-09-28 5:08 UTC (permalink / raw)
To: Zeng, Star, edk2-devel@lists.01.org
Cc: Dong, Eric, Laszlo Ersek, Yao, Jiewen, Kinney, Michael D,
Justen, Jordan L, Wolman, Ayellet
Thanks for the feedback. Please see my comments below.
> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 11:35 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 3/6] MdeModulePkg/Core/Dxe: Add EndOfDxe
> workaround
>
> Some comments to this patch.
>
> 1. How about using lower TPL TPL_CALLBACK instead of TPL_NOTIFY for the
> notification?
I think it's safe to use TPL_CALLBACK.
> 2. Should GCD SetMemorySpaceCapabilities + SetMemorySpaceAttributes be
> used instead of gCpu->SetMemoryAttributes()?
Yes. Since the GCG out-of-sync issue has been fixed, GCD service
should be used instead.
>
> 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 3/6] MdeModulePkg/Core/Dxe: Add EndOfDxe workaround
>
> One of issue caused by enabling NULL pointer detection is that some PCI device
> OptionROM, binary drivers and binary OS boot loaders may have NULL pointer
> access bugs, which will prevent BIOS from booting and is almost impossible to
> fix. BIT7 of PCD PcdNullPointerDetectionPropertyMask is used as a workaround
> to indicate BIOS to disable NULL pointer detection right after event
> gEfiEndOfDxeEventGroupGuid, and then let boot continue.
>
> 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/Dxe/DxeMain.inf | 1 +
> MdeModulePkg/Core/Dxe/Mem/Page.c | 4 ++-
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 48
> +++++++++++++++++++++++++++
> 3 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index 30d5984f7c..0a161ffd71 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -192,6 +192,7 @@
> gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable ##
> CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ##
> CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy
> ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> ## CONSUMES
>
> # [Hob]
> # RESOURCE_DESCRIPTOR ## CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index a142c79ee2..0468df3171 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -188,7 +188,9 @@ CoreAddRange (
> // used for other purposes.
> //
> if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE -
> 1)) {
> - SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> + if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
> + SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> + }
> }
>
> //
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index a73c4ccd64..73e3b269f3 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -995,6 +995,36 @@ MemoryProtectionExitBootServicesCallback (
> }
> }
>
> +/**
> + Disable NULL pointer detection after EndOfDxe. This is a workaround
> +resort in
> + order to skip unfixable NULL pointer access issues detected in
> +OptionROM or
> + boot loaders.
> +
> + @param[in] Event The Event this notify function registered to.
> + @param[in] Context Pointer to the context data registered to the Event.
> +**/
> +VOID
> +EFIAPI
> +DisableNullDetectionAtTheEndOfDxe (
> + EFI_EVENT Event,
> + VOID *Context
> + )
> +{
> + EFI_STATUS Status;
> +
> + DEBUG ((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe():
> + start\r\n")); // // Disable NULL pointer detection by enabling first
> + 4K page // Status = gCpu->SetMemoryAttributes (gCpu, 0,
> + EFI_PAGE_SIZE, 0); ASSERT_EFI_ERROR (Status);
> +
> + CoreCloseEvent (Event);
> + DEBUG ((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): end\r\n"));
> +
> + return;
> +}
> +
> /**
> Initialize Memory Protection support.
> **/
> @@ -1006,6 +1036,7 @@ CoreInitializeMemoryProtection ( {
> EFI_STATUS Status;
> EFI_EVENT Event;
> + EFI_EVENT EndOfDxeEvent;
> VOID *Registration;
>
> mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
> @@ -1044,6 +1075,23 @@ CoreInitializeMemoryProtection (
> );
> ASSERT_EFI_ERROR(Status);
> }
> +
> + //
> + // Register a callback to disable NULL pointer detection at EndOfDxe
> + // if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7))
> + == (BIT0|BIT7)) {
> + Status = CoreCreateEventEx (
> + EVT_NOTIFY_SIGNAL,
> + TPL_NOTIFY,
> + DisableNullDetectionAtTheEndOfDxe,
> + NULL,
> + &gEfiEndOfDxeEventGroupGuid,
> + &EndOfDxeEvent
> + );
> + ASSERT_EFI_ERROR (Status);
> + }
> +
> return ;
> }
>
> --
> 2.14.1.windows.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM code
2017-09-28 1:03 [PATCH v3 0/6] Add NULL pointer detection feature Jian J Wang
` (2 preceding siblings ...)
2017-09-28 1:03 ` [PATCH v3 3/6] MdeModulePkg/Core/Dxe: Add EndOfDxe workaround Jian J Wang
@ 2017-09-28 1:03 ` Jian J Wang
2017-09-28 1:03 ` [PATCH v3 5/6] IntelFrameworkModulePkg/Csm: Add code to bypass NULL pointer detection Jian J Wang
2017-09-28 1:03 ` [PATCH v3 6/6] OvmfPkg/QemuVideoDxe: Bypass NULL pointer detection during VBE SHIM installing Jian J Wang
5 siblings, 0 replies; 19+ messages in thread
From: Jian J Wang @ 2017-09-28 1:03 UTC (permalink / raw)
To: edk2-devel
Cc: Star Zeng, Eric Dong, Laszlo Ersek, Jiewen Yao, Michael Kinney,
Jordan Justen, Ayellet Wolman
The mechanism behind is the same as NULL pointer detection enabled in EDK-II
core. SMM has its own page table and we have to disable page 0 again in SMM
mode.
Cc: 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>
---
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 12 ++++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 25 ++++++++++++++++++++++++-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 12 ++++++++++++
4 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index f295c2ebf2..1c9e239a34 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -155,6 +155,18 @@ SmiPFHandler (
}
}
+ //
+ // If NULL pointer was just accessed
+ //
+ if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0 &&
+ (PFAddress < EFI_PAGE_SIZE)) {
+ DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
+ DEBUG_CODE (
+ DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Rip);
+ );
+ CpuDeadLoop ();
+ }
+
if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
SmmProfilePFHandler (
SystemContext.SystemContextIa32->Eip,
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index f086b97c30..ed2afadb21 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -855,10 +855,10 @@ Gen4GPageTable (
Pte[Index] = (Index << 21) | mAddressEncMask | IA32_PG_PS | PAGE_ATTRIBUTE_BITS;
}
+ Pdpte = (UINT64*)PageTable;
if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
Pages = (UINTN)PageTable + EFI_PAGES_TO_SIZE (5);
GuardPage = mSmmStackArrayBase + EFI_PAGE_SIZE;
- Pdpte = (UINT64*)PageTable;
for (PageIndex = Low2MBoundary; PageIndex <= High2MBoundary; PageIndex += SIZE_2MB) {
Pte = (UINT64*)(UINTN)(Pdpte[BitFieldRead32 ((UINT32)PageIndex, 30, 31)] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));
Pte[BitFieldRead32 ((UINT32)PageIndex, 21, 29)] = (UINT64)Pages | mAddressEncMask | PAGE_ATTRIBUTE_BITS;
@@ -886,6 +886,29 @@ Gen4GPageTable (
}
}
+ if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0) {
+ Pte = (UINT64*)(UINTN)(Pdpte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));
+ if ((Pte[0] & IA32_PG_PS) == 0) {
+ // 4K-page entries are already mapped. Just hide the first one anyway.
+ Pte = (UINT64*)(UINTN)(Pte[0] & ~mAddressEncMask & ~(EFI_PAGE_SIZE - 1));
+ Pte[0] &= ~1; // Hide page 0
+ } else {
+ // Create 4K-page entries
+ Pages = (UINTN)AllocatePageTableMemory (1);
+ ASSERT (Pages != 0);
+
+ Pte[0] = (UINT64)(Pages | mAddressEncMask | PAGE_ATTRIBUTE_BITS);
+
+ Pte = (UINT64*)Pages;
+ PageAddress = 0;
+ Pte[0] = PageAddress | mAddressEncMask; // Hide page 0 but present left
+ for (Index = 1; Index < EFI_PAGE_SIZE / sizeof (*Pte); Index++) {
+ PageAddress += EFI_PAGE_SIZE;
+ Pte[Index] = PageAddress | mAddressEncMask | PAGE_ATTRIBUTE_BITS;
+ }
+ }
+ }
+
return (UINT32)(UINTN)PageTable;
}
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 099792e6ce..31cb215342 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -159,6 +159,7 @@
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStaticPageTable ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ## CONSUMES
[Depex]
gEfiMpServiceProtocolGuid
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 3dde80f9ba..f3791ce897 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -872,6 +872,18 @@ SmiPFHandler (
}
}
+ //
+ // If NULL pointer was just accessed
+ //
+ if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0 &&
+ (PFAddress < EFI_PAGE_SIZE)) {
+ DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
+ DEBUG_CODE (
+ DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip);
+ );
+ CpuDeadLoop ();
+ }
+
if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
SmmProfilePFHandler (
SystemContext.SystemContextX64->Rip,
--
2.14.1.windows.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 5/6] IntelFrameworkModulePkg/Csm: Add code to bypass NULL pointer detection
2017-09-28 1:03 [PATCH v3 0/6] Add NULL pointer detection feature Jian J Wang
` (3 preceding siblings ...)
2017-09-28 1:03 ` [PATCH v3 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Implement NULL pointer detection for SMM code Jian J Wang
@ 2017-09-28 1:03 ` Jian J Wang
2017-09-28 1:03 ` [PATCH v3 6/6] OvmfPkg/QemuVideoDxe: Bypass NULL pointer detection during VBE SHIM installing Jian J Wang
5 siblings, 0 replies; 19+ messages in thread
From: Jian J Wang @ 2017-09-28 1:03 UTC (permalink / raw)
To: edk2-devel
Cc: Star Zeng, Eric Dong, Laszlo Ersek, Jiewen Yao, Michael Kinney,
Jordan Justen, Ayellet Wolman
Legacy has to access interrupt vector, BDA, etc. located in memory between
0-4095. To allow as much code as possible to be monitered by NULL pointer
detection, we add code to temporarily disable this feature right before
those memory access and enable it again afterwards.
> According to Laszlo, get memory attributes before changing them.
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>
---
.../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c | 101 ++++++++++++++
.../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h | 2 +
.../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf | 2 +
.../Csm/LegacyBiosDxe/LegacyBda.c | 4 +
.../Csm/LegacyBiosDxe/LegacyBios.c | 152 +++++++++++++++++++++
.../Csm/LegacyBiosDxe/LegacyBiosDxe.inf | 2 +
.../Csm/LegacyBiosDxe/LegacyBiosInterface.h | 18 +++
.../Csm/LegacyBiosDxe/LegacyBootSupport.c | 23 +++-
.../Csm/LegacyBiosDxe/LegacyPci.c | 17 ++-
IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c | 27 +++-
10 files changed, 338 insertions(+), 10 deletions(-)
diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
index 7308523ad8..d2224a20aa 100644
--- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
+++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
@@ -1732,6 +1732,98 @@ CheckKeyboardConnect (
}
}
+/**
+ Disable NULL pointer detection
+*/
+VOID
+DisableNullDetection (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
+
+ if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+ return;
+ }
+
+ //
+ // Check current capabilities and attributes
+ //
+ Status = gDS->GetMemorySpaceDescriptor (0, &Desc);
+ ASSERT_EFI_ERROR (Status);
+
+ //
+ // Try to add EFI_MEMORY_RP support if necessary
+ //
+ if ((Desc.Capabilities & EFI_MEMORY_RP) == 0) {
+ Desc.Capabilities |= EFI_MEMORY_RP;
+ Status = gDS->SetMemorySpaceCapabilities (0, EFI_PAGES_TO_SIZE(1),
+ Desc.Capabilities);
+ ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ return;
+ }
+ }
+
+ //
+ // Don't bother if EFI_MEMORY_RP is already cleared.
+ //
+ if ((Desc.Attributes & EFI_MEMORY_RP) != 0) {
+ Desc.Attributes &= ~EFI_MEMORY_RP;
+ Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
+ Desc.Attributes);
+ ASSERT_EFI_ERROR (Status);
+ } else {
+ DEBUG ((DEBUG_WARN, "!!! Page 0 is supposed to be disabled !!!\r\n"));
+ }
+}
+
+/**
+ Enable NULL pointer detection
+*/
+VOID
+EnableNullDetection (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
+
+ if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+ return;
+ }
+
+ //
+ // Check current capabilities and attributes
+ //
+ Status = gDS->GetMemorySpaceDescriptor (0, &Desc);
+ ASSERT_EFI_ERROR (Status);
+
+ //
+ // Try to add EFI_MEMORY_RP support if necessary
+ //
+ if ((Desc.Capabilities & EFI_MEMORY_RP) == 0) {
+ Desc.Capabilities |= EFI_MEMORY_RP;
+ Status = gDS->SetMemorySpaceCapabilities (0, EFI_PAGES_TO_SIZE(1),
+ Desc.Capabilities);
+ ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ return;
+ }
+ }
+
+ //
+ // Don't bother if EFI_MEMORY_RP is already set.
+ //
+ if ((Desc.Attributes & EFI_MEMORY_RP) == 0) {
+ Desc.Attributes |= EFI_MEMORY_RP;
+ Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
+ Desc.Attributes);
+ ASSERT_EFI_ERROR (Status);
+ }
+}
+
/**
Timer event handler: read a series of key stroke from 8042
and put them into memory key buffer.
@@ -1839,6 +1931,11 @@ BiosKeyboardTimerHandler (
// 0 Right Shift pressed
+ //
+ // Disable NULL pointer detection temporarily
+ //
+ DisableNullDetection ();
+
//
// Clear the CTRL and ALT BDA flag
//
@@ -1916,6 +2013,10 @@ BiosKeyboardTimerHandler (
KbFlag1 &= ~0x0C;
*((UINT8 *) (UINTN) 0x417) = KbFlag1;
+ //
+ // Restore NULL pointer detection
+ //
+ EnableNullDetection ();
//
// Output EFI input key and shift/toggle state
diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
index 0bf28ea140..c64ec0095e 100644
--- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
+++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.h
@@ -18,6 +18,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <FrameworkDxe.h>
+#include <Pi/PiDxeCis.h>
#include <Guid/StatusCodeDataTypeId.h>
#include <Protocol/SimpleTextIn.h>
@@ -33,6 +34,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Library/ReportStatusCodeLib.h>
#include <Library/UefiDriverEntryPoint.h>
#include <Library/UefiBootServicesTableLib.h>
+#include <Library/DxeServicesTableLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/BaseLib.h>
#include <Library/PcdLib.h>
diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf
index 4d4536466c..596f4ced44 100644
--- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf
+++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf
@@ -60,6 +60,7 @@
DebugLib
BaseLib
PcdLib
+ DxeServicesTableLib
[Protocols]
gEfiIsaIoProtocolGuid ## TO_START
@@ -73,6 +74,7 @@
[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdFastPS2Detection ## SOMETIMES_CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ## CONSUMES
[UserExtensions.TianoCore."ExtraFiles"]
KeyboardDxeExtra.uni
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
index c45d5d4c3e..c6670febee 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
@@ -34,6 +34,8 @@ LegacyBiosInitBda (
BDA_STRUC *Bda;
UINT8 *Ebda;
+ DisableNullDetection ();
+
Bda = (BDA_STRUC *) ((UINTN) 0x400);
Ebda = (UINT8 *) ((UINTN) 0x9fc00);
@@ -62,5 +64,7 @@ LegacyBiosInitBda (
*Ebda = 0x01;
+ EnableNullDetection ();
+
return EFI_SUCCESS;
}
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
index 3ead2d9828..3176a989f6 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
@@ -40,6 +40,7 @@ VOID *mRuntimeSmbiosEntryPoint = NULL;
EFI_PHYSICAL_ADDRESS mReserveSmbiosEntryPoint = 0;
EFI_PHYSICAL_ADDRESS mStructureTableAddress = 0;
UINTN mStructureTablePages = 0;
+BOOLEAN mEndOfDxe = FALSE;
/**
Do an AllocatePages () of type AllocateMaxAddress for EfiBootServicesCode
@@ -765,6 +766,135 @@ InstallSmbiosEventCallback (
}
}
+/**
+ Callback function to toggle EndOfDxe status. NULL pointer detection needs
+ this status to decide if it's necessary to change attributes of page 0.
+
+ @param Event Event whose notification function is being invoked.
+ @param Context The pointer to the notification function's context,
+ which is implementation-dependent.
+
+**/
+VOID
+EFIAPI
+ToggleEndOfDxeStatus (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ mEndOfDxe = TRUE;
+ return;
+}
+
+//
+// Legacy BIOS needs to access memory between 0-4095, which will cause page
+// fault exception if NULL pointer detection mechanism is enabled. Following
+// functions can be used to disable/enable NULL pointer detection before/after
+// accessing those memory.
+//
+
+/**
+ Enable NULL pointer detection
+*/
+VOID
+EnableNullDetection (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
+
+ if (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0)
+ ||
+ ((mEndOfDxe == TRUE) &&
+ ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT7|BIT0))
+ == (BIT7|BIT0)))
+ ) {
+ return;
+ }
+
+ //
+ // Check current capabilities and attributes
+ //
+ Status = gDS->GetMemorySpaceDescriptor (0, &Desc);
+ ASSERT_EFI_ERROR (Status);
+
+ //
+ // Try to add EFI_MEMORY_RP support if necessary
+ //
+ if ((Desc.Capabilities & EFI_MEMORY_RP) == 0) {
+ Desc.Capabilities |= EFI_MEMORY_RP;
+ Status = gDS->SetMemorySpaceCapabilities (0, EFI_PAGES_TO_SIZE(1),
+ Desc.Capabilities);
+ ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ return;
+ }
+ }
+
+ //
+ // Don't bother if EFI_MEMORY_RP is already set.
+ //
+ if ((Desc.Attributes & EFI_MEMORY_RP) == 0) {
+ Desc.Attributes |= EFI_MEMORY_RP;
+ Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
+ Desc.Attributes);
+ ASSERT_EFI_ERROR (Status);
+ }
+}
+
+/**
+ Disable NULL pointer detection
+*/
+VOID
+DisableNullDetection (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
+
+ if (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0)
+ ||
+ ((mEndOfDxe == TRUE) &&
+ ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT7|BIT0))
+ == (BIT7|BIT0)))
+ ) {
+ return;
+ }
+
+ //
+ // Check current capabilities and attributes
+ //
+ Status = gDS->GetMemorySpaceDescriptor (0, &Desc);
+ ASSERT_EFI_ERROR (Status);
+
+ //
+ // Try to add EFI_MEMORY_RP support if necessary
+ //
+ if ((Desc.Capabilities & EFI_MEMORY_RP) == 0) {
+ Desc.Capabilities |= EFI_MEMORY_RP;
+ Status = gDS->SetMemorySpaceCapabilities (0, EFI_PAGES_TO_SIZE(1),
+ Desc.Capabilities);
+ ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ return;
+ }
+ }
+
+ //
+ // Don't bother if EFI_MEMORY_RP is already cleared.
+ //
+ if ((Desc.Attributes & EFI_MEMORY_RP) != 0) {
+ Desc.Attributes &= ~EFI_MEMORY_RP;
+ Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
+ Desc.Attributes);
+ ASSERT_EFI_ERROR (Status);
+ } else {
+ DEBUG ((DEBUG_WARN, "!!! Page 0 is supposed to be disabled !!!\r\n"));
+ }
+}
+
/**
Install Driver to produce Legacy BIOS protocol.
@@ -802,6 +932,7 @@ LegacyBiosInstall (
UINT64 Length;
UINT8 *SecureBoot;
EFI_EVENT InstallSmbiosEvent;
+ EFI_EVENT EndOfDxeEvent;
//
// Load this driver's image to memory
@@ -964,8 +1095,10 @@ LegacyBiosInstall (
// Initialize region from 0x0000 to 4k. This initializes interrupt vector
// range.
//
+ DisableNullDetection ();
gBS->SetMem ((VOID *) ClearPtr, 0x400, INITIAL_VALUE_BELOW_1K);
ZeroMem ((VOID *) ((UINTN)ClearPtr + 0x400), 0xC00);
+ EnableNullDetection ();
//
// Allocate pages for OPROM usage
@@ -1104,12 +1237,17 @@ LegacyBiosInstall (
//
// Save Unexpected interrupt vector so can restore it just prior to boot
//
+ DisableNullDetection ();
+
BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
Private->BiosUnexpectedInt = BaseVectorMaster[0];
IntRedirCode = (UINT32) (UINTN) Private->IntThunk->InterruptRedirectionCode;
for (Index = 0; Index < 8; Index++) {
BaseVectorMaster[Index] = (EFI_SEGMENT (IntRedirCode + Index * 4) << 16) | EFI_OFFSET (IntRedirCode + Index * 4);
}
+
+ EnableNullDetection ();
+
//
// Save EFI value
//
@@ -1133,6 +1271,20 @@ LegacyBiosInstall (
);
ASSERT_EFI_ERROR (Status);
+ //
+ // Create callback to update status of EndOfDxe, which is needed by NULL
+ // pointer detection
+ //
+ Status = gBS->CreateEventEx (
+ EVT_NOTIFY_SIGNAL,
+ TPL_NOTIFY,
+ ToggleEndOfDxeStatus,
+ NULL,
+ &gEfiEndOfDxeEventGroupGuid,
+ &EndOfDxeEvent
+ );
+ ASSERT_EFI_ERROR (Status);
+
//
// Make a new handle and install the protocol
//
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
index 48473a0713..6efc7f36ae 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
@@ -108,6 +108,7 @@
gEfiDiskInfoIdeInterfaceGuid ## SOMETIMES_CONSUMES ##GUID #Used in LegacyBiosBuildIdeData() to assure device is a disk
gEfiSmbiosTableGuid ## SOMETIMES_CONSUMES ##SystemTable
gEfiLegacyBiosGuid ## SOMETIMES_CONSUMES ##GUID #Used in LegacyBiosInstallVgaRom() to locate handle buffer
+ gEfiEndOfDxeEventGroupGuid ## CONSUMES
[Guids.IA32]
gEfiAcpi20TableGuid ## SOMETIMES_CONSUMES ##SystemTable
@@ -147,6 +148,7 @@
gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdHighPmmMemorySize ## CONSUMES
gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdOpromReservedMemoryBase ## CONSUMES
gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdOpromReservedMemorySize ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ## CONSUMES
[Depex]
gEfiLegacyRegion2ProtocolGuid AND gEfiLegacyInterruptProtocolGuid AND gEfiLegacyBiosPlatformProtocolGuid AND gEfiLegacy8259ProtocolGuid AND gEfiGenericMemTestProtocolGuid AND gEfiCpuArchProtocolGuid AND gEfiTimerArchProtocolGuid AND gEfiVariableWriteArchProtocolGuid
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h
index fe9dd7463a..20dfef3fec 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h
@@ -509,6 +509,8 @@ extern BBS_TABLE *mBbsTable;
extern EFI_GENERIC_MEMORY_TEST_PROTOCOL *gGenMemoryTest;
+extern BOOLEAN mEndOfDxe;
+
#define PORT_70 0x70
#define PORT_71 0x71
@@ -1542,4 +1544,20 @@ LegacyBiosInstallVgaRom (
IN LEGACY_BIOS_INSTANCE *Private
);
+/**
+ Enable NULL pointer detection
+*/
+VOID
+EnableNullDetection (
+ VOID
+ );
+
+/**
+ Disable NULL pointer detection
+*/
+VOID
+DisableNullDetection (
+ VOID
+ );
+
#endif
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
index 1e098b3726..c2ac69ce69 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
@@ -1073,8 +1073,10 @@ GenericLegacyBoot (
// Use 182/10 to avoid floating point math.
//
LocalTime = (LocalTime * 182) / 10;
+ DisableNullDetection ();
BdaPtr = (UINT32 *) (UINTN)0x46C;
*BdaPtr = LocalTime;
+ EnableNullDetection ();
//
// Shadow PCI ROMs. We must do this near the end since this will kick
@@ -1320,6 +1322,7 @@ GenericLegacyBoot (
// set of TIANO vectors) or takes it over.
//
//
+ DisableNullDetection ();
BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
for (Index = 0; Index < 8; Index++) {
Private->ThunkSavedInt[Index] = BaseVectorMaster[Index];
@@ -1327,6 +1330,7 @@ GenericLegacyBoot (
BaseVectorMaster[Index] = (UINT32) (Private->BiosUnexpectedInt);
}
}
+ EnableNullDetection ();
ZeroMem (&Regs, sizeof (EFI_IA32_REGISTER_SET));
Regs.X.AX = Legacy16Boot;
@@ -1340,10 +1344,12 @@ GenericLegacyBoot (
0
);
+ DisableNullDetection ();
BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
for (Index = 0; Index < 8; Index++) {
BaseVectorMaster[Index] = Private->ThunkSavedInt[Index];
}
+ EnableNullDetection ();
}
Private->LegacyBootEntered = TRUE;
if ((mBootMode == BOOT_LEGACY_OS) || (mBootMode == BOOT_UNCONVENTIONAL_DEVICE)) {
@@ -1731,9 +1737,11 @@ LegacyBiosBuildE820 (
//
// First entry is 0 to (640k - EBDA)
//
+ DisableNullDetection ();
E820Table[0].BaseAddr = 0;
E820Table[0].Length = (UINT64) ((*(UINT16 *) (UINTN)0x40E) << 4);
E820Table[0].Type = EfiAcpiAddressRangeMemory;
+ EnableNullDetection ();
//
// Second entry is (640k - EBDA) to 640k
@@ -1967,6 +1975,8 @@ LegacyBiosCompleteBdaBeforeBoot (
UINT16 MachineConfig;
DEVICE_PRODUCER_DATA_HEADER *SioPtr;
+ DisableNullDetection ();
+
Bda = (BDA_STRUC *) ((UINTN) 0x400);
MachineConfig = 0;
@@ -2025,6 +2035,8 @@ LegacyBiosCompleteBdaBeforeBoot (
MachineConfig = (UINT16) (MachineConfig + 0x00 + 0x02 + (SioPtr->MousePresent * 0x04));
Bda->MachineConfig = MachineConfig;
+ EnableNullDetection ();
+
return EFI_SUCCESS;
}
@@ -2049,15 +2061,20 @@ LegacyBiosUpdateKeyboardLedStatus (
UINT8 LocalLeds;
EFI_IA32_REGISTER_SET Regs;
- Bda = (BDA_STRUC *) ((UINTN) 0x400);
-
Private = LEGACY_BIOS_INSTANCE_FROM_THIS (This);
+
+ DisableNullDetection ();
+
+ Bda = (BDA_STRUC *) ((UINTN) 0x400);
LocalLeds = Leds;
Bda->LedStatus = (UINT8) ((Bda->LedStatus &~0x07) | LocalLeds);
LocalLeds = (UINT8) (LocalLeds << 4);
Bda->ShiftStatus = (UINT8) ((Bda->ShiftStatus &~0x70) | LocalLeds);
LocalLeds = (UINT8) (Leds & 0x20);
Bda->KeyboardStatus = (UINT8) ((Bda->KeyboardStatus &~0x20) | LocalLeds);
+
+ EnableNullDetection ();
+
//
// Call into Legacy16 code to allow it to do any processing
//
@@ -2102,7 +2119,9 @@ LegacyBiosCompleteStandardCmosBeforeBoot (
// to large capacity drives
// CMOS 14 = BDA 40:10 plus bit 3(display enabled)
//
+ DisableNullDetection ();
Bda = (UINT8)(*((UINT8 *)((UINTN)0x410)) | BIT3);
+ EnableNullDetection ();
//
// Force display enabled
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c
index 8ffdf0c1ff..d38cef3e33 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c
@@ -2279,6 +2279,7 @@ LegacyBiosInstallRom (
UINTN Function;
EFI_IA32_REGISTER_SET Regs;
UINT8 VideoMode;
+ UINT8 OldVideoMode;
EFI_TIME BootTime;
UINT32 *BdaPtr;
UINT32 LocalTime;
@@ -2299,6 +2300,7 @@ LegacyBiosInstallRom (
Device = 0;
Function = 0;
VideoMode = 0;
+ OldVideoMode = 0;
PhysicalAddress = 0;
MaxRomAddr = PcdGet32 (PcdEndOpromShadowAddress);
@@ -2401,6 +2403,8 @@ LegacyBiosInstallRom (
// 2. BBS compliants drives will not change 40:75 until boot time.
// 3. Onboard IDE controllers will change 40:75
//
+ DisableNullDetection ();
+
LocalDiskStart = (UINT8) ((*(UINT8 *) ((UINTN) 0x475)) + 0x80);
if ((Private->Disk4075 + 0x80) < LocalDiskStart) {
//
@@ -2426,6 +2430,9 @@ LegacyBiosInstallRom (
//
VideoMode = *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE));
}
+
+ EnableNullDetection ();
+
//
// Notify the platform that we are about to scan the ROM
//
@@ -2466,9 +2473,11 @@ LegacyBiosInstallRom (
// Multiply result by 18.2 for number of ticks since midnight.
// Use 182/10 to avoid floating point math.
//
+ DisableNullDetection ();
LocalTime = (LocalTime * 182) / 10;
BdaPtr = (UINT32 *) ((UINTN) 0x46C);
*BdaPtr = LocalTime;
+ EnableNullDetection ();
//
// Pass in handoff data
@@ -2564,7 +2573,11 @@ LegacyBiosInstallRom (
//
// Set mode settings since PrepareToScanRom may change mode
//
- if (VideoMode != *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE))) {
+ DisableNullDetection ();
+ OldVideoMode = *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE));
+ EnableNullDetection ();
+
+ if (VideoMode != OldVideoMode) {
//
// The active video mode is changed, restore it to original mode.
//
@@ -2604,7 +2617,9 @@ LegacyBiosInstallRom (
}
}
+ DisableNullDetection ();
LocalDiskEnd = (UINT8) ((*(UINT8 *) ((UINTN) 0x475)) + 0x80);
+ EnableNullDetection ();
//
// Allow platform to perform any required actions after the
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
index 3d9a8b9649..f42c13cd89 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
@@ -57,7 +57,11 @@ LegacyBiosInt86 (
IN EFI_IA32_REGISTER_SET *Regs
)
{
- UINT32 *VectorBase;
+ UINT16 Segment;
+ UINT16 Offset;
+ LEGACY_BIOS_INSTANCE *Private;
+
+ Private = LEGACY_BIOS_INSTANCE_FROM_THIS (This);
Regs->X.Flags.Reserved1 = 1;
Regs->X.Flags.Reserved2 = 0;
@@ -72,12 +76,15 @@ LegacyBiosInt86 (
// The base address of legacy interrupt vector table is 0.
// We use this base address to get the legacy interrupt handler.
//
- VectorBase = 0;
+ DisableNullDetection ();
+ Segment = (UINT16)(((UINT32 *)0)[BiosInt] >> 16);
+ Offset = (UINT16)((UINT32 *)0)[BiosInt];
+ EnableNullDetection ();
return InternalLegacyBiosFarCall (
This,
- (UINT16) ((VectorBase)[BiosInt] >> 16),
- (UINT16) (VectorBase)[BiosInt],
+ Segment,
+ Offset,
Regs,
&Regs->X.Flags,
sizeof (Regs->X.Flags)
@@ -293,9 +300,15 @@ InternalLegacyBiosFarCall (
UINTN EbdaBaseAddress;
UINTN ReservedEbdaBaseAddress;
- EbdaBaseAddress = (*(UINT16 *) (UINTN) 0x40E) << 4;
- ReservedEbdaBaseAddress = CONVENTIONAL_MEMORY_TOP - PcdGet32 (PcdEbdaReservedMemorySize);
- ASSERT (ReservedEbdaBaseAddress <= EbdaBaseAddress);
+ //
+ // Skip this part of debug code if NULL pointer detection is enabled
+ //
+ if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
+ EbdaBaseAddress = (*(UINT16 *) (UINTN) 0x40E) << 4;
+ ReservedEbdaBaseAddress = CONVENTIONAL_MEMORY_TOP
+ - PcdGet32 (PcdEbdaReservedMemorySize);
+ ASSERT (ReservedEbdaBaseAddress <= EbdaBaseAddress);
+ }
}
);
--
2.14.1.windows.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 6/6] OvmfPkg/QemuVideoDxe: Bypass NULL pointer detection during VBE SHIM installing
2017-09-28 1:03 [PATCH v3 0/6] Add NULL pointer detection feature Jian J Wang
` (4 preceding siblings ...)
2017-09-28 1:03 ` [PATCH v3 5/6] IntelFrameworkModulePkg/Csm: Add code to bypass NULL pointer detection Jian J Wang
@ 2017-09-28 1:03 ` Jian J Wang
2017-09-28 7:59 ` Laszlo Ersek
2017-10-02 17:58 ` Jordan Justen
5 siblings, 2 replies; 19+ messages in thread
From: Jian J Wang @ 2017-09-28 1:03 UTC (permalink / raw)
To: edk2-devel
Cc: Star Zeng, Eric Dong, Laszlo Ersek, Jiewen Yao, Michael Kinney,
Jordan Justen, Ayellet Wolman
QemuVideoDxe driver will link VBE SHIM into page 0. If NULL pointer
detection is enabled, this driver will fail to load. NULL pointer detection
bypassing code is added to prevent such problem during boot.
Please note that Windows 7 will try to access VBE SHIM during boot if it's
installed, and then cause boot failure. This can be fixed by setting BIT7
of PcdNullPointerDetectionPropertyMask to disable NULL pointer detection
after EndOfDxe. As far as we know, there's no other OSs has such issue.
> According to Laszlo, remove the code disabling/enabling the NULL pointer
> detection but just ignore the installing if it's enabled
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>
---
OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 1 +
OvmfPkg/QemuVideoDxe/VbeShim.c | 14 ++++++++++++++
2 files changed, 15 insertions(+)
diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
index 577e07b0a8..ff68c99e96 100644
--- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
+++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
@@ -77,3 +77,4 @@
[Pcd]
gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
+ gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.c b/OvmfPkg/QemuVideoDxe/VbeShim.c
index e45a08e887..8ba5522cde 100644
--- a/OvmfPkg/QemuVideoDxe/VbeShim.c
+++ b/OvmfPkg/QemuVideoDxe/VbeShim.c
@@ -75,6 +75,20 @@ InstallVbeShim (
UINTN Printed;
VBE_MODE_INFO *VbeModeInfo;
+ if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) {
+ DEBUG ((
+ DEBUG_WARN,
+ "%a: page 0 protected, not installing VBE shim\n",
+ __FUNCTION__
+ ));
+ DEBUG ((
+ DEBUG_WARN,
+ "%a: page 0 protection prevents Windows 7 from booting anyway\n",
+ __FUNCTION__
+ ));
+ return;
+ }
+
Segment0 = 0x00000;
SegmentC = 0xC0000;
SegmentF = 0xF0000;
--
2.14.1.windows.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 6/6] OvmfPkg/QemuVideoDxe: Bypass NULL pointer detection during VBE SHIM installing
2017-09-28 1:03 ` [PATCH v3 6/6] OvmfPkg/QemuVideoDxe: Bypass NULL pointer detection during VBE SHIM installing Jian J Wang
@ 2017-09-28 7:59 ` Laszlo Ersek
2017-10-02 17:58 ` Jordan Justen
1 sibling, 0 replies; 19+ messages in thread
From: Laszlo Ersek @ 2017-09-28 7:59 UTC (permalink / raw)
To: Jian J Wang, edk2-devel
Cc: Star Zeng, Eric Dong, Jiewen Yao, Michael Kinney, Jordan Justen,
Ayellet Wolman
On 09/28/17 03:03, Jian J Wang wrote:
> QemuVideoDxe driver will link VBE SHIM into page 0. If NULL pointer
> detection is enabled, this driver will fail to load. NULL pointer detection
> bypassing code is added to prevent such problem during boot.
>
> Please note that Windows 7 will try to access VBE SHIM during boot if it's
> installed, and then cause boot failure. This can be fixed by setting BIT7
> of PcdNullPointerDetectionPropertyMask to disable NULL pointer detection
> after EndOfDxe. As far as we know, there's no other OSs has such issue.
>
>> According to Laszlo, remove the code disabling/enabling the NULL pointer
>> detection but just ignore the installing if it's enabled
>
> 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>
> ---
> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 1 +
> OvmfPkg/QemuVideoDxe/VbeShim.c | 14 ++++++++++++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> index 577e07b0a8..ff68c99e96 100644
> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> @@ -77,3 +77,4 @@
> [Pcd]
> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.c b/OvmfPkg/QemuVideoDxe/VbeShim.c
> index e45a08e887..8ba5522cde 100644
> --- a/OvmfPkg/QemuVideoDxe/VbeShim.c
> +++ b/OvmfPkg/QemuVideoDxe/VbeShim.c
> @@ -75,6 +75,20 @@ InstallVbeShim (
> UINTN Printed;
> VBE_MODE_INFO *VbeModeInfo;
>
> + if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) {
> + DEBUG ((
> + DEBUG_WARN,
> + "%a: page 0 protected, not installing VBE shim\n",
> + __FUNCTION__
> + ));
> + DEBUG ((
> + DEBUG_WARN,
> + "%a: page 0 protection prevents Windows 7 from booting anyway\n",
> + __FUNCTION__
> + ));
> + return;
> + }
> +
> Segment0 = 0x00000;
> SegmentC = 0xC0000;
> SegmentF = 0xF0000;
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 6/6] OvmfPkg/QemuVideoDxe: Bypass NULL pointer detection during VBE SHIM installing
2017-09-28 1:03 ` [PATCH v3 6/6] OvmfPkg/QemuVideoDxe: Bypass NULL pointer detection during VBE SHIM installing Jian J Wang
2017-09-28 7:59 ` Laszlo Ersek
@ 2017-10-02 17:58 ` Jordan Justen
1 sibling, 0 replies; 19+ messages in thread
From: Jordan Justen @ 2017-10-02 17:58 UTC (permalink / raw)
To: Jian J Wang, edk2-devel
Cc: Star Zeng, Eric Dong, Laszlo Ersek, Jiewen Yao, Michael Kinney,
Ayellet Wolman
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
On 2017-09-27 18:03:53, Jian J Wang wrote:
> QemuVideoDxe driver will link VBE SHIM into page 0. If NULL pointer
> detection is enabled, this driver will fail to load. NULL pointer detection
> bypassing code is added to prevent such problem during boot.
>
> Please note that Windows 7 will try to access VBE SHIM during boot if it's
> installed, and then cause boot failure. This can be fixed by setting BIT7
> of PcdNullPointerDetectionPropertyMask to disable NULL pointer detection
> after EndOfDxe. As far as we know, there's no other OSs has such issue.
>
> > According to Laszlo, remove the code disabling/enabling the NULL pointer
> > detection but just ignore the installing if it's enabled
>
> 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>
> ---
> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 1 +
> OvmfPkg/QemuVideoDxe/VbeShim.c | 14 ++++++++++++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> index 577e07b0a8..ff68c99e96 100644
> --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf
> @@ -77,3 +77,4 @@
> [Pcd]
> gOptionRomPkgTokenSpaceGuid.PcdDriverSupportedEfiVersion
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> diff --git a/OvmfPkg/QemuVideoDxe/VbeShim.c b/OvmfPkg/QemuVideoDxe/VbeShim.c
> index e45a08e887..8ba5522cde 100644
> --- a/OvmfPkg/QemuVideoDxe/VbeShim.c
> +++ b/OvmfPkg/QemuVideoDxe/VbeShim.c
> @@ -75,6 +75,20 @@ InstallVbeShim (
> UINTN Printed;
> VBE_MODE_INFO *VbeModeInfo;
>
> + if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) {
> + DEBUG ((
> + DEBUG_WARN,
> + "%a: page 0 protected, not installing VBE shim\n",
> + __FUNCTION__
> + ));
> + DEBUG ((
> + DEBUG_WARN,
> + "%a: page 0 protection prevents Windows 7 from booting anyway\n",
> + __FUNCTION__
> + ));
> + return;
> + }
> +
> Segment0 = 0x00000;
> SegmentC = 0xC0000;
> SegmentF = 0xF0000;
> --
> 2.14.1.windows.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread