On 2017-09-13 02:25:04, Wang, Jian J wrote: > 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. >
Your commit message line is > 450 columns. https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format I think BaseTools/Scripts/PatchCheck.py can help detect this issue. (more below...) > Cc: Jiewen Yao <[email protected]> > Cc: Eric Dong <[email protected]> > Cc: Star Zeng <[email protected]> > Cc: Laszlo Ersek <[email protected]> > Cc: Justen, Jordan L <[email protected]> > Cc: Kinney, Michael D <[email protected]> > Cc: Wolman, Ayellet <[email protected]> > Suggested-by: Wolman, Ayellet <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Wang, Jian J <[email protected]> > --- > 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 Why is this line changed? > > [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) I think maybe the code style would look better if you defined a function for this rather than using a macro. What do you think? > > // > // 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) { This code line is longer than 80 columns. There are more cases of this in your patchset. -Jordan > + 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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
