* Re: [PATCH] MdeModulePkg/Core: Fix heap guard issues
@ 2017-12-25 2:28 Wang, Jian J
0 siblings, 0 replies; only message in thread
From: Wang, Jian J @ 2017-12-25 2:28 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric, Zeng, Star
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
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2017-12-25 2:23 UTC | newest]
Thread overview: (only message) (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-25 2:28 [PATCH] MdeModulePkg/Core: Fix heap guard issues Wang, Jian J
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox