Re: [PATCH v3 2/6] MdeModulePkg/DxeIpl: Implement NULL pointer detection

["Zeng, Star" <star.zeng@intel.com>]

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