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

["Wang, Jian J" <jian.j.wang@intel.com>]

>From this perspective, you're right.

> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 1:10 PM
> 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
> 
> 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