Re: [PATCH] MdeModulePkg/Core: Fix heap guard issues

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

Sorry for the email title. This is not a series patch.

And I missed one more issue addressed by this patch:
c. NULL address handling due to allocation failure
When allocation failure, normally a NULL will be returned. But Heap Guard code
will still try to adjust the starting address of it, which will cause a non-NULL pointer
returned.

Regards,
Jian


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jian J
> Wang
> Sent: Monday, December 25, 2017 10:23 AM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH 1/2] MdeModulePkg/Core: Fix heap guard issues
> 
> Two issues addressed here:
> 
> a. Make NX memory protection and heap guard to be compatible
> The solution is to check PcdDxeNxMemoryProtectionPolicy in Heap Guard to see
> if the free memory should be set to NX, and set the Guard page to NX before
> it's freed back to memory pool. This can solve the issue which NX setting
> would be overwritten by Heap Guard feature in certain configuration.
> 
> b. Returned pool address was not 8-byte aligned sometimes
> This happened only when BIT7 is not set in PcdHeapGuardPropertyMask. Since
> 8-byte alignment is UEFI spec required, letting allocated pool adjacent to
> tail guard page cannot be guaranteed.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Mem/HeapGuard.c         | 31
> ++++++++++++++++++++++-----
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c |  3 ---
>  MdeModulePkg/Core/PiSmmCore/HeapGuard.c       | 14 ++++++++++--
>  3 files changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> index bee229f4c8..0f035043e1 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> @@ -583,8 +583,7 @@ SetGuardPage (
>    mOnGuarding = TRUE;
>    //
>    // Note: This might overwrite other attributes needed by other features,
> -  // such as memory protection (NX). Please make sure they are not enabled
> -  // at the same time.
> +  // such as NX memory protection.
>    //
>    gCpu->SetMemoryAttributes (gCpu, BaseAddress, EFI_PAGE_SIZE,
> EFI_MEMORY_RP);
>    mOnGuarding = FALSE;
> @@ -605,6 +604,18 @@ UnsetGuardPage (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress
>    )
>  {
> +  UINT64          Attributes;
> +
> +  //
> +  // Once the Guard page is unset, it will be freed back to memory pool. NX
> +  // memory protection must be restored for this page if NX is enabled for free
> +  // memory.
> +  //
> +  Attributes = 0;
> +  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & (1 <<
> EfiConventionalMemory)) != 0) {
> +    Attributes |= EFI_MEMORY_XP;
> +  }
> +
>    //
>    // Set flag to make sure allocating memory without GUARD for page table
>    // operation; otherwise infinite loops could be caused.
> @@ -615,7 +626,7 @@ UnsetGuardPage (
>    // such as memory protection (NX). Please make sure they are not enabled
>    // at the same time.
>    //
> -  gCpu->SetMemoryAttributes (gCpu, BaseAddress, EFI_PAGE_SIZE, 0);
> +  gCpu->SetMemoryAttributes (gCpu, BaseAddress, EFI_PAGE_SIZE, Attributes);
>    mOnGuarding = FALSE;
>  }
> 
> @@ -869,6 +880,15 @@ AdjustMemoryS (
>  {
>    UINT64  Target;
> 
> +  //
> +  // UEFI spec requires that allocated pool must be 8-byte aligned. If it's
> +  // indicated to put the pool near the Tail Guard, we need extra bytes to
> +  // make sure alignment of the returned pool address.
> +  //
> +  if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) == 0) {
> +    SizeRequested = ALIGN_VALUE(SizeRequested, 8);
> +  }
> +
>    Target = Start + Size - SizeRequested;
> 
>    //
> @@ -1052,7 +1072,7 @@ AdjustPoolHeadA (
>    IN UINTN                   Size
>    )
>  {
> -  if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {
> +  if (Memory == 0 || (PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {
>      //
>      // Pool head is put near the head Guard
>      //
> @@ -1062,6 +1082,7 @@ AdjustPoolHeadA (
>    //
>    // Pool head is put near the tail Guard
>    //
> +  Size = ALIGN_VALUE (Size, 8);
>    return (VOID *)(UINTN)(Memory + EFI_PAGES_TO_SIZE (NoPages) - Size);
>  }
> 
> @@ -1077,7 +1098,7 @@ AdjustPoolHeadF (
>    IN EFI_PHYSICAL_ADDRESS    Memory
>    )
>  {
> -  if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {
> +  if (Memory == 0 || (PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {
>      //
>      // Pool head is put near the head Guard
>      //
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index a74cfc137a..862593f562 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -1070,15 +1070,12 @@ CoreInitializeMemoryProtection (
>    // - code regions should have no EFI_MEMORY_XP attribute
>    // - EfiConventionalMemory and EfiBootServicesData should use the
>    //   same attribute
> -  // - heap guard should not be enabled for the same type of memory
>    //
>    ASSERT ((GetPermissionAttributeForMemoryType (EfiBootServicesCode) &
> EFI_MEMORY_XP) == 0);
>    ASSERT ((GetPermissionAttributeForMemoryType (EfiRuntimeServicesCode) &
> EFI_MEMORY_XP) == 0);
>    ASSERT ((GetPermissionAttributeForMemoryType (EfiLoaderCode) &
> EFI_MEMORY_XP) == 0);
>    ASSERT (GetPermissionAttributeForMemoryType (EfiBootServicesData) ==
>            GetPermissionAttributeForMemoryType (EfiConventionalMemory));
> -  ASSERT ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & PcdGet64
> (PcdHeapGuardPoolType)) == 0);
> -  ASSERT ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & PcdGet64
> (PcdHeapGuardPageType)) == 0);
> 
>    if (mImageProtectionPolicy != 0 || PcdGet64
> (PcdDxeNxMemoryProtectionPolicy) != 0) {
>      Status = CoreCreateEvent (
> diff --git a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
> b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
> index 04fa1747a1..063a330b18 100644
> --- a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
> +++ b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
> @@ -877,6 +877,15 @@ AdjustMemoryS (
>  {
>    UINT64  Target;
> 
> +  //
> +  // UEFI spec requires that allocated pool must be 8-byte aligned. If it's
> +  // indicated to put the pool near the Tail Guard, we need extra bytes to
> +  // make sure alignment of the returned pool address.
> +  //
> +  if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) == 0) {
> +    SizeRequested = ALIGN_VALUE(SizeRequested, 8);
> +  }
> +
>    Target = Start + Size - SizeRequested;
> 
>    //
> @@ -1060,7 +1069,7 @@ AdjustPoolHeadA (
>    IN UINTN                   Size
>    )
>  {
> -  if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {
> +  if (Memory == 0 || (PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {
>      //
>      // Pool head is put near the head Guard
>      //
> @@ -1070,6 +1079,7 @@ AdjustPoolHeadA (
>    //
>    // Pool head is put near the tail Guard
>    //
> +  Size = ALIGN_VALUE (Size, 8);
>    return (VOID *)(UINTN)(Memory + EFI_PAGES_TO_SIZE (NoPages) - Size);
>  }
> 
> @@ -1085,7 +1095,7 @@ AdjustPoolHeadF (
>    IN EFI_PHYSICAL_ADDRESS    Memory
>    )
>  {
> -  if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {
> +  if (Memory == 0 || (PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {
>      //
>      // Pool head is put near the head Guard
>      //
> --
> 2.15.1.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel