[PATCH v2 0/5] Add freed-memory guard feature

[PATCH v2 0/5] Add freed-memory guard feature

[Jian J Wang]

> v2 changes:
> a. Drop PCD PcdUseAfterFreeDetectionPropertyMask. Use BIT4 of
>    PcdHeapGuardPropertyMask instead. Add more descriptions about
>    the new usage in dec/uni file as well.
> b. Use global of BOOLEAN other than EFI_LOCK to avoid reentrance
>    of calling InitializePageTablePool()
> c. Update implementation of CoreGetMemorySpaceMap() and 
>    CoreGetIoSpaceMap() to avoid lock failure. Drop the code to
>    detect debug print level used to achieve the same effect.
> d. Change prototype and implementation of IsHeapGuardEnabled()
>    to allow it to check freed-memory guard feature.
> e. Move the sanity check of freed-memory guard and heap guard
>    into HeapGuardCpuArchProtocolNotify()
> f. Add GuardFreedPagesChecked() to avoid duplicate feature check
> g. Split patch series into smaller patch files

Freed-memory guard is a new feauture used to detect UAF (Use-After-Free)
memory issue.


Jian J Wang (5):
  MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature
  UefiCpuPkg/CpuDxe: fix an infinite loop issue
  MdeModulePkg/Core: fix a lock issue in GCD memory map dump
  MdeModulePkg/Core: add freed-memory guard feature
  MdeModulePkg/Core: fix-up for changes introduced by freed-memory guard

 MdeModulePkg/Core/Dxe/Gcd/Gcd.c               | 140 +++++----
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c         | 409 +++++++++++++++++++++++++-
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.h         |  63 +++-
 MdeModulePkg/Core/Dxe/Mem/Page.c              |  41 ++-
 MdeModulePkg/Core/Dxe/Mem/Pool.c              |  21 +-
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c |   2 +-
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c  |  16 +-
 MdeModulePkg/MdeModulePkg.dec                 |  10 +
 MdeModulePkg/MdeModulePkg.uni                 |   6 +-
 UefiCpuPkg/CpuDxe/CpuDxe.h                    |   2 +-
 UefiCpuPkg/CpuDxe/CpuPageTable.c              |  19 +-
 11 files changed, 640 insertions(+), 89 deletions(-)

-- 
2.16.2.windows.1

[PATCH v2 1/5] MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature

[Jian J Wang]

> v2 changes:
> a. Drop PCD PcdUseAfterFreeDetectionPropertyMask. Use BIT4 of
>    PcdHeapGuardPropertyMask instead. Update related descriptions in both
>    dec and uni files.

Freed-memory guard is used to detect UAF (Use-After-Free) memory issue
which is illegal access to memory which has been freed. The principle
behind is similar to heap guard feature, that is we'll turn all pool
memory allocation to page allocation and mark them to be not-present
once they are freed.

This also implies that, once a page is allocated and freed, it cannot
be re-allocated. This will bring another issue, which is that there's
risk that memory space will be used out. To address it, this patch
series add logic put part (at most 64 pages a time) of freed pages
back into page pool, so that the memory service can still have memory
to allocate, when all memory space have been allocated once. This is
called memory promotion. The promoted pages are always from the eldest
pages freed.

BIT4 of following PCD is used to enable the freed-memory guard feature.

  gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask

Please note this feature cannot be enabled with heap guard at the same
time.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdeModulePkg/MdeModulePkg.dec | 10 ++++++++++
 MdeModulePkg/MdeModulePkg.uni |  6 +++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 6037504fa7..255b92ea67 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -955,6 +955,8 @@
   # free pages for all of them. The page allocation for the type related to
   # cleared bits keeps the same as ususal.
   #
+  # This PCD is only valid if BIT0 and/or BIT2 are set in PcdHeapGuardPropertyMask.
+  #
   # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
   #  EfiReservedMemoryType             0x0000000000000001<BR>
   #  EfiLoaderCode                     0x0000000000000002<BR>
@@ -984,6 +986,8 @@
   # if there's enough free memory for all of them. The pool allocation for the
   # type related to cleared bits keeps the same as ususal.
   #
+  # This PCD is only valid if BIT1 and/or BIT3 are set in PcdHeapGuardPropertyMask.
+  #
   # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
   #  EfiReservedMemoryType             0x0000000000000001<BR>
   #  EfiLoaderCode                     0x0000000000000002<BR>
@@ -1007,14 +1011,20 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType|0x0|UINT64|0x30001053
 
   ## This mask is to control Heap Guard behavior.
+  #
   # Note that due to the limit of pool memory implementation and the alignment
   # requirement of UEFI spec, BIT7 is a try-best setting which cannot guarantee
   # that the returned pool is exactly adjacent to head guard page or tail guard
   # page.
+  #
+  # Note that UEFI freed-memory guard and pool/page guard cannot be enabled
+  # at the same time.
+  #
   #   BIT0 - Enable UEFI page guard.<BR>
   #   BIT1 - Enable UEFI pool guard.<BR>
   #   BIT2 - Enable SMM page guard.<BR>
   #   BIT3 - Enable SMM pool guard.<BR>
+  #   BIT4 - Enable UEFI freed-memory guard (Use-After-Free memory detection).<BR>
   #   BIT6 - Enable non-stop mode.<BR>
   #   BIT7 - The direction of Guard Page for Pool Guard.
   #          0 - The returned pool is near the tail guard page.<BR>
diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni
index a6bcb627cf..e72b893509 100644
--- a/MdeModulePkg/MdeModulePkg.uni
+++ b/MdeModulePkg/MdeModulePkg.uni
@@ -1171,6 +1171,7 @@
                                                                                         " before and after corresponding type of pages allocated if there's enough\n"
                                                                                         " free pages for all of them. The page allocation for the type related to\n"
                                                                                         " cleared bits keeps the same as ususal.\n\n"
+                                                                                        " This PCD is only valid if BIT0 and/or BIT2 are set in PcdHeapGuardPropertyMask.\n\n"
                                                                                         " Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>\n"
                                                                                         "  EfiReservedMemoryType             0x0000000000000001\n"
                                                                                         "  EfiLoaderCode                     0x0000000000000002\n"
@@ -1198,6 +1199,7 @@
                                                                                         " before and after corresponding type of pages which the allocated pool occupies,\n"
                                                                                         " if there's enough free memory for all of them. The pool allocation for the\n"
                                                                                         " type related to cleared bits keeps the same as ususal.\n\n"
+                                                                                        " This PCD is only valid if BIT1 and/or BIT3 are set in PcdHeapGuardPropertyMask.\n\n"
                                                                                         " Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>\n"
                                                                                         "  EfiReservedMemoryType             0x0000000000000001\n"
                                                                                         "  EfiLoaderCode                     0x0000000000000002\n"
@@ -1225,11 +1227,13 @@
                                                                                             "Note that due to the limit of pool memory implementation and the alignment\n"
                                                                                             "requirement of UEFI spec, BIT7 is a try-best setting which cannot guarantee\n"
                                                                                             "that the returned pool is exactly adjacent to head guard page or tail guard\n"
-                                                                                            "page.\n"
+                                                                                            "page.\n\n"
+                                                                                            "Note that UEFI freed-memory guard and pool/page guard cannot be enabled at the same time.\n\n"
                                                                                             "   BIT0 - Enable UEFI page guard.<BR>\n"
                                                                                             "   BIT1 - Enable UEFI pool guard.<BR>\n"
                                                                                             "   BIT2 - Enable SMM page guard.<BR>\n"
                                                                                             "   BIT3 - Enable SMM pool guard.<BR>\n"
+                                                                                            "   BIT4 - Enable UEFI freed-memory guard (Use-After-Free memory detection).<BR>\n"
                                                                                             "   BIT7 - The direction of Guard Page for Pool Guard.\n"
                                                                                             "          0 - The returned pool is near the tail guard page.<BR>\n"
                                                                                             "          1 - The returned pool is near the head guard page.<BR>"
-- 
2.16.2.windows.1

[PATCH v2 2/5] UefiCpuPkg/CpuDxe: fix an infinite loop issue

[Jian J Wang]

> v2 changes:
> a. Change the type of mPageTablePoolLock to be BOOLEAN. Related code
>    logic is also updated and refined.
> b. Add non-stop mode for freed-memory guard feature

The freed-memory guard feature will cause an infinite calling
of InitializePageTablePool(). This is due to a fact that
AllocateAlignedPages() is used to allocate page table pool memory.
This function will most likely call gBS->FreePages to free unaligned
pages and then cause another round of page attributes change, like
below

   FreePages() <===============|
=> SetMemoryAttributes()       |
=> <if out of page table>      |
=> InitializePageTablePool()   |
=> AllocateAlignedPages()      |
=> FreePages() ================|

The solution is add a global variable as a lock in page table pool
allocation function and fail any other requests if it has not been
done.

This patch also add non-stop mode for freed-memory guard.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Michael D Kinney <michael.d.kinney@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>
---
 UefiCpuPkg/CpuDxe/CpuDxe.h       |  2 +-
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 19 +++++++++++++++++--
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
index 064ea05bba..3183a3f7f4 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.h
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
@@ -58,7 +58,7 @@
                                        )
 
 #define HEAP_GUARD_NONSTOP_MODE       \
-        ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT1|BIT0)) > BIT6)
+        ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT4|BIT1|BIT0)) > BIT6)
 
 #define NULL_DETECTION_NONSTOP_MODE   \
         ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT6|BIT0)) > BIT6)
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index 33e8ee2d2c..b7beaf935b 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -100,6 +100,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
 };
 
 PAGE_TABLE_POOL                   *mPageTablePool = NULL;
+BOOLEAN                           mPageTablePoolLock = FALSE;
 PAGE_TABLE_LIB_PAGING_CONTEXT     mPagingContext;
 EFI_SMM_BASE2_PROTOCOL            *mSmmBase2 = NULL;
 
