MdeModulePkg/Core: Fix heap guard issues

Three 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.

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.

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>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index bee229f4c8a73b3101b6c2c4e600706c73822766..0f035043e15a2ead446f9b8d0de89f08a7df2dff 100644 (file)
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -583,8 +583,7 @@ SetGuardPage (
   mOnGuarding = TRUE;\r
   //\r
   // Note: This might overwrite other attributes needed by other features,\r
-  // such as memory protection (NX). Please make sure they are not enabled\r
-  // at the same time.\r
+  // such as NX memory protection.\r
   //\r
   gCpu->SetMemoryAttributes (gCpu, BaseAddress, EFI_PAGE_SIZE, EFI_MEMORY_RP);\r
   mOnGuarding = FALSE;\r
@@ -605,6 +604,18 @@ UnsetGuardPage (
   IN  EFI_PHYSICAL_ADDRESS      BaseAddress\r
   )\r
 {\r
+  UINT64          Attributes;\r
+\r
+  //\r
+  // Once the Guard page is unset, it will be freed back to memory pool. NX\r
+  // memory protection must be restored for this page if NX is enabled for free\r
+  // memory.\r
+  //\r
+  Attributes = 0;\r
+  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & (1 << EfiConventionalMemory)) != 0) {\r
+    Attributes |= EFI_MEMORY_XP;\r
+  }\r
+\r
   //\r
   // Set flag to make sure allocating memory without GUARD for page table\r
   // operation; otherwise infinite loops could be caused.\r
@@ -615,7 +626,7 @@ UnsetGuardPage (
   // such as memory protection (NX). Please make sure they are not enabled\r
   // at the same time.\r
   //\r
-  gCpu->SetMemoryAttributes (gCpu, BaseAddress, EFI_PAGE_SIZE, 0);\r
+  gCpu->SetMemoryAttributes (gCpu, BaseAddress, EFI_PAGE_SIZE, Attributes);\r
   mOnGuarding = FALSE;\r
 }\r
 \r
@@ -869,6 +880,15 @@ AdjustMemoryS (
 {\r
   UINT64  Target;\r
 \r
+  //\r
+  // UEFI spec requires that allocated pool must be 8-byte aligned. If it's\r
+  // indicated to put the pool near the Tail Guard, we need extra bytes to\r
+  // make sure alignment of the returned pool address.\r
+  //\r
+  if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) == 0) {\r
+    SizeRequested = ALIGN_VALUE(SizeRequested, 8);\r
+  }\r
+\r
   Target = Start + Size - SizeRequested;\r
 \r
   //\r
@@ -1052,7 +1072,7 @@ AdjustPoolHeadA (
   IN UINTN                   Size\r
   )\r
 {\r
-  if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {\r
+  if (Memory == 0 || (PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {\r
     //\r
     // Pool head is put near the head Guard\r
     //\r
@@ -1062,6 +1082,7 @@ AdjustPoolHeadA (
   //\r
   // Pool head is put near the tail Guard\r
   //\r
+  Size = ALIGN_VALUE (Size, 8);\r
   return (VOID *)(UINTN)(Memory + EFI_PAGES_TO_SIZE (NoPages) - Size);\r
 }\r
 \r
@@ -1077,7 +1098,7 @@ AdjustPoolHeadF (
   IN EFI_PHYSICAL_ADDRESS    Memory\r
   )\r
 {\r
-  if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {\r
+  if (Memory == 0 || (PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {\r
     //\r
     // Pool head is put near the head Guard\r
     //\r

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index a74cfc137a228a0d4705195088fbc1d6df8bd4db..862593f562ca7defed3e1e41ad732443037b4458 100644 (file)
--- 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\r
   // - EfiConventionalMemory and EfiBootServicesData should use the\r
   //   same attribute\r
-  // - heap guard should not be enabled for the same type of memory\r
   //\r
   ASSERT ((GetPermissionAttributeForMemoryType (EfiBootServicesCode) & EFI_MEMORY_XP) == 0);\r
   ASSERT ((GetPermissionAttributeForMemoryType (EfiRuntimeServicesCode) & EFI_MEMORY_XP) == 0);\r
   ASSERT ((GetPermissionAttributeForMemoryType (EfiLoaderCode) & EFI_MEMORY_XP) == 0);\r
   ASSERT (GetPermissionAttributeForMemoryType (EfiBootServicesData) ==\r
           GetPermissionAttributeForMemoryType (EfiConventionalMemory));\r
-  ASSERT ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & PcdGet64 (PcdHeapGuardPoolType)) == 0);\r
-  ASSERT ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & PcdGet64 (PcdHeapGuardPageType)) == 0);\r
 \r
   if (mImageProtectionPolicy != 0 || PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0) {\r
     Status = CoreCreateEvent (\r

diff --git a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
index 04fa1747a1e15ea9397ee85762744d951b163f8a..063a330b18594cbc991e5000db0285ded7756226 100644 (file)
--- a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
+++ b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c
@@ -877,6 +877,15 @@ AdjustMemoryS (
 {\r
   UINT64  Target;\r
 \r
+  //\r
+  // UEFI spec requires that allocated pool must be 8-byte aligned. If it's\r
+  // indicated to put the pool near the Tail Guard, we need extra bytes to\r
+  // make sure alignment of the returned pool address.\r
+  //\r
+  if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) == 0) {\r
+    SizeRequested = ALIGN_VALUE(SizeRequested, 8);\r
+  }\r
+\r
   Target = Start + Size - SizeRequested;\r
 \r
   //\r
@@ -1060,7 +1069,7 @@ AdjustPoolHeadA (
   IN UINTN                   Size\r
   )\r
 {\r
-  if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {\r
+  if (Memory == 0 || (PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {\r
     //\r
     // Pool head is put near the head Guard\r
     //\r
@@ -1070,6 +1079,7 @@ AdjustPoolHeadA (
   //\r
   // Pool head is put near the tail Guard\r
   //\r
+  Size = ALIGN_VALUE (Size, 8);\r
   return (VOID *)(UINTN)(Memory + EFI_PAGES_TO_SIZE (NoPages) - Size);\r
 }\r
 \r
@@ -1085,7 +1095,7 @@ AdjustPoolHeadF (
   IN EFI_PHYSICAL_ADDRESS    Memory\r
   )\r
 {\r
-  if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {\r
+  if (Memory == 0 || (PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) {\r
     //\r
     // Pool head is put near the head Guard\r
     //\r