MdeModulePkg/Core: fix a logic hole in page free

This hole will cause page fault randomly. The root cause is that Guard
page, which is just freed back to page pool but not yet cleared not-
present attribute, will be allocated right away by internal function
CoreFreeMemoryMapStack(). The solution to this issue is to clear the
not-present attribute for freed Guard page before doing any free
operation, instead of after those operation.

The reason we didn't do this before is due to the fact that manipulating
page attributes might cause memory allocation action which would cause a
dead lock inside a memory allocation/free operation. So we always set or
unset Guard page outside the memory lock. After a thorough analysis, we
believe clearing a Guard page will not cause memory allocation because
memory we're to manipulate was already manipulated before for sure.
Therefore there should be no memory allocation occurring in this
situation.

Since we cleared Guard page not-present attribute before freeing instead
of after freeing, the debug code to clear freed memory can now be restored
to its original way (aka no checking and bypassing Guard page).

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
Reviewed-by: Star Zeng <star.zeng@intel.com>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index 0f035043e15a2ead446f9b8d0de89f08a7df2dff..92753c7269acc0315277436912e65b95db748801 100644 (file)
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -1127,11 +1127,26 @@ CoreConvertPagesWithGuard (
   IN EFI_MEMORY_TYPE  NewType\r
   )\r
 {\r
+  UINT64  OldStart;\r
+  UINTN   OldPages;\r
+\r
   if (NewType == EfiConventionalMemory) {\r
+    OldStart = Start;\r
+    OldPages = NumberOfPages;\r
+\r
     AdjustMemoryF (&Start, &NumberOfPages);\r
     if (NumberOfPages == 0) {\r
       return EFI_SUCCESS;\r
     }\r
+\r
+    //\r
+    // It's safe to unset Guard page inside memory lock because there should\r
+    // be no memory allocation occurred in updating memory page attribute at\r
+    // this point. And unsetting Guard page before free will prevent Guard\r
+    // page just freed back to pool from being allocated right away before\r
+    // marking it usable (from non-present to present).\r
+    //\r
+    UnsetGuardForMemory (OldStart, OldPages);\r
   } else {\r
     AdjustMemoryA (&Start, &NumberOfPages);\r
   }\r

diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index db32d0f940de983898fc5a7a2d22a2eff8c58ea8..8d5d03a6d90a8d13e5c520dfcd5f18e1a3ddd5e2 100644 (file)
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -900,42 +900,17 @@ CoreConvertPagesEx (
     //\r
     CoreAddRange (MemType, Start, RangeEnd, Attribute);\r
     if (ChangingType && (MemType == EfiConventionalMemory)) {\r
+      //\r
+      // Avoid calling DEBUG_CLEAR_MEMORY() for an address of 0 because this\r
+      // macro will ASSERT() if address is 0.  Instead, CoreAddRange() guarantees\r
+      // that the page starting at address 0 is always filled with zeros.\r
+      //\r
       if (Start == 0) {\r
-        //\r
-        // Avoid calling DEBUG_CLEAR_MEMORY() for an address of 0 because this\r
-        // macro will ASSERT() if address is 0.  Instead, CoreAddRange()\r
-        // guarantees that the page starting at address 0 is always filled\r
-        // with zeros.\r
-        //\r
         if (RangeEnd > EFI_PAGE_SIZE) {\r
           DEBUG_CLEAR_MEMORY ((VOID *)(UINTN) EFI_PAGE_SIZE, (UINTN) (RangeEnd - EFI_PAGE_SIZE + 1));\r
         }\r
       } else {\r
-        //\r
-        // If Heap Guard is enabled, the page at the top and/or bottom of\r
-        // this memory block to free might be inaccessible. Skipping them\r
-        // to avoid page fault exception.\r
-        //\r
-        UINT64  StartToClear;\r
-        UINT64  EndToClear;\r
-\r
-        StartToClear = Start;\r
-        EndToClear   = RangeEnd + 1;\r
-        if (PcdGet8 (PcdHeapGuardPropertyMask) & (BIT1|BIT0)) {\r
-          if (IsGuardPage(StartToClear)) {\r
-            StartToClear += EFI_PAGE_SIZE;\r
-          }\r
-          if (IsGuardPage (EndToClear - 1)) {\r
-            EndToClear -= EFI_PAGE_SIZE;\r
-          }\r
-        }\r
-\r
-        if (EndToClear > StartToClear) {\r
-          DEBUG_CLEAR_MEMORY(\r
-            (VOID *)(UINTN)StartToClear,\r
-            (UINTN)(EndToClear - StartToClear)\r
-            );\r
-        }\r
+        DEBUG_CLEAR_MEMORY ((VOID *)(UINTN) Start, (UINTN) (RangeEnd - Start + 1));\r
       }\r
     }\r
 \r
@@ -1513,9 +1488,6 @@ CoreInternalFreePages (
 \r
 Done:\r
   CoreReleaseMemoryLock ();\r
-  if (IsGuarded) {\r
-    UnsetGuardForMemory(Memory, NumberOfPages);\r
-  }\r
   return Status;\r
 }\r
 \r

diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index 7464d8773a2787b904d142dce9fa031cee566ef3..df9a1d28dff50006bb4128238e09ffdb92d2f9de 100644 (file)
--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -643,10 +643,16 @@ CoreFreePoolPagesWithGuard (
 \r
   AdjustMemoryF (&Memory, &NoPages);\r
   if (NoPages > 0) {\r
+    //\r
+    // It's safe to unset Guard page inside memory lock because there should\r
+    // be no memory allocation occurred in updating memory page attribute at\r
+    // this point. And unsetting Guard page before free will prevent Guard\r
+    // page just freed back to pool from being allocated right away before\r
+    // marking it usable (from non-present to present).\r
+    //\r
+    UnsetGuardForMemory (MemoryGuarded, NoPagesGuarded);\r
     CoreFreePoolPagesI (PoolType, Memory, NoPages);\r
   }\r
-\r
-  UnsetGuardForMemory (MemoryGuarded, NoPagesGuarded);\r
 }\r
 \r
 /**\r