@@ -1046,6 +1047,16 @@ InitializePageTablePool (
   VOID                      *Buffer;
   BOOLEAN                   IsModified;
 
+  //
+  // Do not allow re-entrance.
+  //
+  if (mPageTablePoolLock) {
+    return FALSE;
+  }
+
+  mPageTablePoolLock = TRUE;
+  IsModified = FALSE;
+
   //
   // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
   // header.
@@ -1056,7 +1067,9 @@ InitializePageTablePool (
   Buffer = AllocateAlignedPages (PoolPages, PAGE_TABLE_POOL_ALIGNMENT);
   if (Buffer == NULL) {
     DEBUG ((DEBUG_ERROR, "ERROR: Out of aligned pages\r\n"));
-    return FALSE;
+    goto Done;
+  } else {
+    DEBUG ((DEBUG_INFO, "Paging: added %ld pages to page table pool\r\n", (UINT64)PoolPages));
   }
 
   //
@@ -1092,7 +1105,9 @@ InitializePageTablePool (
     );
   ASSERT (IsModified == TRUE);
 
-  return TRUE;
+Done:
+  mPageTablePoolLock = FALSE;
+  return IsModified;
 }
 
 /**
-- 
2.16.2.windows.1

[PATCH v2 3/5] MdeModulePkg/Core: fix a lock issue in GCD memory map dump

[Jian J Wang]

> v2 changes:
> a. Update implementation of CoreGetMemorySpaceMap() and 
>    CoreGetIoSpaceMap() to avoid lock failure. Drop the code to
>    detect debug print level used to achieve the same effect.

This issue is hidden in current code but exposed by introduction
of freed-memory guard feature due to the fact that the feature
will turn all pool allocation to page allocation.

The solution is move the memory allocation in CoreGetMemorySpaceMap()
and CoreGetIoSpaceMap() to be out of the GCD memory map lock.

   CoreDumpGcdMemorySpaceMap()
=> CoreGetMemorySpaceMap()
=> CoreAcquireGcdMemoryLock () *
   AllocatePool()
=> InternalAllocatePool()
=> CoreAllocatePool()
=> CoreAllocatePoolI()
=> CoreAllocatePoolPagesI()
=> CoreAllocatePoolPages()
=> FindFreePages()
=> PromoteMemoryResource()
=> CoreAcquireGcdMemoryLock()  **

Cc: Star Zeng <star.zeng@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 140 ++++++++++++++++++++++++----------------
 1 file changed, 86 insertions(+), 54 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index d9c65a8045..133c3dcd87 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -1691,10 +1691,10 @@ CoreGetMemorySpaceMap (
   OUT EFI_GCD_MEMORY_SPACE_DESCRIPTOR  **MemorySpaceMap
   )
 {
-  EFI_STATUS                       Status;
   LIST_ENTRY                       *Link;
   EFI_GCD_MAP_ENTRY                *Entry;
   EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *Descriptor;
+  UINTN                            Number;
 
   //
   // Make sure parameters are valid
@@ -1706,38 +1706,54 @@ CoreGetMemorySpaceMap (
     return EFI_INVALID_PARAMETER;
   }
 
-  CoreAcquireGcdMemoryLock ();
-
   //
   // Count the number of descriptors
   //
-  *NumberOfDescriptors = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
+  Number = 0;
+  *NumberOfDescriptors = 0;
+  *MemorySpaceMap = NULL;
+  while (TRUE) {
+    //
+    // Allocate the MemorySpaceMap
+    //
+    if (Number != 0) {
+      if (*MemorySpaceMap != NULL) {
+        FreePool (*MemorySpaceMap);
+      }
 
-  //
-  // Allocate the MemorySpaceMap
-  //
-  *MemorySpaceMap = AllocatePool (*NumberOfDescriptors * sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
-  if (*MemorySpaceMap == NULL) {
-    Status = EFI_OUT_OF_RESOURCES;
-    goto Done;
-  }
+      *MemorySpaceMap = AllocatePool (Number * sizeof(EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
+      if (*MemorySpaceMap == NULL) {
+        return EFI_OUT_OF_RESOURCES;
+      }
 
-  //
-  // Fill in the MemorySpaceMap
-  //
-  Descriptor = *MemorySpaceMap;
-  Link = mGcdMemorySpaceMap.ForwardLink;
-  while (Link != &mGcdMemorySpaceMap) {
-    Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
-    BuildMemoryDescriptor (Descriptor, Entry);
-    Descriptor++;
-    Link = Link->ForwardLink;
+      *NumberOfDescriptors = Number;
+    }
+
+    CoreAcquireGcdMemoryLock ();
+
+    Number = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
+    if (Number == *NumberOfDescriptors) {
+      //
+      // Fill in the MemorySpaceMap
+      //
+      Descriptor = *MemorySpaceMap;
+      Link = mGcdMemorySpaceMap.ForwardLink;
+      while (Link != &mGcdMemorySpaceMap && Number != 0) {
+        Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
+        BuildMemoryDescriptor (Descriptor, Entry);
+        Descriptor++;
+        Number--;
+        Link = Link->ForwardLink;
+      }
+
+      CoreReleaseGcdMemoryLock ();
+      break;
+    }
+
+    CoreReleaseGcdMemoryLock ();
   }
-  Status = EFI_SUCCESS;
 
-Done:
-  CoreReleaseGcdMemoryLock ();
-  return Status;
+  return EFI_SUCCESS;
 }
 
 
@@ -1964,10 +1980,10 @@ CoreGetIoSpaceMap (
   OUT EFI_GCD_IO_SPACE_DESCRIPTOR  **IoSpaceMap
   )
 {
-  EFI_STATUS                   Status;
   LIST_ENTRY                   *Link;
   EFI_GCD_MAP_ENTRY            *Entry;
   EFI_GCD_IO_SPACE_DESCRIPTOR  *Descriptor;
+  UINTN                        Number;
 
   //
   // Make sure parameters are valid
@@ -1979,38 +1995,54 @@ CoreGetIoSpaceMap (
     return EFI_INVALID_PARAMETER;
   }
 
-  CoreAcquireGcdIoLock ();
+  Number = 0;
+  *NumberOfDescriptors = 0;
+  *IoSpaceMap = NULL;
+  while (TRUE) {
+    //
+    // Allocate the IoSpaceMap
+    //
+    if (Number != 0) {
+      if (*IoSpaceMap != NULL) {
+        FreePool (*IoSpaceMap);
+      }
 
-  //
-  // Count the number of descriptors
-  //
-  *NumberOfDescriptors = CoreCountGcdMapEntry (&mGcdIoSpaceMap);
+      *IoSpaceMap = AllocatePool (Number * sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR));
+      if (*IoSpaceMap == NULL) {
+        return EFI_OUT_OF_RESOURCES;
+      }
 
-  //
-  // Allocate the IoSpaceMap
-  //
-  *IoSpaceMap = AllocatePool (*NumberOfDescriptors * sizeof (EFI_GCD_IO_SPACE_DESCRIPTOR));
-  if (*IoSpaceMap == NULL) {
-    Status = EFI_OUT_OF_RESOURCES;
-    goto Done;
-  }
+      *NumberOfDescriptors = Number;
+    }
 
-  //
-  // Fill in the IoSpaceMap
-  //
-  Descriptor = *IoSpaceMap;
-  Link = mGcdIoSpaceMap.ForwardLink;
-  while (Link != &mGcdIoSpaceMap) {
-    Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
-    BuildIoDescriptor (Descriptor, Entry);
-    Descriptor++;
-    Link = Link->ForwardLink;
+    CoreAcquireGcdIoLock ();
+
+    //
+    // Count the number of descriptors
+    //
+    Number = CoreCountGcdMapEntry (&mGcdIoSpaceMap);
+    if (Number == *NumberOfDescriptors) {
+      //
+      // Fill in the IoSpaceMap
+      //
+      Descriptor = *IoSpaceMap;
+      Link = mGcdIoSpaceMap.ForwardLink;
+      while (Link != &mGcdIoSpaceMap && Number != 0) {
+        Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
+        BuildIoDescriptor (Descriptor, Entry);
+        Descriptor++;
+        Number--;
+        Link = Link->ForwardLink;
+      }
+
+      CoreReleaseGcdIoLock ();
+      break;
+    }
+
+    CoreReleaseGcdIoLock ();
   }
-  Status = EFI_SUCCESS;
 
-Done:
-  CoreReleaseGcdIoLock ();
-  return Status;
+  return EFI_SUCCESS;
 }
 
 
-- 
2.16.2.windows.1

[PATCH v2 4/5] MdeModulePkg/Core: add freed-memory guard feature

[Jian J Wang]

> v2 changes:
> a. Change prototype and implementation of IsHeapGuardEnabled()
>    to allow it to check freed-memory guard feature.
> b. Drop IsUafEnabled() because of a.
> c. Move the sanity check of freed-memory guard and heap guard
>    into HeapGuardCpuArchProtocolNotify()
> d. Add GuardFreedPagesChecked() to avoid duplicate feature check
> e. Coding style cleanup

Freed-memory guard is used to detect UAF (Use-After-Free) memory issue
which is illegal access to memory which has been freed. The principle
behind is similar to heap guard feature, that is we'll turn all pool
memory allocation to page allocation and mark them to be not-present
once they are freed.

This also implies that, once a page is allocated and freed, it cannot
be re-allocated. This will bring another issue, which is that there's
risk that memory space will be used out. To address it, the memory
service add logic to put part (at most 64 pages a time) of freed pages
back into page pool, so that the memory service can still have memory
to allocate, when all memory space have been allocated once. This is
called memory promotion. The promoted pages are always from the eldest
pages which haven been freed.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.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 | 409 +++++++++++++++++++++++++++++++++-
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.h |  63 +++++-
 MdeModulePkg/Core/Dxe/Mem/Page.c      |  41 +++-
 MdeModulePkg/Core/Dxe/Mem/Pool.c      |  21 +-
 4 files changed, 513 insertions(+), 21 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index 663f969c0d..5666420a6d 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -44,6 +44,11 @@ GLOBAL_REMOVE_IF_UNREFERENCED UINTN mLevelShift[GUARDED_HEAP_MAP_TABLE_DEPTH]
 GLOBAL_REMOVE_IF_UNREFERENCED UINTN mLevelMask[GUARDED_HEAP_MAP_TABLE_DEPTH]
                                     = GUARDED_HEAP_MAP_TABLE_DEPTH_MASKS;
 
+//
+// Used for promoting freed but not used pages.
+//
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_PHYSICAL_ADDRESS mLastPromotedPage = BASE_4GB;
+
 /**
   Set corresponding bits in bitmap table to 1 according to the address.
 
@@ -379,7 +384,7 @@ ClearGuardedMemoryBits (
 
   @return An integer containing the guarded memory bitmap.
 **/
-UINTN
+UINT64
 GetGuardedMemoryBits (
   IN EFI_PHYSICAL_ADDRESS    Address,
   IN UINTN                   NumberOfPages
@@ -387,7 +392,7 @@ GetGuardedMemoryBits (
 {
   UINT64            *BitMap;
   UINTN             Bits;
-  UINTN             Result;
+  UINT64            Result;
   UINTN             Shift;
   UINTN             BitsToUnitEnd;
 
@@ -660,15 +665,16 @@ IsPageTypeToGuard (
 /**
   Check to see if the heap guard is enabled for page and/or pool allocation.
 
+  @param[in]  GuardType   Specify the sub-type(s) of Heap Guard.
+
   @return TRUE/FALSE.
 **/
 BOOLEAN
 IsHeapGuardEnabled (
-  VOID
+  UINT8           GuardType
   )
 {
-  return IsMemoryTypeToGuard (EfiMaxMemoryType, AllocateAnyPages,
-                              GUARD_HEAP_TYPE_POOL|GUARD_HEAP_TYPE_PAGE);
+  return IsMemoryTypeToGuard (EfiMaxMemoryType, AllocateAnyPages, GuardType);
 }
 
 /**
@@ -1203,6 +1209,380 @@ SetAllGuardPages (
   }
 }
 
+/**
+  Find the address of top-most guarded free page.
+
+  @param[out]  Address    Start address of top-most guarded free page.
+
+  @return VOID.
+**/
+VOID
+GetLastGuardedFreePageAddress (
+  OUT EFI_PHYSICAL_ADDRESS      *Address
+  )
+{
+  EFI_PHYSICAL_ADDRESS    AddressGranularity;
+  EFI_PHYSICAL_ADDRESS    BaseAddress;
+  UINTN                   Level;
+  UINT64                  Map;
+  INTN                    Index;
+
+  ASSERT (mMapLevel >= 1);
+
+  BaseAddress = 0;
+  Map = mGuardedMemoryMap;
+  for (Level = GUARDED_HEAP_MAP_TABLE_DEPTH - mMapLevel;
+       Level < GUARDED_HEAP_MAP_TABLE_DEPTH;
+       ++Level) {
+    AddressGranularity = LShiftU64 (1, mLevelShift[Level]);
+
+    //
+    // Find the non-NULL entry at largest index.
+    //
+    for (Index = (INTN)mLevelMask[Level]; Index >= 0 ; --Index) {
+      if (((UINT64 *)(UINTN)Map)[Index] != 0) {
+        BaseAddress += MultU64x32 (AddressGranularity, (UINT32)Index);
+        Map = ((UINT64 *)(UINTN)Map)[Index];
+        break;
+      }
+    }
+  }
+
+  //
+  // Find the non-zero MSB then get the page address.
+  //
+  while (Map != 0) {
+    Map = RShiftU64 (Map, 1);
+    BaseAddress += EFI_PAGES_TO_SIZE (1);
+  }
+
+  *Address = BaseAddress;
+}
+
+/**
+  Record freed pages.
+
+  @param[in]  BaseAddress   Base address of just freed pages.
+  @param[in]  Pages         Number of freed pages.
+
+  @return VOID.
+**/
+VOID
+MarkFreedPages (
+  IN EFI_PHYSICAL_ADDRESS     BaseAddress,
+  IN UINTN                    Pages
+  )
+{
+  SetGuardedMemoryBits (BaseAddress, Pages);
+}
+
+/**
+  Record freed pages as well as mark them as not-present.
+
+  @param[in]  BaseAddress   Base address of just freed pages.
+  @param[in]  Pages         Number of freed pages.
+
+  @return VOID.
+**/
+VOID
+EFIAPI
+GuardFreedPages (
+  IN  EFI_PHYSICAL_ADDRESS    BaseAddress,
+  IN  UINTN                   Pages
+  )
+{
+  EFI_STATUS      Status;
+
+  //
+  // Legacy memory lower than 1MB might be accessed with no allocation. Leave
+  // them alone.
+  //
+  if (BaseAddress < BASE_1MB) {
+    return;
+  }
+
+  MarkFreedPages (BaseAddress, Pages);
+  if (gCpu != NULL) {
+    //
+    // Set flag to make sure allocating memory without GUARD for page table
+    // operation; otherwise infinite loops could be caused.
+    //
+    mOnGuarding = TRUE;
+    //
+    // Note: This might overwrite other attributes needed by other features,
+    // such as NX memory protection.
+    //
+    Status = gCpu->SetMemoryAttributes (
+                     gCpu,
+                     BaseAddress,
+                     EFI_PAGES_TO_SIZE (Pages),
+                     EFI_MEMORY_RP
+                     );
+    //
+    // Normally we should ASSERT the returned Status. But there might be memory
+    // alloc/free involved in SetMemoryAttributes(), which might fail this
+    // calling. It's rare case so it's OK to let a few tiny holes be not-guarded.
+    //
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_WARN, "Failed to guard freed pages: %p (%ld)\n", BaseAddress, (UINT64)Pages));
+    }
+    mOnGuarding = FALSE;
+  }
+}
+
+/**
+  Record freed pages as well as mark them as not-present, if enabled.
+
+  @param[in]  BaseAddress   Base address of just freed pages.
+  @param[in]  Pages         Number of freed pages.
+
+  @return VOID.
+**/
+VOID
+EFIAPI
+GuardFreedPagesChecked (
+  IN  EFI_PHYSICAL_ADDRESS    BaseAddress,
+  IN  UINTN                   Pages
+  )
+{
+  if (IsHeapGuardEnabled (GUARD_HEAP_TYPE_FREED)) {
+    GuardFreedPages (BaseAddress, Pages);
+  }
+}
+
+/**
+  Mark all pages freed before CPU Arch Protocol as not-present.
+
+**/
+VOID
+GuardAllFreedPages (
+  VOID
+  )
+{
+  UINTN     Entries[GUARDED_HEAP_MAP_TABLE_DEPTH];
+  UINTN     Shifts[GUARDED_HEAP_MAP_TABLE_DEPTH];
+  UINTN     Indices[GUARDED_HEAP_MAP_TABLE_DEPTH];
+  UINT64    Tables[GUARDED_HEAP_MAP_TABLE_DEPTH];
+  UINT64    Addresses[GUARDED_HEAP_MAP_TABLE_DEPTH];
+  UINT64    TableEntry;
+  UINT64    Address;
+  UINT64    GuardPage;
+  INTN      Level;
+  UINTN     BitIndex;
+  UINTN     GuardPageNumber;
+
+  if (mGuardedMemoryMap == 0 ||
+      mMapLevel == 0 ||
+      mMapLevel > GUARDED_HEAP_MAP_TABLE_DEPTH) {
+    return;
+  }
+
+  CopyMem (Entries, mLevelMask, sizeof (Entries));
+  CopyMem (Shifts, mLevelShift, sizeof (Shifts));
+
+  SetMem (Tables, sizeof(Tables), 0);
+  SetMem (Addresses, sizeof(Addresses), 0);
+  SetMem (Indices, sizeof(Indices), 0);
+
+  Level           = GUARDED_HEAP_MAP_TABLE_DEPTH - mMapLevel;
+  Tables[Level]   = mGuardedMemoryMap;
+  Address         = 0;
+  GuardPage       = (UINT64)-1;
+  GuardPageNumber = 0;
+
+  while (TRUE) {
+    if (Indices[Level] > Entries[Level]) {
+      Tables[Level] = 0;
+      Level        -= 1;
+    } else {
+      TableEntry  = ((UINT64 *)(UINTN)(Tables[Level]))[Indices[Level]];
+      Address     = Addresses[Level];
+
+      if (Level < GUARDED_HEAP_MAP_TABLE_DEPTH - 1) {
+        Level            += 1;
+        Tables[Level]     = TableEntry;
+        Addresses[Level]  = Address;
+        Indices[Level]    = 0;
+
+        continue;
+      } else {
+        BitIndex = 1;
+        while (BitIndex != 0) {
+          if ((TableEntry & BitIndex) != 0) {
+            if (GuardPage == (UINT64)-1) {
+              GuardPage = Address;
+            }
+            ++GuardPageNumber;
+          } else if (GuardPageNumber > 0) {
+            GuardFreedPages (GuardPage, GuardPageNumber);
+            GuardPageNumber = 0;
+            GuardPage       = (UINT64)-1;
+          }
+
+          if (TableEntry == 0) {
+            break;
+          }
+
+          Address += EFI_PAGES_TO_SIZE (1);
+          BitIndex = LShiftU64 (BitIndex, 1);
+        }
+      }
+    }
+
+    if (Level < (GUARDED_HEAP_MAP_TABLE_DEPTH - (INTN)mMapLevel)) {
+      break;
+    }
+
+    Indices[Level] += 1;
+    Address = (Level == 0) ? 0 : Addresses[Level - 1];
+    Addresses[Level] = Address | LShiftU64 (Indices[Level], Shifts[Level]);
+
+  }
+
+  //
+  // Update the maximum address of freed page which can be used for memory
+  // promotion upon out-of-memory-space.
+  //
+  GetLastGuardedFreePageAddress (&Address);
+  if (Address != 0) {
+    mLastPromotedPage = Address;
+  }
+}
+
+/**
+  This function checks to see if the given memory map descriptor in a memory map
+  can be merged with any guarded free pages.
+
+  @param  MemoryMapEntry    A pointer to a descriptor in MemoryMap.
+  @param  MaxAddress        Maximum address to stop the merge.
+
+  @return VOID
+
+**/
+VOID
+MergeGuardPages (
+  IN EFI_MEMORY_DESCRIPTOR      *MemoryMapEntry,
+  IN EFI_PHYSICAL_ADDRESS       MaxAddress
+  )
+{
+  EFI_PHYSICAL_ADDRESS        EndAddress;
+  UINT64                      Bitmap;
+  INTN                        Pages;
+
+  if (!IsHeapGuardEnabled (GUARD_HEAP_TYPE_FREED) ||
+      MemoryMapEntry->Type >= EfiMemoryMappedIO) {
+    return;
+  }
+
+  Bitmap = 0;
+  Pages  = EFI_SIZE_TO_PAGES (MaxAddress - MemoryMapEntry->PhysicalStart);
+  Pages -= MemoryMapEntry->NumberOfPages;
+  while (Pages > 0) {
+    if (Bitmap == 0) {
+      EndAddress = MemoryMapEntry->PhysicalStart +
+                   EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages);
+      Bitmap = GetGuardedMemoryBits (EndAddress, GUARDED_HEAP_MAP_ENTRY_BITS);
+    }
+
+    if ((Bitmap & 1) == 0) {
+      break;
+    }
+
+    Pages--;
+    MemoryMapEntry->NumberOfPages++;
+    Bitmap = RShiftU64 (Bitmap, 1);
+  }
+}
+
+/**
+  Put part (at most 64 pages a time) guarded free pages back to free page pool.
+
+  Freed memory guard is used to detect Use-After-Free (UAF) memory issue, which
+  makes use of 'Used then throw away' way to detect any illegal access to freed
+  memory. The thrown-away memory will be marked as not-present so that any access
+  to those memory (after free) will be caught by page-fault exception.
+
+  The problem is that this will consume lots of memory space. Once no memory
+  left in pool to allocate, we have to restore part of the freed pages to their
+  normal function. Otherwise the whole system will stop functioning.
+
+  @param  StartAddress    Start address of promoted memory.
+  @param  EndAddress      End address of promoted memory.
+
+  @return TRUE    Succeeded to promote memory.
+  @return FALSE   No free memory found.
+
+**/
+BOOLEAN
+PromoteGuardedFreePages (
+  OUT EFI_PHYSICAL_ADDRESS      *StartAddress,
+  OUT EFI_PHYSICAL_ADDRESS      *EndAddress
+  )
+{
+  EFI_STATUS              Status;
+  UINTN                   AvailablePages;
+  UINT64                  Bitmap;
+  EFI_PHYSICAL_ADDRESS    Start;
+
+  if (!IsHeapGuardEnabled (GUARD_HEAP_TYPE_FREED)) {
+    return FALSE;
+  }
+
+  //
+  // Similar to memory allocation service, always search the freed pages in
+  // descending direction.
+  //
+  Start           = mLastPromotedPage;
+  AvailablePages  = 0;
+  while (AvailablePages == 0) {
+    Start -= EFI_PAGES_TO_SIZE (GUARDED_HEAP_MAP_ENTRY_BITS);
+    //
+    // If the address wraps around, try the really freed pages at top.
+    //
+    if (Start > mLastPromotedPage) {
+      GetLastGuardedFreePageAddress (&Start);
+      ASSERT (Start != 0);
+      Start -= EFI_PAGES_TO_SIZE (GUARDED_HEAP_MAP_ENTRY_BITS);
+    }
+
+    Bitmap = GetGuardedMemoryBits (Start, GUARDED_HEAP_MAP_ENTRY_BITS);
+    while (Bitmap > 0) {
+      if ((Bitmap & 1) != 0) {
+        ++AvailablePages;
+      } else if (AvailablePages == 0) {
+        Start += EFI_PAGES_TO_SIZE (1);
+      } else {
+        break;
+      }
+
+      Bitmap = RShiftU64 (Bitmap, 1);
+    }
+  }
+
+  if (AvailablePages) {
+    DEBUG ((DEBUG_INFO, "Promoted pages: %lX (%lx)\r\n", Start, (UINT64)AvailablePages));
+    ClearGuardedMemoryBits (Start, AvailablePages);
+
+    if (gCpu != NULL) {
+      //
+      // Set flag to make sure allocating memory without GUARD for page table
+      // operation; otherwise infinite loops could be caused.
+      //
+      mOnGuarding = TRUE;
+      Status = gCpu->SetMemoryAttributes (gCpu, Start, EFI_PAGES_TO_SIZE(AvailablePages), 0);
+      ASSERT_EFI_ERROR (Status);
+      mOnGuarding = FALSE;
+    }
+
+    mLastPromotedPage = Start;
+    *StartAddress     = Start;
+    *EndAddress       = Start + EFI_PAGES_TO_SIZE (AvailablePages) - 1;
+    return TRUE;
+  }
+
+  return FALSE;
+}
+
 /**
   Notify function used to set all Guard pages before CPU Arch Protocol installed.
 **/
@@ -1212,7 +1592,20 @@ HeapGuardCpuArchProtocolNotify (
   )
 {
   ASSERT (gCpu != NULL);
-  SetAllGuardPages ();
+
+  if (IsHeapGuardEnabled (GUARD_HEAP_TYPE_PAGE|GUARD_HEAP_TYPE_POOL) &&
+      IsHeapGuardEnabled (GUARD_HEAP_TYPE_FREED)) {
+    DEBUG ((DEBUG_ERROR, "Heap guard and freed memory guard cannot be enabled at the same time.\n"));
+    CpuDeadLoop ();
+  }
+
+  if (IsHeapGuardEnabled (GUARD_HEAP_TYPE_PAGE|GUARD_HEAP_TYPE_POOL)) {
+    SetAllGuardPages ();
+  }
+
+  if (IsHeapGuardEnabled (GUARD_HEAP_TYPE_FREED)) {
+    GuardAllFreedPages ();
+  }
 }
 
 /**
@@ -1264,6 +1657,10 @@ DumpGuardedMemoryBitmap (
   CHAR8     *Ruler1;
   CHAR8     *Ruler2;
 
+  if (!IsHeapGuardEnabled (GUARD_HEAP_TYPE_ALL)) {
+    return;
+  }
+
   if (mGuardedMemoryMap == 0 ||
       mMapLevel == 0 ||
       mMapLevel > GUARDED_HEAP_MAP_TABLE_DEPTH) {
diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
index 8c34692439..4abcf3a77d 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
@@ -160,6 +160,9 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 //
 #define GUARD_HEAP_TYPE_PAGE        BIT0
 #define GUARD_HEAP_TYPE_POOL        BIT1
+#define GUARD_HEAP_TYPE_FREED       BIT4
+#define GUARD_HEAP_TYPE_ALL         \
+        (GUARD_HEAP_TYPE_PAGE|GUARD_HEAP_TYPE_POOL|GUARD_HEAP_TYPE_FREED)
 
 //
 // Debug message level
@@ -392,11 +395,13 @@ AdjustPoolHeadF (
 /**
   Check to see if the heap guard is enabled for page and/or pool allocation.
 
+  @param[in]  GuardType   Specify the sub-type(s) of Heap Guard.
+
   @return TRUE/FALSE.
 **/
 BOOLEAN
 IsHeapGuardEnabled (
-  VOID
+  UINT8           GuardType
   );
 
 /**
@@ -407,6 +412,62 @@ HeapGuardCpuArchProtocolNotify (
   VOID
   );
 
+/**
+  This function checks to see if the given memory map descriptor in a memory map
+  can be merged with any guarded free pages.
+
+  @param  MemoryMapEntry    A pointer to a descriptor in MemoryMap.
+  @param  MaxAddress        Maximum address to stop the merge.
+
+  @return VOID
+
+**/
+VOID
+MergeGuardPages (
+  IN EFI_MEMORY_DESCRIPTOR      *MemoryMapEntry,
+  IN EFI_PHYSICAL_ADDRESS       MaxAddress
+  );
+
+/**
+  Record freed pages as well as mark them as not-present, if enabled.
+
+  @param[in]  BaseAddress   Base address of just freed pages.
+  @param[in]  Pages         Number of freed pages.
+
+  @return VOID.
+**/
+VOID
+EFIAPI
+GuardFreedPagesChecked (
+  IN  EFI_PHYSICAL_ADDRESS    BaseAddress,
+  IN  UINTN                   Pages
+  );
+
+/**
+  Put part (at most 64 pages a time) guarded free pages back to free page pool.
+
+  Freed memory guard is used to detect Use-After-Free (UAF) memory issue, which
+  makes use of 'Used then throw away' way to detect any illegal access to freed
+  memory. The thrown-away memory will be marked as not-present so that any access
+  to those memory (after free) will be caught by page-fault exception.
+
+  The problem is that this will consume lots of memory space. Once no memory
+  left in pool to allocate, we have to restore part of the freed pages to their
+  normal function. Otherwise the whole system will stop functioning.
+
+  @param  StartAddress    Start address of promoted memory.
+  @param  EndAddress      End address of promoted memory.
+
+  @return TRUE    Succeeded to promote memory.
+  @return FALSE   No free memory found.
+
+**/
+BOOLEAN
+PromoteGuardedFreePages (
+  OUT EFI_PHYSICAL_ADDRESS      *StartAddress,
+  OUT EFI_PHYSICAL_ADDRESS      *EndAddress
+  );
+
 extern BOOLEAN mOnGuarding;
 
 #endif
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 3b4cc08e7c..9d9abcf565 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -401,9 +401,11 @@ PromoteMemoryResource (
   VOID
   )
 {
-  LIST_ENTRY         *Link;
-  EFI_GCD_MAP_ENTRY  *Entry;
-  BOOLEAN            Promoted;
+  LIST_ENTRY            *Link;
+  EFI_GCD_MAP_ENTRY     *Entry;
+  BOOLEAN               Promoted;
+  EFI_PHYSICAL_ADDRESS  StartAddress;
+  EFI_PHYSICAL_ADDRESS  EndAddress;
 
   DEBUG ((DEBUG_PAGE, "Promote the memory resource\n"));
 
@@ -451,6 +453,24 @@ PromoteMemoryResource (
 
   CoreReleaseGcdMemoryLock ();
 
+  if (!Promoted) {
+    //
+    // If freed-memory guard is enabled, we could promote pages from
+    // guarded free pages.
+    //
+    Promoted = PromoteGuardedFreePages (&StartAddress, &EndAddress);
+    if (Promoted) {
+      CoreAcquireGcdMemoryLock ();
+      CoreAddRange (
+        EfiConventionalMemory,
+        StartAddress,
+        EndAddress,
+        EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB
+        );
+      CoreReleaseGcdMemoryLock ();
+    }
+  }
+
   return Promoted;
 }
 /**
@@ -896,9 +916,15 @@ CoreConvertPagesEx (
     }
 
     //
-    // Add our new range in
+    // Add our new range in. Don't do this for freed pages if freed-memory
+    // guard is enabled.
     //
-    CoreAddRange (MemType, Start, RangeEnd, Attribute);
+    if (!IsHeapGuardEnabled (GUARD_HEAP_TYPE_FREED) ||
+        !ChangingType ||
+        MemType != EfiConventionalMemory) {
+      CoreAddRange (MemType, Start, RangeEnd, Attribute);
+    }
+
     if (ChangingType && (MemType == EfiConventionalMemory)) {
       //
       // Avoid calling DEBUG_CLEAR_MEMORY() for an address of 0 because this
@@ -1514,6 +1540,7 @@ CoreFreePages (
 
   Status = CoreInternalFreePages (Memory, NumberOfPages, &MemoryType);
   if (!EFI_ERROR (Status)) {
+    GuardFreedPagesChecked (Memory, NumberOfPages);
     CoreUpdateProfile (
       (EFI_PHYSICAL_ADDRESS) (UINTN) RETURN_ADDRESS (0),
       MemoryProfileActionFreePages,
@@ -1908,9 +1935,7 @@ Done:
   *MemoryMapSize = BufferSize;
 
   DEBUG_CODE (
-    if (PcdGet8 (PcdHeapGuardPropertyMask) & (BIT1|BIT0)) {
-      DumpGuardedMemoryBitmap ();
-    }
+    DumpGuardedMemoryBitmap ();
   );
 
   return Status;
diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index 1ff2061f7f..d47b94b95c 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -26,7 +26,8 @@ typedef struct {
 } POOL_FREE;
 
 
-#define POOL_HEAD_SIGNATURE   SIGNATURE_32('p','h','d','0')
+#define POOL_HEAD_SIGNATURE       SIGNATURE_32('p','h','d','0')
+#define POOLPAGE_HEAD_SIGNATURE   SIGNATURE_32('p','h','d','1')
 typedef struct {
   UINT32          Signature;
   UINT32          Reserved;
@@ -367,6 +368,7 @@ CoreAllocatePoolI (
   UINTN       NoPages;
   UINTN       Granularity;
   BOOLEAN     HasPoolTail;
+  BOOLEAN     PageAsPool;
 
   ASSERT_LOCKED (&mPoolMemoryLock);
 
@@ -386,6 +388,7 @@ CoreAllocatePoolI (
 
   HasPoolTail  = !(NeedGuard &&
                    ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) == 0));
+  PageAsPool = (IsHeapGuardEnabled (GUARD_HEAP_TYPE_FREED) && !mOnGuarding);
 
   //
   // Adjusting the Size to be of proper alignment so that
@@ -406,7 +409,7 @@ CoreAllocatePoolI (
   // If allocation is over max size, just allocate pages for the request
   // (slow)
   //
-  if (Index >= SIZE_TO_LIST (Granularity) || NeedGuard) {
+  if (Index >= SIZE_TO_LIST (Granularity) || NeedGuard || PageAsPool) {
     if (!HasPoolTail) {
       Size -= sizeof (POOL_TAIL);
     }
@@ -498,7 +501,7 @@ Done:
     //
     // If we have a pool buffer, fill in the header & tail info
     //
-    Head->Signature = POOL_HEAD_SIGNATURE;
+    Head->Signature = (PageAsPool) ? POOLPAGE_HEAD_SIGNATURE : POOL_HEAD_SIGNATURE;
     Head->Size      = Size;
     Head->Type      = (EFI_MEMORY_TYPE) PoolType;
     Buffer          = Head->Data;
@@ -615,6 +618,7 @@ CoreFreePoolPagesI (
   CoreFreePoolPages (Memory, NoPages);
   CoreReleaseMemoryLock ();
 
+  GuardFreedPagesChecked (Memory, NoPages);
   ApplyMemoryProtectionPolicy (PoolType, EfiConventionalMemory,
     (EFI_PHYSICAL_ADDRESS)(UINTN)Memory, EFI_PAGES_TO_SIZE (NoPages));
 }
@@ -685,15 +689,19 @@ CoreFreePoolI (
   UINTN       Granularity;
   BOOLEAN     IsGuarded;
   BOOLEAN     HasPoolTail;
+  BOOLEAN     PageAsPool;
 
   ASSERT(Buffer != NULL);
   //
   // Get the head & tail of the pool entry
   //
-  Head = CR (Buffer, POOL_HEAD, Data, POOL_HEAD_SIGNATURE);
+  Head = BASE_CR (Buffer, POOL_HEAD, Data);
   ASSERT(Head != NULL);
 
-  if (Head->Signature != POOL_HEAD_SIGNATURE) {
+  if (Head->Signature != POOL_HEAD_SIGNATURE &&
+      Head->Signature != POOLPAGE_HEAD_SIGNATURE) {
+    ASSERT (Head->Signature == POOL_HEAD_SIGNATURE ||
+            Head->Signature == POOLPAGE_HEAD_SIGNATURE);
     return EFI_INVALID_PARAMETER;
   }
 
@@ -701,6 +709,7 @@ CoreFreePoolI (
                 IsMemoryGuarded ((EFI_PHYSICAL_ADDRESS)(UINTN)Head);
   HasPoolTail = !(IsGuarded &&
                   ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) == 0));
+  PageAsPool = (Head->Signature == POOLPAGE_HEAD_SIGNATURE);
 
   if (HasPoolTail) {
     Tail = HEAD_TO_TAIL (Head);
@@ -757,7 +766,7 @@ CoreFreePoolI (
   //
   // If it's not on the list, it must be pool pages
   //
-  if (Index >= SIZE_TO_LIST (Granularity) || IsGuarded) {
+  if (Index >= SIZE_TO_LIST (Granularity) || IsGuarded || PageAsPool) {
 
     //
     // Return the memory pages back to free memory
-- 
2.16.2.windows.1

[PATCH v2 5/5] MdeModulePkg/Core: fix-up for changes introduced by freed-memory guard

[Jian J Wang]

> v2 changes:
> a. Update IsHeapGuardEnabled() calling because its prototype change

Two changes are fixed up:
a. Prototype change of function IsHeapGuardEnabled()
b. More memory map descriptors are introduced by new feature.
   MergeMemoryMap() is updated to merge freed-pages into adjacent memory
   descriptor to reduce the overall descriptor number.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c |  2 +-
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c  | 16 +++++++---------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index fa8f8fe91a..6298b67db1 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -1250,7 +1250,7 @@ ApplyMemoryProtectionPolicy (
   // Don't overwrite Guard pages, which should be the first and/or last page,
   // if any.
   //
-  if (IsHeapGuardEnabled ()) {
+  if (IsHeapGuardEnabled (GUARD_HEAP_TYPE_PAGE|GUARD_HEAP_TYPE_POOL)) {
     if (IsGuardPage (Memory))  {
       Memory += EFI_PAGE_SIZE;
       Length -= EFI_PAGE_SIZE;
diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
index 05eb4f422b..f6595c90ed 100644
--- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
@@ -32,6 +32,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Guid/PropertiesTable.h>
 
 #include "DxeMain.h"
+#include "HeapGuard.h"
 
 #define PREVIOUS_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
   ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) - (Size)))
@@ -205,16 +206,13 @@ MergeMemoryMap (
     NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
 
     do {
-      MemoryBlockLength = (UINT64) (EfiPagesToSize (MemoryMapEntry->NumberOfPages));
+      MergeGuardPages (NewMemoryMapEntry, NextMemoryMapEntry->PhysicalStart);
+      MemoryBlockLength = (UINT64) (EfiPagesToSize (NewMemoryMapEntry->NumberOfPages));
       if (((UINTN)NextMemoryMapEntry < (UINTN)MemoryMapEnd) &&
-          (MemoryMapEntry->Type == NextMemoryMapEntry->Type) &&
-          (MemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) &&
-          ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) == NextMemoryMapEntry->PhysicalStart)) {
-        MemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
-        if (NewMemoryMapEntry != MemoryMapEntry) {
-          NewMemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
-        }
-
+          (NewMemoryMapEntry->Type == NextMemoryMapEntry->Type) &&
+          (NewMemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) &&
+          ((NewMemoryMapEntry->PhysicalStart + MemoryBlockLength) == NextMemoryMapEntry->PhysicalStart)) {
+        NewMemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
         NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (NextMemoryMapEntry, DescriptorSize);
         continue;
       } else {
-- 
2.16.2.windows.1

Re: [PATCH v2 1/5] MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature

[Laszlo Ersek]

On 10/23/18 16:53, Jian J Wang wrote:
>> v2 changes:
>> a. Drop PCD PcdUseAfterFreeDetectionPropertyMask. Use BIT4 of
>>    PcdHeapGuardPropertyMask instead. Update related descriptions in both
>>    dec and uni files.
>
> Freed-memory guard is used to detect UAF (Use-After-Free) memory issue
> which is illegal access to memory which has been freed. The principle
> behind is similar to heap guard feature, that is we'll turn all pool
> memory allocation to page allocation and mark them to be not-present
> once they are freed.
>
> This also implies that, once a page is allocated and freed, it cannot
> be re-allocated. This will bring another issue, which is that there's
> risk that memory space will be used out. To address it, this patch
> series add logic put part (at most 64 pages a time) of freed pages
> back into page pool, so that the memory service can still have memory
> to allocate, when all memory space have been allocated once. This is
> called memory promotion. The promoted pages are always from the eldest
> pages freed.
>
> BIT4 of following PCD is used to enable the freed-memory guard feature.
>
>   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
>
> Please note this feature cannot be enabled with heap guard at the same
> time.
>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/MdeModulePkg.dec | 10 ++++++++++
>  MdeModulePkg/MdeModulePkg.uni |  6 +++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 6037504fa7..255b92ea67 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -955,6 +955,8 @@
>    # free pages for all of them. The page allocation for the type related to
>    # cleared bits keeps the same as ususal.
>    #
> +  # This PCD is only valid if BIT0 and/or BIT2 are set in PcdHeapGuardPropertyMask.
> +  #
>    # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
>    #  EfiReservedMemoryType             0x0000000000000001<BR>
>    #  EfiLoaderCode                     0x0000000000000002<BR>
> @@ -984,6 +986,8 @@
>    # if there's enough free memory for all of them. The pool allocation for the
>    # type related to cleared bits keeps the same as ususal.
>    #
> +  # This PCD is only valid if BIT1 and/or BIT3 are set in PcdHeapGuardPropertyMask.
> +  #
>    # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
>    #  EfiReservedMemoryType             0x0000000000000001<BR>
>    #  EfiLoaderCode                     0x0000000000000002<BR>
> @@ -1007,14 +1011,20 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType|0x0|UINT64|0x30001053
>
>    ## This mask is to control Heap Guard behavior.
> +  #
>    # Note that due to the limit of pool memory implementation and the alignment
>    # requirement of UEFI spec, BIT7 is a try-best setting which cannot guarantee
>    # that the returned pool is exactly adjacent to head guard page or tail guard
>    # page.

(1) The above changes are not related to the use-after-free feature;
they should go into a separate cleanup patch. (Which is very welcome
otherwise.) The cleanup patch should be patch #1 in the series.

The subject should be, for example:

  MdeModulePkg: clean up Heap Guard PageType / PoolType PCD documentation

(71 characters)

> +  #
> +  # Note that UEFI freed-memory guard and pool/page guard cannot be enabled
> +  # at the same time.
> +  #
>    #   BIT0 - Enable UEFI page guard.<BR>
>    #   BIT1 - Enable UEFI pool guard.<BR>
>    #   BIT2 - Enable SMM page guard.<BR>
>    #   BIT3 - Enable SMM pool guard.<BR>
> +  #   BIT4 - Enable UEFI freed-memory guard (Use-After-Free memory detection).<BR>
>    #   BIT6 - Enable non-stop mode.<BR>
>    #   BIT7 - The direction of Guard Page for Pool Guard.
>    #          0 - The returned pool is near the tail guard page.<BR>

(2) This should go into patch #2 in the series. However, the current
subject line is useless:

  MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature

Instead, I suggest:

  MdeModulePkg: introduce UEFI Use After Free feature bit in Heap Guard PCD

(73 characters).

> diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni
> index a6bcb627cf..e72b893509 100644
> --- a/MdeModulePkg/MdeModulePkg.uni
> +++ b/MdeModulePkg/MdeModulePkg.uni
> @@ -1171,6 +1171,7 @@
>                                                                                          " before and after corresponding type of pages allocated if there's enough\n"
>                                                                                          " free pages for all of them. The page allocation for the type related to\n"
>                                                                                          " cleared bits keeps the same as ususal.\n\n"
> +                                                                                        " This PCD is only valid if BIT0 and/or BIT2 are set in PcdHeapGuardPropertyMask.\n\n"
>                                                                                          " Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>\n"
>                                                                                          "  EfiReservedMemoryType             0x0000000000000001\n"
>                                                                                          "  EfiLoaderCode                     0x0000000000000002\n"
> @@ -1198,6 +1199,7 @@
>                                                                                          " before and after corresponding type of pages which the allocated pool occupies,\n"
>                                                                                          " if there's enough free memory for all of them. The pool allocation for the\n"
>                                                                                          " type related to cleared bits keeps the same as ususal.\n\n"
> +                                                                                        " This PCD is only valid if BIT1 and/or BIT3 are set in PcdHeapGuardPropertyMask.\n\n"
>                                                                                          " Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>\n"
>                                                                                          "  EfiReservedMemoryType             0x0000000000000001\n"
>                                                                                          "  EfiLoaderCode                     0x0000000000000002\n"

(3) Same as (1).

> @@ -1225,11 +1227,13 @@
>                                                                                              "Note that due to the limit of pool memory implementation and the alignment\n"
>                                                                                              "requirement of UEFI spec, BIT7 is a try-best setting which cannot guarantee\n"
>                                                                                              "that the returned pool is exactly adjacent to head guard page or tail guard\n"
> -                                                                                            "page.\n"
> +                                                                                            "page.\n\n"
> +                                                                                            "Note that UEFI freed-memory guard and pool/page guard cannot be enabled at the same time.\n\n"
>                                                                                              "   BIT0 - Enable UEFI page guard.<BR>\n"
>                                                                                              "   BIT1 - Enable UEFI pool guard.<BR>\n"
>                                                                                              "   BIT2 - Enable SMM page guard.<BR>\n"
>                                                                                              "   BIT3 - Enable SMM pool guard.<BR>\n"
> +                                                                                            "   BIT4 - Enable UEFI freed-memory guard (Use-After-Free memory detection).<BR>\n"
>                                                                                              "   BIT7 - The direction of Guard Page for Pool Guard.\n"
>                                                                                              "          0 - The returned pool is near the tail guard page.<BR>\n"
>                                                                                              "          1 - The returned pool is near the head guard page.<BR>"
>

(4) Same as (2).

With those changes:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [PATCH v2 2/5] UefiCpuPkg/CpuDxe: fix an infinite loop issue

[Laszlo Ersek]

On 10/23/18 16:53, Jian J Wang wrote:
>> v2 changes:
>> a. Change the type of mPageTablePoolLock to be BOOLEAN. Related code
>>    logic is also updated and refined.
>> b. Add non-stop mode for freed-memory guard feature
>
> The freed-memory guard feature will cause an infinite calling
> of InitializePageTablePool().

(1) An important statement is missing from the commit message. Namely,
under the UAF guard feature, FreePages() will modify page attributes.

Similarly, the subject line is mostly useless. First, we're not "fixing"
an issue: the issue doesn't exist yet. We're preventing the issue before
the next patches could introduce it. Second, "infinite loop" is way too
generic. Here's a suggestion:

  UefiCpuPkg/CpuDxe: prevent inf. recursion from FreePages->SetMemoryAttributes

(77 characters. A bit longer than I'd like (which is 74), but given the
long function names, it's hard to compress down the subject better.

Also, I don't recommend "inf. loop", we have recursion here, not a plain
loop.)


> This is due to a fact that
> AllocateAlignedPages() is used to allocate page table pool memory.
> This function will most likely call gBS->FreePages to free unaligned
> pages and then cause another round of page attributes change, like
> below
>
>    FreePages() <===============|
> => SetMemoryAttributes()       |
> => <if out of page table>      |
> => InitializePageTablePool()   |
> => AllocateAlignedPages()      |
> => FreePages() ================|
>
> The solution is add a global variable as a lock in page table pool
> allocation function and fail any other requests if it has not been
> done.

(2) Please document what the practical consequences are when the global
variable prevents the infinite recursion. As far as I can see, we are
going to propagate various error codes as far as possible outwards.
Questions:

- Will this error reach the caller (for example, a 3rd party UEFI
driver) if it checks the return status of gBS->FreePages()?

- What is the consequence for the UAF guard? Is it only that the freed
pages will not be marked inaccessible, or something more serious (like
some inconsistency in internal platform firmware structures)?


> This patch also add non-stop mode for freed-memory guard.

(3) Please isolate the update to the non-stop macro
(HEAP_GUARD_NONSTOP_MODE) to a separate patch.

- Non-stop mode was introduced / explained in commit 8f2613628acf
("MdeModulePkg/MdeModulePkg.dec: add new settings for PCDs",
2018-08-30).

- The macro HEAP_GUARD_NONSTOP_MODE was added to CpuDxe in commit
dcc026217fdc ("UefiCpuPkg/CpuDxe: implement non-stop mode for uefi",
2018-08-30).

- Another instance of the macro HEAP_GUARD_NONSTOP_MODE was added to
PiSmmCpuDxeSmm -- with BIT1|BIT0 replaced with BIT3|BIT2 -- in commit
09afd9a42a7f ("UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for
SMM", 2018-08-30)

- According to the first patch in the present series, the UAF guard is
UEFI-only. (Well, "DXE-only" would be more precise; the point is, it
does not cover SMM.)

Thus, the fact that we don't modify the macro definition in
PiSmmCpuDxeSmm, and also *why* we don't do that, should be mentioned
explicitly in the commit message of the patch that modifies
HEAP_GUARD_NONSTOP_MODE in "UefiCpuPkg/CpuDxe/CpuDxe.h":

  UefiCpuPkg/CpuDxe: consider Use After Free guard in non-stop mode

(65 characters)


> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@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>
> ---
>  UefiCpuPkg/CpuDxe/CpuDxe.h       |  2 +-
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 19 +++++++++++++++++--
>  2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
> index 064ea05bba..3183a3f7f4 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.h
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
> @@ -58,7 +58,7 @@
>                                         )
>
>  #define HEAP_GUARD_NONSTOP_MODE       \
> -        ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT1|BIT0)) > BIT6)
> +        ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT4|BIT1|BIT0)) > BIT6)
>
>  #define NULL_DETECTION_NONSTOP_MODE   \
>          ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT6|BIT0)) > BIT6)
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index 33e8ee2d2c..b7beaf935b 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -100,6 +100,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
>  };
>
>  PAGE_TABLE_POOL                   *mPageTablePool = NULL;
> +BOOLEAN                           mPageTablePoolLock = FALSE;
>  PAGE_TABLE_LIB_PAGING_CONTEXT     mPagingContext;
>  EFI_SMM_BASE2_PROTOCOL            *mSmmBase2 = NULL;
>
> @@ -1046,6 +1047,16 @@ InitializePageTablePool (
>    VOID                      *Buffer;
>    BOOLEAN                   IsModified;
>
> +  //
> +  // Do not allow re-entrance.
> +  //
> +  if (mPageTablePoolLock) {
> +    return FALSE;
> +  }
> +
> +  mPageTablePoolLock = TRUE;
> +  IsModified = FALSE;
> +
>    //
>    // Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
>    // header.
> @@ -1056,7 +1067,9 @@ InitializePageTablePool (
>    Buffer = AllocateAlignedPages (PoolPages, PAGE_TABLE_POOL_ALIGNMENT);
>    if (Buffer == NULL) {
>      DEBUG ((DEBUG_ERROR, "ERROR: Out of aligned pages\r\n"));
> -    return FALSE;
> +    goto Done;
> +  } else {
> +    DEBUG ((DEBUG_INFO, "Paging: added %ld pages to page table pool\r\n", (UINT64)PoolPages));
>    }

(4) Please don't add an "else" branch right after a "return" or "goto".
(This is a very annoying, and regrettably frequent, anti-pattern in
edk2.)

(5) Please print UINT64 values with "%lu", not "%ld".

(6) I think this message should be logged at the DEBUG_VERBOSE level. I
don't insist, but please consider it at least.


>    //
> @@ -1092,7 +1105,9 @@ InitializePageTablePool (
>      );
>    ASSERT (IsModified == TRUE);
>
> -  return TRUE;
> +Done:
> +  mPageTablePoolLock = FALSE;
> +  return IsModified;
>  }
>
>  /**
>

Looks OK to me otherwise.

Thanks,
Laszlo

Re: [PATCH v2 5/5] MdeModulePkg/Core: fix-up for changes introduced by freed-memory guard

[Laszlo Ersek]

On 10/23/18 16:53, Jian J Wang wrote:
>> v2 changes:
>> a. Update IsHeapGuardEnabled() calling because its prototype change
> 
> Two changes are fixed up:
> a. Prototype change of function IsHeapGuardEnabled()

This kind of separation, between the patches, is wrong then. If someone
bisects the edk2 git history, and checks out the edk2 tree at patch #4
in this series, they will get a build failure.

> b. More memory map descriptors are introduced by new feature.
>    MergeMemoryMap() is updated to merge freed-pages into adjacent memory
>    descriptor to reduce the overall descriptor number.

In such cases, the usual way to structure the patch series is as follows:

- First the patch is added that makes the dependent / consumer code more
generic, so that it can later cope with the new feature. Right after
this "prep" patch, the new code paths in the consumer code are not
exercised in practice. Importantly however, there is neither a
compilation failure, nor a functionality error. It's just that the new
additions are not active yet; they work as NOPs.

- Second, the patch with the new feature is added; it basically "snaps
in place", and activates the code paths that were prepared earlier.

In large patch sets, it's not uncommon to see 5+ "prep" patches, each
equipping a separate aspect (or consumer site) for the new feature,
gradually. And then the final feature patch is plugged in.

Thanks
Laszlo

> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c |  2 +-
>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c  | 16 +++++++---------
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index fa8f8fe91a..6298b67db1 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -1250,7 +1250,7 @@ ApplyMemoryProtectionPolicy (
>    // Don't overwrite Guard pages, which should be the first and/or last page,
>    // if any.
>    //
> -  if (IsHeapGuardEnabled ()) {
> +  if (IsHeapGuardEnabled (GUARD_HEAP_TYPE_PAGE|GUARD_HEAP_TYPE_POOL)) {
>      if (IsGuardPage (Memory))  {
>        Memory += EFI_PAGE_SIZE;
>        Length -= EFI_PAGE_SIZE;
> diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> index 05eb4f422b..f6595c90ed 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> @@ -32,6 +32,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Guid/PropertiesTable.h>
>  
>  #include "DxeMain.h"
> +#include "HeapGuard.h"
>  
>  #define PREVIOUS_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
>    ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) - (Size)))
> @@ -205,16 +206,13 @@ MergeMemoryMap (
>      NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
>  
>      do {
> -      MemoryBlockLength = (UINT64) (EfiPagesToSize (MemoryMapEntry->NumberOfPages));
> +      MergeGuardPages (NewMemoryMapEntry, NextMemoryMapEntry->PhysicalStart);
> +      MemoryBlockLength = (UINT64) (EfiPagesToSize (NewMemoryMapEntry->NumberOfPages));
>        if (((UINTN)NextMemoryMapEntry < (UINTN)MemoryMapEnd) &&
> -          (MemoryMapEntry->Type == NextMemoryMapEntry->Type) &&
> -          (MemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) &&
> -          ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) == NextMemoryMapEntry->PhysicalStart)) {
> -        MemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
> -        if (NewMemoryMapEntry != MemoryMapEntry) {
> -          NewMemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
> -        }
> -
> +          (NewMemoryMapEntry->Type == NextMemoryMapEntry->Type) &&
> +          (NewMemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) &&
> +          ((NewMemoryMapEntry->PhysicalStart + MemoryBlockLength) == NextMemoryMapEntry->PhysicalStart)) {
> +        NewMemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
>          NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (NextMemoryMapEntry, DescriptorSize);
>          continue;
>        } else {
> 

Re: [PATCH v2 3/5] MdeModulePkg/Core: fix a lock issue in GCD memory map dump

[Laszlo Ersek]

On 10/23/18 16:53, Jian J Wang wrote:
>> v2 changes:
>> a. Update implementation of CoreGetMemorySpaceMap() and
>>    CoreGetIoSpaceMap() to avoid lock failure. Drop the code to
>>    detect debug print level used to achieve the same effect.
>
> This issue is hidden in current code but exposed by introduction
> of freed-memory guard feature due to the fact that the feature
> will turn all pool allocation to page allocation.
>
> The solution is move the memory allocation in CoreGetMemorySpaceMap()
> and CoreGetIoSpaceMap() to be out of the GCD memory map lock.
>
>    CoreDumpGcdMemorySpaceMap()
> => CoreGetMemorySpaceMap()
> => CoreAcquireGcdMemoryLock () *
>    AllocatePool()
> => InternalAllocatePool()
> => CoreAllocatePool()
> => CoreAllocatePoolI()
> => CoreAllocatePoolPagesI()
> => CoreAllocatePoolPages()
> => FindFreePages()
> => PromoteMemoryResource()
> => CoreAcquireGcdMemoryLock()  **
>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 140 ++++++++++++++++++++++++----------------
>  1 file changed, 86 insertions(+), 54 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> index d9c65a8045..133c3dcd87 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -1691,10 +1691,10 @@ CoreGetMemorySpaceMap (
>    OUT EFI_GCD_MEMORY_SPACE_DESCRIPTOR  **MemorySpaceMap
>    )
>  {
> -  EFI_STATUS                       Status;
>    LIST_ENTRY                       *Link;
>    EFI_GCD_MAP_ENTRY                *Entry;
>    EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *Descriptor;
> +  UINTN                            Number;
>
>    //
>    // Make sure parameters are valid
> @@ -1706,38 +1706,54 @@ CoreGetMemorySpaceMap (
>      return EFI_INVALID_PARAMETER;
>    }
>
> -  CoreAcquireGcdMemoryLock ();
> -
>    //
>    // Count the number of descriptors
>    //
> -  *NumberOfDescriptors = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
> +  Number = 0;
> +  *NumberOfDescriptors = 0;
> +  *MemorySpaceMap = NULL;
> +  while (TRUE) {
> +    //
> +    // Allocate the MemorySpaceMap
> +    //
> +    if (Number != 0) {
> +      if (*MemorySpaceMap != NULL) {
> +        FreePool (*MemorySpaceMap);
> +      }
>
> -  //
> -  // Allocate the MemorySpaceMap
> -  //
> -  *MemorySpaceMap = AllocatePool (*NumberOfDescriptors * sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
> -  if (*MemorySpaceMap == NULL) {
> -    Status = EFI_OUT_OF_RESOURCES;
> -    goto Done;
> -  }
> +      *MemorySpaceMap = AllocatePool (Number * sizeof(EFI_GCD_MEMORY_SPACE_DESCRIPTOR));

(1) Side comment: you dropped the space character after "sizeof".

> +      if (*MemorySpaceMap == NULL) {
> +        return EFI_OUT_OF_RESOURCES;
> +      }
>
> -  //
> -  // Fill in the MemorySpaceMap
> -  //
> -  Descriptor = *MemorySpaceMap;
> -  Link = mGcdMemorySpaceMap.ForwardLink;
> -  while (Link != &mGcdMemorySpaceMap) {
> -    Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
> -    BuildMemoryDescriptor (Descriptor, Entry);
> -    Descriptor++;
> -    Link = Link->ForwardLink;
> +      *NumberOfDescriptors = Number;
> +    }
> +
> +    CoreAcquireGcdMemoryLock ();
> +
> +    Number = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
> +    if (Number == *NumberOfDescriptors) {
> +      //
> +      // Fill in the MemorySpaceMap
> +      //
> +      Descriptor = *MemorySpaceMap;
> +      Link = mGcdMemorySpaceMap.ForwardLink;
> +      while (Link != &mGcdMemorySpaceMap && Number != 0) {
> +        Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
> +        BuildMemoryDescriptor (Descriptor, Entry);
> +        Descriptor++;
> +        Number--;
> +        Link = Link->ForwardLink;
> +      }
> +
> +      CoreReleaseGcdMemoryLock ();
> +      break;
> +    }
> +
> +    CoreReleaseGcdMemoryLock ();
>    }
> -  Status = EFI_SUCCESS;
>
> -Done:
> -  CoreReleaseGcdMemoryLock ();
> -  return Status;
> +  return EFI_SUCCESS;
>  }

I think this loop can be improved. Here's the facts that I don't like
about it:

(2) The "Number" variable name is bad. It should be "DescriptorCount".

(3) The ways the outer "if"s are formulated in the loop body are hard to
    read. In particular, I think what bothers me the most is that the
    loop body starts with the memory allocation, and not with the
    CoreCountGcdMapEntry() call. I think we can do better.

(4) We have one location in the code that acquires the lock, but two
    locations that release it. While it is technically correct, it's
    hard to read as well.

(5) Decreasing "Number" in the inner "while" loop, and checking it in
    the inner loop condition, seems strange. The GCD memory space map
    will have at least one entry at all times (minimally there is a
    NonExistent entry that covers the entire address space, according to
    the address width from the CPU HOB). In addition, I don't see when
    it could validly occur that Number doesn't match the length of the
    list.

How about the following instead (I'm quoting the full function, untested
/ uncompiled):

> EFI_STATUS
> EFIAPI
> CoreGetMemorySpaceMap (
>   OUT UINTN                            *NumberOfDescriptors,
>   OUT EFI_GCD_MEMORY_SPACE_DESCRIPTOR  **MemorySpaceMap
>   )
> {
>   LIST_ENTRY                       *Link;
>   EFI_GCD_MAP_ENTRY                *Entry;
>   EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *Descriptor;
>   UINTN                            DescriptorCount;
>
>   //
>   // Make sure parameters are valid
>   //
>   if (NumberOfDescriptors == NULL) {
>     return EFI_INVALID_PARAMETER;
>   }
>   if (MemorySpaceMap == NULL) {
>     return EFI_INVALID_PARAMETER;
>   }
>
>   //
>   // No candidate map allocated yet.
>   //
>   *NumberOfDescriptors = 0;
>   *MemorySpaceMap = NULL;
>
>   //
>   // Take the lock, for entering the loop with the lock held.
>   //
>   CoreAcquireGcdMemoryLock ();
>   while (TRUE) {
>     //
>     // Count the descriptors.
>     //
>     DescriptorCount = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
>
>     if (DescriptorCount == *NumberOfDescriptors) {
>       //
>       // Fill in the MemorySpaceMap
>       //
>       Descriptor = *MemorySpaceMap;
>       Link = mGcdMemorySpaceMap.ForwardLink;
>       while (Link != &mGcdMemorySpaceMap) {
>         Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
>         BuildMemoryDescriptor (Descriptor, Entry);
>         Descriptor++;
>         Link = Link->ForwardLink;
>       }
>
>       //
>       // We're done; exit the loop with the lock held.
>       //
>       break;
>     }
>
>     CoreReleaseGcdMemoryLock ();
>
>     //
>     // Free previous allocation, with the lock released.
>     //
>     if (*MemorySpaceMap != NULL) {
>       FreePool (*MemorySpaceMap);
>     }
>
>     //
>     // Allocate the MemorySpaceMap, with the lock released.
>     //
>     *MemorySpaceMap = AllocatePool (
>                         (DescriptorCount *
>                          sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR))
>                         );
>     if (*MemorySpaceMap == NULL) {
>       *NumberOfDescriptors = 0;
>       return EFI_OUT_OF_RESOURCES;
>     }
>     *NumberOfDescriptors = DescriptorCount;
>
>     //
>     // Re-acquire the lock, for the next iteration.
>     //
>     CoreAcquireGcdMemoryLock ();
>   }
>
>   //
>   // We exited the loop with the lock held, release it.
>   //
>   CoreReleaseGcdMemoryLock ();
>
>   return EFI_SUCCESS;
> }
>

I don't insist on this variant, of course, it's just an idea to discuss!
If others find your variant easier to read, I'm fine with it as well.
(Still, I think points (1), (2) and (5) should be fixed.)


(6) I've snipped the CoreGetIoSpaceMap() changes because, AFAICS, they
mirror the CoreGetMemorySpaceMap() changes. However: do we *really* need
to update CoreGetIoSpaceMap()? Because, allocating pool memory, even if
it ends up allocating page memory, should be independent of the *IO Port
space* in GCD.

If you fix CoreGetMemorySpaceMap() but don't touch CoreGetIoSpaceMap(),
can you actually trigger a deadlock (with or without enabling the UAF
guard)?

Thanks,
Laszlo

Re: [PATCH v2 4/5] MdeModulePkg/Core: add freed-memory guard feature

[Laszlo Ersek]

On 10/23/18 16:53, Jian J Wang wrote:
>> v2 changes:
>> a. Change prototype and implementation of IsHeapGuardEnabled()
>>    to allow it to check freed-memory guard feature.
>> b. Drop IsUafEnabled() because of a.
>> c. Move the sanity check of freed-memory guard and heap guard
>>    into HeapGuardCpuArchProtocolNotify()
>> d. Add GuardFreedPagesChecked() to avoid duplicate feature check
>> e. Coding style cleanup
> 
> Freed-memory guard is used to detect UAF (Use-After-Free) memory issue
> which is illegal access to memory which has been freed. The principle
> behind is similar to heap guard feature, that is we'll turn all pool
> memory allocation to page allocation and mark them to be not-present
> once they are freed.
> 
> This also implies that, once a page is allocated and freed, it cannot
> be re-allocated. This will bring another issue, which is that there's
> risk that memory space will be used out. To address it, the memory
> service add logic to put part (at most 64 pages a time) of freed pages
> back into page pool, so that the memory service can still have memory
> to allocate, when all memory space have been allocated once. This is
> called memory promotion. The promoted pages are always from the eldest
> pages which haven been freed.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.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 | 409 +++++++++++++++++++++++++++++++++-
>  MdeModulePkg/Core/Dxe/Mem/HeapGuard.h |  63 +++++-
>  MdeModulePkg/Core/Dxe/Mem/Page.c      |  41 +++-
>  MdeModulePkg/Core/Dxe/Mem/Pool.c      |  21 +-
>  4 files changed, 513 insertions(+), 21 deletions(-)

I don't know when I will find the time to review this patch. Please make
sure that with BIT4 clear in the PCD, the changes are a no-op.

I'd prefer if you could regression-test the changes on OVMF as well, not
just on physical platforms.

Other than that, until I find the time, please proceed with the normal
review workflow -- feel free to submit further versions, according to
the MdeModulePkg maintainers' comments, and/or even push the final
version, should I prove unable to comment on this patch in time.

Thanks!
Laszlo

Re: [PATCH v2 1/5] MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature

[Wang, Jian J]

Laszlo,

Thank you very much for the comments. I went through all of them in other
patch emails and I think I have no objection. So to save all of us time I'm not
going to respond them separately. I'll send out v3 patch soon. Thanks again.

Regards,
Jian


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, October 24, 2018 12:09 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH v2 1/5] MdeModulePkg/MdeModulePkg.dec: update
> PCD description for new feature
> 
> On 10/23/18 16:53, Jian J Wang wrote:
> >> v2 changes:
> >> a. Drop PCD PcdUseAfterFreeDetectionPropertyMask. Use BIT4 of
> >>    PcdHeapGuardPropertyMask instead. Update related descriptions in both
> >>    dec and uni files.
> >
> > Freed-memory guard is used to detect UAF (Use-After-Free) memory issue
> > which is illegal access to memory which has been freed. The principle
> > behind is similar to heap guard feature, that is we'll turn all pool
> > memory allocation to page allocation and mark them to be not-present
> > once they are freed.
> >
> > This also implies that, once a page is allocated and freed, it cannot
> > be re-allocated. This will bring another issue, which is that there's
> > risk that memory space will be used out. To address it, this patch
> > series add logic put part (at most 64 pages a time) of freed pages
> > back into page pool, so that the memory service can still have memory
> > to allocate, when all memory space have been allocated once. This is
> > called memory promotion. The promoted pages are always from the eldest
> > pages freed.
> >
> > BIT4 of following PCD is used to enable the freed-memory guard feature.
> >
> >   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> >
> > Please note this feature cannot be enabled with heap guard at the same
> > time.
> >
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >  MdeModulePkg/MdeModulePkg.dec | 10 ++++++++++
> >  MdeModulePkg/MdeModulePkg.uni |  6 +++++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> > index 6037504fa7..255b92ea67 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -955,6 +955,8 @@
> >    # free pages for all of them. The page allocation for the type related to
> >    # cleared bits keeps the same as ususal.
> >    #
> > +  # This PCD is only valid if BIT0 and/or BIT2 are set in
> PcdHeapGuardPropertyMask.
> > +  #
> >    # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
> >    #  EfiReservedMemoryType             0x0000000000000001<BR>
> >    #  EfiLoaderCode                     0x0000000000000002<BR>
> > @@ -984,6 +986,8 @@
> >    # if there's enough free memory for all of them. The pool allocation for the
> >    # type related to cleared bits keeps the same as ususal.
> >    #
> > +  # This PCD is only valid if BIT1 and/or BIT3 are set in
> PcdHeapGuardPropertyMask.
> > +  #
> >    # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
> >    #  EfiReservedMemoryType             0x0000000000000001<BR>
> >    #  EfiLoaderCode                     0x0000000000000002<BR>
> > @@ -1007,14 +1011,20 @@
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType|0x0|UINT64|0x30
> 001053
> >
> >    ## This mask is to control Heap Guard behavior.
> > +  #
> >    # Note that due to the limit of pool memory implementation and the
> alignment
> >    # requirement of UEFI spec, BIT7 is a try-best setting which cannot
> guarantee
> >    # that the returned pool is exactly adjacent to head guard page or tail guard
> >    # page.
> 
> (1) The above changes are not related to the use-after-free feature;
> they should go into a separate cleanup patch. (Which is very welcome
> otherwise.) The cleanup patch should be patch #1 in the series.
> 
> The subject should be, for example:
> 
>   MdeModulePkg: clean up Heap Guard PageType / PoolType PCD
> documentation
> 
> (71 characters)
> 
> > +  #
> > +  # Note that UEFI freed-memory guard and pool/page guard cannot be
> enabled
> > +  # at the same time.
> > +  #
> >    #   BIT0 - Enable UEFI page guard.<BR>
> >    #   BIT1 - Enable UEFI pool guard.<BR>
> >    #   BIT2 - Enable SMM page guard.<BR>
> >    #   BIT3 - Enable SMM pool guard.<BR>
> > +  #   BIT4 - Enable UEFI freed-memory guard (Use-After-Free memory
> detection).<BR>
> >    #   BIT6 - Enable non-stop mode.<BR>
> >    #   BIT7 - The direction of Guard Page for Pool Guard.
> >    #          0 - The returned pool is near the tail guard page.<BR>
> 
> (2) This should go into patch #2 in the series. However, the current
> subject line is useless:
> 
>   MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature
> 
> Instead, I suggest:
> 
>   MdeModulePkg: introduce UEFI Use After Free feature bit in Heap Guard PCD
> 
> (73 characters).
> 
> > diff --git a/MdeModulePkg/MdeModulePkg.uni
> b/MdeModulePkg/MdeModulePkg.uni
> > index a6bcb627cf..e72b893509 100644
> > --- a/MdeModulePkg/MdeModulePkg.uni
> > +++ b/MdeModulePkg/MdeModulePkg.uni
> > @@ -1171,6 +1171,7 @@
> >                                                                                          " before and after
> corresponding type of pages allocated if there's enough\n"
> >                                                                                          " free pages for all of them.
> The page allocation for the type related to\n"
> >                                                                                          " cleared bits keeps the same
> as ususal.\n\n"
> > +                                                                                        " This PCD is only valid if BIT0
> and/or BIT2 are set in PcdHeapGuardPropertyMask.\n\n"
> >                                                                                          " Below is bit mask for this
> PCD: (Order is same as UEFI spec)<BR>\n"
> >                                                                                          "  EfiReservedMemoryType
> 0x0000000000000001\n"
> >                                                                                          "  EfiLoaderCode
> 0x0000000000000002\n"
> > @@ -1198,6 +1199,7 @@
> >                                                                                          " before and after
> corresponding type of pages which the allocated pool occupies,\n"
> >                                                                                          " if there's enough free
> memory for all of them. The pool allocation for the\n"
> >                                                                                          " type related to cleared bits
> keeps the same as ususal.\n\n"
> > +                                                                                        " This PCD is only valid if BIT1
> and/or BIT3 are set in PcdHeapGuardPropertyMask.\n\n"
> >                                                                                          " Below is bit mask for this
> PCD: (Order is same as UEFI spec)<BR>\n"
> >                                                                                          "  EfiReservedMemoryType
> 0x0000000000000001\n"
> >                                                                                          "  EfiLoaderCode
> 0x0000000000000002\n"
> 
> (3) Same as (1).
> 
> > @@ -1225,11 +1227,13 @@
> >                                                                                              "Note that due to the limit
> of pool memory implementation and the alignment\n"
> >                                                                                              "requirement of UEFI spec,
> BIT7 is a try-best setting which cannot guarantee\n"
> >                                                                                              "that the returned pool is
> exactly adjacent to head guard page or tail guard\n"
> > -                                                                                            "page.\n"
> > +                                                                                            "page.\n\n"
> > +                                                                                            "Note that UEFI freed-
> memory guard and pool/page guard cannot be enabled at the same time.\n\n"
> >                                                                                              "   BIT0 - Enable UEFI page
> guard.<BR>\n"
> >                                                                                              "   BIT1 - Enable UEFI pool
> guard.<BR>\n"
> >                                                                                              "   BIT2 - Enable SMM page
> guard.<BR>\n"
> >                                                                                              "   BIT3 - Enable SMM pool
> guard.<BR>\n"
> > +                                                                                            "   BIT4 - Enable UEFI
> freed-memory guard (Use-After-Free memory detection).<BR>\n"
> >                                                                                              "   BIT7 - The direction of
> Guard Page for Pool Guard.\n"
> >                                                                                              "          0 - The returned pool
> is near the tail guard page.<BR>\n"
> >                                                                                              "          1 - The returned pool
> is near the head guard page.<BR>"
> >
> 
> (4) Same as (2).
> 
> With those changes:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo