ClearLegacyMemory() assumes that the memory allocation HOB comes after the resource descriptor HOB in the HOB list. Is that guaranteed? I'd think that the memory allocation HOB traversal should be a separate loop, after the resource descriptor HOB traversal loop.
Other than that: Reviewed-by: Brian J. Johnson <brian.john...@hpe.com> -----Original Message----- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang, Jian J Sent: Wednesday, September 13, 2017 4:25 AM To: edk2-devel@lists.01.org Cc: jus...@ml01.01.org; Eric Dong <eric.d...@intel.com>; kin...@ml01.01.org; Jordan L <jordan.l.jus...@intel.com>; wol...@ml01.01.org; Jiewen Yao <jiewen....@intel.com>; Ayellet <ayellet.wol...@intel.com>; Michael D <michael.d.kin...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Star Zeng <star.z...@intel.com> Subject: [edk2] [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core. The mechanism behind is to trigger a page fault exception at address 0. This can be made by disabling page 0 (0-4095) during page table setup. So this feature can only be available on platform with paging enabled. Once this feature is enabled, any code, like CSM, which has to access memory in page 0 needs to enable this page temporarily in advance and disable it afterwards. PcdNullPointerDetectionPropertyMask is used to control and elaborate the use cases. Cc: Jiewen Yao <jiewen....@intel.com> Cc: Eric Dong <eric.d...@intel.com> Cc: Star Zeng <star.z...@intel.com> Cc: Laszlo Ersek <ler...@redhat.com> Cc: Justen, Jordan L <jordan.l.jus...@intel.com> Cc: Kinney, Michael D <michael.d.kin...@intel.com> Cc: Wolman, Ayellet <ayellet.wol...@intel.com> Suggested-by: Wolman, Ayellet <ayellet.wol...@intel.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Wang, Jian J <jian.j.w...@intel.com> --- MdeModulePkg/Core/Dxe/DxeMain.inf | 3 +- MdeModulePkg/Core/Dxe/Mem/Page.c | 21 ++++++---- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 47 +++++++++++++++++++++ MdeModulePkg/Core/DxeIplPeim/DxeIpl.h | 15 +++++++ MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 3 +- MdeModulePkg/Core/DxeIplPeim/DxeLoad.c | 53 ++++++++++++++++++++++++ MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c | 8 +++- MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c | 2 + MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 23 ++++++---- MdeModulePkg/MdeModulePkg.dec | 12 ++++++ 10 files changed, 167 insertions(+), 20 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf index 30d5984f7c..273b8b7c0e 100644 --- a/MdeModulePkg/Core/Dxe/DxeMain.inf +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf @@ -179,7 +179,7 @@ gEfiWatchdogTimerArchProtocolGuid ## CONSUMES [FeaturePcd] - gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport ## CONSUMES + gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport ## CONSUMES [Pcd] gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber ## SOMETIMES_CONSUMES @@ -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..2e0b72f864 100644 --- a/MdeModulePkg/Core/Dxe/Mem/Page.c +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c @@ -170,6 +170,7 @@ CoreAddRange ( { LIST_ENTRY *Link; MEMORY_MAP *Entry; + EFI_STATUS Status; ASSERT ((Start & EFI_PAGE_MASK) == 0); ASSERT (End > Start) ; @@ -188,7 +189,17 @@ 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); + } else if (gCpu != NULL) { + Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, 0); + ASSERT_EFI_ERROR(Status); + + SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0); + + Status = gCpu->SetMemoryAttributes(gCpu, 0, EFI_PAGE_SIZE, EFI_MEMORY_RP); + ASSERT_EFI_ERROR(Status); + } } // @@ -1972,11 +1983,3 @@ Done: return Status; } - - - - - - - - diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c index a73c4ccd64..2367d674e1 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,22 @@ 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 ; } diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h index 72d2532f50..104599156c 100644 --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h @@ -52,6 +52,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. #define STACK_SIZE 0x20000 #define BSP_STORE_SIZE 0x4000 +#define NULL_DETECTION_ENABLED ((PcdGet8(PcdNullPointerDetectionPropertyMask) & BIT0) != 0) // // This PPI is installed to indicate the end of the PEI usage of memory @@ -240,4 +241,18 @@ 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 + ); + #endif diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf index c54afe4aa6..fde70f94bb 100644 --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf @@ -110,11 +110,12 @@ gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplBuildPageTables ## CONSUMES [FeaturePcd] - gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress ## CONSUMES + gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress ## CONSUMES [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..b5f9d92f5b 100644 --- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c +++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c @@ -825,3 +825,56 @@ 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 RscDescHob; + EFI_PEI_HOB_POINTERS MemAllocHob; + BOOLEAN DoClear; + + RscDescHob.Raw = HobStart; + MemAllocHob.Raw = HobStart; + DoClear = FALSE; + + // + // Check if page 0 exists and free + // + while ((RscDescHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, RscDescHob.Raw)) != NULL) { + if (RscDescHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY && + RscDescHob.ResourceDescriptor->PhysicalStart == 0) { + DoClear = TRUE; + // + // Make sure memory at 0-4095 has not been allocated. + // + while ((MemAllocHob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, MemAllocHob.Raw)) != NULL) { + if (MemAllocHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress < EFI_PAGE_SIZE) { + DoClear = FALSE; + break; + } + MemAllocHob.Raw = GET_NEXT_HOB (MemAllocHob); + } + break; + } + RscDescHob.Raw = GET_NEXT_HOB (RscDescHob); + } + + if (DoClear) { + DEBUG((DEBUG_INFO, "Clearing first 4K-page!\r\n")); + SetMem(NULL, EFI_PAGE_SIZE, 0); + } + + return; +} + diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c index 1957326caf..a8aa0d5d1b 100644 --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c @@ -123,7 +123,8 @@ Create4GPageTablesIa32Pae ( PageDirectoryPointerEntry->Bits.Present = 1; for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress += SIZE_2MB) { - if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase)) { + if ((NULL_DETECTION_ENABLED && PhysicalAddress == 0) + || ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + SIZE_2MB) > StackBase))) { // // Need to split this 2M page that covers stack range. // @@ -240,6 +241,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 +382,8 @@ HandOffToDxeCore ( TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT); PageTables = 0; - BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) && IsIa32PaeSupport () && IsExecuteDisableBitAvailable ()); + BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () && IsExecuteDisableBitAvailable () + && (PcdGetBool (PcdSetNxForStack) || NULL_DETECTION_ENABLED)); 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..50a8d77a5b 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..ccd6e10cb2 100644 --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c @@ -90,8 +90,14 @@ Split2MPageTo4K ( // PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask; PageTableEntry->Bits.ReadWrite = 1; - PageTableEntry->Bits.Present = 1; - if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + StackSize)) { + + if (NULL_DETECTION_ENABLED && 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 +143,10 @@ Split1GPageTo2M ( PhysicalAddress2M = PhysicalAddress; for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += SIZE_2MB) { - if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M + SIZE_2MB) > StackBase)) { + if ((NULL_DETECTION_ENABLED && 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 +286,8 @@ 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 ((NULL_DETECTION_ENABLED && PageAddress == 0) + || (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + StackSize) && ((PageAddress + SIZE_1GB) > StackBase))) { Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry, StackBase, StackSize); } else { // @@ -308,9 +316,10 @@ 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 ((NULL_DETECTION_ENABLED && 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 { diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index 593bff357a..1cc84894af 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -860,6 +860,18 @@ # @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 -- 2.14.1.windows.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
