>From this perspective, you're right. > -----Original Message----- > From: Zeng, Star > Sent: Thursday, September 28, 2017 1:10 PM > 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 > > 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
