Re: [PATCH 1/4] MdeModulePkg/Core: Implement NULL pointer detection in EDK-II Core.

[Jordan Justen <jordan.l.justen@intel.com>]

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 <jiewen.yao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Wolman, Ayellet <ayellet.wolman@intel.com>
> Suggested-by: Wolman, Ayellet <ayellet.wolman@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wang, Jian J <jian.j.wang@intel.com>
> ---
>  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
>