* [PATCH 1/2] MdeModulePkg/Core: Fix heap guard issues
@ 2017-12-25 2:23 Jian J Wang
2017-12-26 1:23 ` Yao, Jiewen
0 siblings, 1 reply; 2+ messages in thread
From: Jian J Wang @ 2017-12-25 2:23 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Eric Dong, Jiewen Yao
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH 1/2] MdeModulePkg/Core: Fix heap guard issues
2017-12-25 2:23 [PATCH 1/2] MdeModulePkg/Core: Fix heap guard issues Jian J Wang
@ 2017-12-26 1:23 ` Yao, Jiewen
0 siblings, 0 replies; 2+ messages in thread
From: Yao, Jiewen @ 2017-12-26 1:23 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org; +Cc: Dong, Eric, Zeng, Star
It is pity to add this alignment requirement, because the some special case tail overflow might not be caught.
If any other driver has such assumption, we may add ASSERT() to make sure the memory allocated is 8-byte aligned. As such we can catch this problem earlier, such as the Variable driver. It can be done in a separate patch.
Reviewed-by: Jiewen.yao@intel.com
Thank you
Yao Jiewen
> -----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] 2+ messages in thread
end of thread, other threads:[~2017-12-26 1:18 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-25 2:23 [PATCH 1/2] MdeModulePkg/Core: Fix heap guard issues Jian J Wang
2017-12-26 1:23 ` Yao, Jiewen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox