Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and heap guard

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 1/29/2018 8:09 PM, Jian J Wang wrote:
> Considering following scenario (both NX memory protection and heap guard
> are enabled):
> 
>     1. Allocate 3 pages. The attributes of adjacent memory pages will be
> 
>        |NOT-PRESENT|  present  |  present  |  present  |NOT-PRESENT|
> 
>     2. Free the middle page. The attributes of adjacent memory pages should
>        be
> 
>        |NOT-PRESENT|  present  |NOT-PRESENT|  present  |NOT-PRESENT|
> 
>        But the NX feature will overwrite the attributes of middle page. So
>        it looks still like below, which is wrong.
> 
>        |NOT-PRESENT|  present  |  PRESENT  |  present  |NOT-PRESENT|
> 
> The solution is checking the first and/or last page of a memory block to be
> marked as NX, and skipping them if they are Guard pages.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@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         | 14 ++++++++++++++
>   MdeModulePkg/Core/Dxe/Mem/HeapGuard.h         | 10 ++++++++++
>   MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 22 ++++++++++++++++++++++
>   3 files changed, 46 insertions(+)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> index 392aeb8a02..d7906e08c5 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> @@ -728,6 +728,20 @@ IsPageTypeToGuard (
>     return IsMemoryTypeToGuard (MemoryType, AllocateType, GUARD_HEAP_TYPE_PAGE);
>   }
>   
> +/**
> +  Check to see if the heap guard is enabled for page and/or pool allocation.
> +
> +  @return TRUE/FALSE.
> +**/
> +BOOLEAN
> +IsHeapGuardEnabled (
> +  VOID
> +  )
> +{
> +  return IsMemoryTypeToGuard (EfiMaxMemoryType, AllocateAnyPages,
> +                              GUARD_HEAP_TYPE_POOL|GUARD_HEAP_TYPE_PAGE);
> +}
> +
>   /**
>     Set head Guard and tail Guard for the given memory range.
>   
> diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
> index 30ac0e678f..7208ab1437 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
> +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
> @@ -389,6 +389,16 @@ AdjustPoolHeadF (
>     IN EFI_PHYSICAL_ADDRESS    Memory
>     );
>   
> +/**
> +  Check to see if the heap guard is enabled for page and/or pool allocation.
> +
> +  @return TRUE/FALSE.
> +**/
> +BOOLEAN
> +IsHeapGuardEnabled (
> +  VOID
> +  );
> +
>   extern BOOLEAN mOnGuarding;
>   
>   #endif
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 150167bf66..877e6e5025 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -48,6 +48,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>   #include <Protocol/SimpleFileSystem.h>
>   
>   #include "DxeMain.h"
> +#include "Mem/HeapGuard.h"
>   
>   #define CACHE_ATTRIBUTE_MASK   (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)
>   #define MEMORY_ATTRIBUTE_MASK  (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
> @@ -1200,6 +1201,27 @@ ApplyMemoryProtectionPolicy (
>       return EFI_SUCCESS;
>     }
>   
> +  //
> +  // Don't overwrite Guard pages, which should be the first and/or last page,
> +  // if any.
> +  //
> +  if (IsHeapGuardEnabled ()) {
> +    if (IsGuardPage (Memory))  {
> +      Memory += EFI_PAGE_SIZE;
> +      Length -= EFI_PAGE_SIZE;
> +      if (Length == 0) {
> +        return EFI_SUCCESS;
> +      }
> +    }
> +
> +    if (IsGuardPage (Memory + Length - EFI_PAGE_SIZE))  {
> +      Length -= EFI_PAGE_SIZE;
> +      if (Length == 0) {
> +        return EFI_SUCCESS;
> +      }
> +    }
> +  }
> +
>     //
>     // Update the executable permissions according to the DXE memory
>     // protection policy, but only if
> 
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-- 
Thanks,
Ray