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 <[email protected]>; [email protected] Cc: Dong, Eric <[email protected]>; Laszlo Ersek <[email protected]>; Yao, Jiewen <[email protected]>; Kinney, Michael D <[email protected]>; Justen, Jordan L <[email protected]>; Wolman, Ayellet <[email protected]> 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 <[email protected]>; [email protected] > Cc: Dong, Eric <[email protected]>; Laszlo Ersek > <[email protected]>; Yao, Jiewen <[email protected]>; Kinney, > Michael D <[email protected]>; Justen, Jordan L > <[email protected]>; Wolman, Ayellet > <[email protected]>; Zeng, Star <[email protected]> > 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 <[email protected]>; [email protected] > Cc: Dong, Eric <[email protected]>; Laszlo Ersek > <[email protected]>; Yao, Jiewen <[email protected]>; Kinney, > Michael D <[email protected]>; Justen, Jordan L > <[email protected]>; Wolman, Ayellet > <[email protected]>; Zeng, Star <[email protected]> > 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: [email protected] > Cc: Zeng, Star <[email protected]>; Dong, Eric > <[email protected]>; Laszlo Ersek <[email protected]>; Yao, Jiewen > <[email protected]>; Kinney, Michael D > <[email protected]>; Justen, Jordan L > <[email protected]>; Wolman, Ayellet > <[email protected]> > 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 <[email protected]> > Cc: Eric Dong <[email protected]> > Cc: Laszlo Ersek <[email protected]> > Cc: Jiewen Yao <[email protected]> > Cc: Michael Kinney <[email protected]> > Cc: Jordan Justen <[email protected]> > Cc: Ayellet Wolman <[email protected]> > Suggested-by: Ayellet Wolman <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <[email protected]> > --- > MdeModulePkg/Core/DxeIplPeim/DxeIpl.h | 25 +++++++++ > MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 1 + > MdeModulePkg/Core/DxeIplPeim/DxeLoad.c | 65 > ++++++++++++++++++++++++ > MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c | 11 +++- > MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c | 2 + > MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 31 ++++++++--- > 6 files changed, 126 insertions(+), 9 deletions(-) > > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h > b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h > index 72d2532f50..1654bcd2dc 100644 > --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h > @@ -240,4 +240,29 @@ Decompress ( > OUT UINTN *OutputSize > ); > > +/** > + Clear legacy memory located at the first 4K-page. > + > + This function traverses the whole HOB list to check if memory from 0 to > 4095 > + exists and has not been allocated, and then clear it if so. > + > + @param HoStart The start of HobList passed to DxeCore. > + > +**/ > +VOID > +ClearLegacyMemory ( > + IN VOID *HobStart > + ); > + > +/** > + Return configure status of NULL pointer detection feature > + > + @return TRUE NULL pointer detection feature is enabled > + @return FALSE NULL pointer detection feature is disabled **/ > +BOOLEAN IsNullDetectionEnabled ( > + VOID > + ); > + > #endif > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > index c54afe4aa6..9d0e76a293 100644 > --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > @@ -115,6 +115,7 @@ > [Pcd.IA32,Pcd.X64] > gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable ## > SOMETIMES_CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask > ## CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask > ## CONSUMES > > [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64] > gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ## > SOMETIMES_CONSUMES > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c > b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c > index 50b5440d15..0a71b1f3de 100644 > --- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c > @@ -825,3 +825,68 @@ UpdateStackHob ( > Hob.Raw = GET_NEXT_HOB (Hob); > } > } > + > +/** > + Clear legacy memory located at the first 4K-page, if available. > + > + This function traverses the whole HOB list to check if memory from 0 to > 4095 > + exists and has not been allocated, and then clear it if so. > + > + @param HoStart The start of HobList passed to DxeCore. > + > +**/ > +VOID > +ClearLegacyMemory ( > + IN VOID *HobStart > + ) > +{ > + EFI_PEI_HOB_POINTERS RscHob; > + EFI_PEI_HOB_POINTERS MemHob; > + BOOLEAN DoClear; > + > + RscHob.Raw = HobStart; > + MemHob.Raw = HobStart; > + DoClear = FALSE; > + > + // > + // Check if page 0 exists and free > + // > + while ((RscHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, > + RscHob.Raw)) != NULL) { > + if (RscHob.ResourceDescriptor->ResourceType == > EFI_RESOURCE_SYSTEM_MEMORY && > + RscHob.ResourceDescriptor->PhysicalStart == 0) { > + DoClear = TRUE; > + // > + // Make sure memory at 0-4095 has not been allocated. > + // > + while ((MemHob.Raw = GetNextHob > (EFI_HOB_TYPE_MEMORY_ALLOCATION, > + MemHob.Raw)) != NULL) { > + if (MemHob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress > + < EFI_PAGE_SIZE) { > + DoClear = FALSE; > + break; > + } > + MemHob.Raw = GET_NEXT_HOB (MemHob); > + } > + break; > + } > + RscHob.Raw = GET_NEXT_HOB (RscHob); } > + > + if (DoClear) { > + DEBUG ((DEBUG_INFO, "Clearing first 4K-page!\r\n")); > + SetMem (NULL, EFI_PAGE_SIZE, 0); > + } > + > + return; > +} > + > +BOOLEAN > +IsNullDetectionEnabled ( > + VOID > + ) > +{ > + return (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0) ? > + TRUE : FALSE); > +} > + > diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c > b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c > index 1957326caf..7a8421dd16 100644 > --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c > +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c > @@ -123,7 +123,9 @@ Create4GPageTablesIa32Pae ( > PageDirectoryPointerEntry->Bits.Present = 1; > > for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries > < 512; > IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress > IndexOfPageDirectoryEntries+++= > SIZE_2MB) { > - if ((PhysicalAddress < StackBase + StackSize) && ((PhysicalAddress + > SIZE_2MB) > StackBase)) { > + if ((IsNullDetectionEnabled () && PhysicalAddress == 0) > + || ((PhysicalAddress < StackBase + StackSize) > + && ((PhysicalAddress + SIZE_2MB) > StackBase))) { > // > // Need to split this 2M page that covers stack range. > // > @@ -240,6 +242,8 @@ HandOffToDxeCore ( > EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi; > BOOLEAN BuildPageTablesIa32Pae; > > + ClearLegacyMemory (HobList.Raw); > + > Status = PeiServicesAllocatePages (EfiBootServicesData, > EFI_SIZE_TO_PAGES (STACK_SIZE), &BaseOfStack); > ASSERT_EFI_ERROR (Status); > > @@ -379,7 +383,10 @@ HandOffToDxeCore ( > TopOfStack = (EFI_PHYSICAL_ADDRESS) (UINTN) ALIGN_POINTER > (TopOfStack, CPU_STACK_ALIGNMENT); > > PageTables = 0; > - BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) && > IsIa32PaeSupport () && IsExecuteDisableBitAvailable ()); > + BuildPageTablesIa32Pae = (BOOLEAN) (IsIa32PaeSupport () && > + (IsNullDetectionEnabled () || > + (PcdGetBool (PcdSetNxForStack) && > + > + IsExecuteDisableBitAvailable ()))); > if (BuildPageTablesIa32Pae) { > PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE); > EnableExecuteDisableBit (); > diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c > b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c > index 6488880eab..d93a9c5a2d 100644 > --- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c > +++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c > @@ -42,6 +42,8 @@ HandOffToDxeCore ( > EFI_VECTOR_HANDOFF_INFO *VectorInfo; > EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi; > > + ClearLegacyMemory (HobList.Raw); > + > // > // Get Vector Hand-off Info PPI and build Guided HOB > // > diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c > b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c > index 48150be4e1..80c1821eca 100644 > --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c > +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c > @@ -90,8 +90,16 @@ Split2MPageTo4K ( > // > PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask; > PageTableEntry->Bits.ReadWrite = 1; > - PageTableEntry->Bits.Present = 1; > - if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + > StackSize)) { > + > + if (IsNullDetectionEnabled () && PhysicalAddress4K == 0) { > + PageTableEntry->Bits.Present = 0; > + } else { > + PageTableEntry->Bits.Present = 1; > + } > + > + if (PcdGetBool (PcdSetNxForStack) > + && (PhysicalAddress4K >= StackBase) > + && (PhysicalAddress4K < StackBase + StackSize)) { > // > // Set Nx bit for stack. > // > @@ -137,9 +145,12 @@ Split1GPageTo2M ( > > PhysicalAddress2M = PhysicalAddress; > for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < > 512; > IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M > IndexOfPageDirectoryEntries+++= > SIZE_2MB) { > - if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M + > SIZE_2MB) > StackBase)) { > + if ((IsNullDetectionEnabled () && PhysicalAddress2M == 0) > + || (PcdGetBool (PcdSetNxForStack) > + && (PhysicalAddress2M < StackBase + StackSize) > + && ((PhysicalAddress2M + SIZE_2MB) > StackBase))) { > // > - // Need to split this 2M page that covers stack range. > + // Need to split this 2M page that covers NULL or stack range. > // > Split2MPageTo4K (PhysicalAddress2M, (UINT64 *) > PageDirectoryEntry, StackBase, StackSize); > } else { > @@ -279,7 +290,10 @@ CreateIdentityMappingPageTables ( > PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry; > > for (IndexOfPageDirectoryEntries = 0; > IndexOfPageDirectoryEntries < 512; > IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress += > SIZE_1GB) { > - if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + > StackSize) && ((PageAddress + SIZE_1GB) > StackBase)) { > + if ((IsNullDetectionEnabled () && PageAddress == 0) > + || (PcdGetBool (PcdSetNxForStack) > + && (PageAddress < StackBase + StackSize) > + && ((PageAddress + SIZE_1GB) > StackBase))) { > Split1GPageTo2M (PageAddress, (UINT64 *) > PageDirectory1GEntry, StackBase, StackSize); > } else { > // > @@ -308,9 +322,12 @@ CreateIdentityMappingPageTables ( > PageDirectoryPointerEntry->Bits.Present = 1; > > for (IndexOfPageDirectoryEntries = 0; > IndexOfPageDirectoryEntries < 512; > IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress += > SIZE_2MB) { > - if (PcdGetBool (PcdSetNxForStack) && (PageAddress < StackBase + > StackSize) && ((PageAddress + SIZE_2MB) > StackBase)) { > + if ((IsNullDetectionEnabled () && PageAddress == 0) > + || (PcdGetBool (PcdSetNxForStack) > + && (PageAddress < StackBase + StackSize) > + && ((PageAddress + SIZE_2MB) > StackBase))) { > // > - // Need to split this 2M page that covers stack range. > + // Need to split this 2M page that covers NULL or stack range. > // > Split2MPageTo4K (PageAddress, (UINT64 *) > PageDirectoryEntry, StackBase, StackSize); > } else { > -- > 2.14.1.windows.1 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
