MdeModulePkg: Leak Memory if Not RW on FreePages

Currently, if the DebugClearMemory bit is set in the
PcdDebugPropertyMask, CoreConvertPagesEx will attempt to write
a pattern to the pages being freed. However, it does not check
that the page is writeable, which will cause a page fault if not.
Furthermore, if NX protections are not enabled, the core does not
ensure that any freed pages are RW, which is the state expected
when they are allocated next. If they are not RW, the allocating
driver will crash trying to use them.

This patch updates the page freeing code to query the memory
attributes protocol, if present, for the attributes. If this call
fails or the attributes are not RW at a minimum, the core leaks
the memory (returning success to the caller). If the memory
attribute protocol is not present (either because a platform doesn't
produce it or it is before the protocol has been produced, the core
continues with freeing memory. This is either before the CPU Arch
protocol is available (so drivers can't change memory attributes) or
otherwise matches existing behavior. This was deemed the best
approach to let memory that can't be guaranteed to be RW leak
instead of letting a driver crash when allocating it. It was deemed
less brittle to simply leak the memory instead of attempting to
change the attributes.

Signed-off-by: Oliver Smith-Denny <osde@microsoft.com>

4 files changed, 98 insertions, 0 deletions

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index cd3940d..6f08554 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -45,6 +45,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Protocol/HiiPackageList.h>

 #include <Protocol/SmmBase2.h>

 #include <Protocol/PeCoffImageEmulator.h>

+#include <Protocol/MemoryAttribute.h>

 #include <Guid/MemoryTypeInformation.h>

 #include <Guid/FirmwareFileSystem2.h>

 #include <Guid/FirmwareFileSystem3.h>

@@ -250,6 +251,7 @@ extern EFI_SECURITY_ARCH_PROTOCOL        *gSecurity;
 extern EFI_SECURITY2_ARCH_PROTOCOL       *gSecurity2;

 extern EFI_BDS_ARCH_PROTOCOL             *gBds;

 extern EFI_SMM_BASE2_PROTOCOL            *gSmmBase2;

+extern EFI_MEMORY_ATTRIBUTE_PROTOCOL     *gMemoryAttributeProtocol;

 

 extern EFI_TPL  gEfiCurrentTpl;

 

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index cc315ac..40e7dc8 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -156,6 +156,7 @@
   gEfiHiiPackageListProtocolGuid                ## SOMETIMES_PRODUCES

   gEfiSmmBase2ProtocolGuid                      ## SOMETIMES_CONSUMES

   gEdkiiPeCoffImageEmulatorProtocolGuid         ## SOMETIMES_CONSUMES

+  gEfiMemoryAttributeProtocolGuid               ## CONSUMES

 

   # Arch Protocols

   gEfiBdsArchProtocolGuid                       ## CONSUMES

diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 9df64cc..27c2788 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1746,6 +1746,38 @@ CoreFreePages (
 {

   EFI_STATUS       Status;

   EFI_MEMORY_TYPE  MemoryType;

+  UINT64           Attributes;

+

+  // check if this memory is returned to the core as RW at a minimum. If the memory attribute protocol is not installed,

+  // then we assume that the memory is RW by default and continue to free it.

+  if (gMemoryAttributeProtocol != NULL) {

+    Status = gMemoryAttributeProtocol->GetMemoryAttributes (

+                                         gMemoryAttributeProtocol,

+                                         Memory,

+                                         EFI_PAGES_TO_SIZE (NumberOfPages),

+                                         &Attributes

+                                         );

+

+    // if we failed to get the attributes, or if the memory is read-only or read-protected,

+    // then we leak the memory and return success. This is done because the UEFI spec does not specify whether pages

+    // should be freed with any specific permission attributes. As such, there exist bootloaders in the wild that will

+    // free memory that is marked RO, which can crash the core if DebugClearMemory is enabled or can be passed out to a

+    // driver in the next AllocatePages() call, which can cause a crash later on. It is deemed lower risk to leak the

+    // memory than to attempt to fix up the attributes as that requires syncing the GCD and the page table.

+    if (EFI_ERROR (Status) || (Attributes & EFI_MEMORY_RO) || (Attributes & EFI_MEMORY_RP)) {

+      DEBUG ((

+        DEBUG_WARN,

+        "%a: Memory %llx for %llx Pages failed to get attributes with status %r or it is read-only or read-protected. "

+        "Attributes: %llx. Leaking memory!\n",

+        __func__,

+        Memory,

+        NumberOfPages,

+        Status,

+        Attributes

+        ));

+      return EFI_SUCCESS;

+    }

+  }

 

   Status = CoreInternalFreePages (Memory, NumberOfPages, &MemoryType);

   if (!EFI_ERROR (Status)) {

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 33357eb..69d26cb 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -38,6 +38,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Guid/MemoryAttributesTable.h>

 

 #include <Protocol/FirmwareVolume2.h>

+#include <Protocol/MemoryAttribute.h>

 #include <Protocol/SimpleFileSystem.h>

 

 #include "DxeMain.h"

@@ -67,6 +68,8 @@ extern LIST_ENTRY  mGcdMemorySpaceMap;
 

 STATIC LIST_ENTRY  mProtectedImageRecordList;

 

+EFI_MEMORY_ATTRIBUTE_PROTOCOL  *gMemoryAttributeProtocol;

+

 /**

   Get the image type.

 

@@ -1034,6 +1037,37 @@ DisableNullDetectionAtTheEndOfDxe (
 }

 

 /**

+  A notification for the Memory Attribute Protocol Installation.

+

+  @param[in]  Event                 Event whose notification function is being invoked.

+  @param[in]  Context               Pointer to the notification function's context,

+                                    which is implementation-dependent.

+

+**/

+VOID

+EFIAPI

+MemoryAttributeProtocolNotify (

+  IN EFI_EVENT  Event,

+  IN VOID       *Context

+  )

+{

+  EFI_STATUS  Status;

+

+  Status = gBS->LocateProtocol (&gEfiMemoryAttributeProtocolGuid, NULL, (VOID **)&gMemoryAttributeProtocol);

+

+  if (EFI_ERROR (Status)) {

+    DEBUG ((

+      DEBUG_INFO,

+      "%a - Unable to locate the memory attribute protocol! Status = %r\n",

+      __func__,

+      Status

+      ));

+  }

+

+  CoreCloseEvent (Event);

+}

+

+/**

   Initialize Memory Protection support.

 **/

 VOID

@@ -1084,6 +1118,35 @@ CoreInitializeMemoryProtection (
              );

   ASSERT_EFI_ERROR (Status);

 

+  // Register an event to populate the memory attribute protocol

+  Status = CoreCreateEvent (

+             EVT_NOTIFY_SIGNAL,

+             TPL_CALLBACK,

+             MemoryAttributeProtocolNotify,

+             NULL,

+             &Event

+             );

+

+  // if we fail to create the event or the protocol notify, we should still continue, we won't be able to query the

+  // memory attributes on FreePages(), so we may encounter a driver or bootloader that has not set attributes back to

+  // RW, but this matches the state of the world before this protocol was introduced, so it is not a regression.

+  if (EFI_ERROR (Status)) {

+    DEBUG ((DEBUG_ERROR, "%a - Failed to create event for the Memory Attribute Protocol notification: %r\n", __func__, Status));

+    ASSERT_EFI_ERROR (Status);

+  }

+

+  // Register for protocol notification

+  Status = CoreRegisterProtocolNotify (

+             &gEfiMemoryAttributeProtocolGuid,

+             Event,

+             &Registration

+             );

+

+  if (EFI_ERROR (Status)) {

+    DEBUG ((DEBUG_ERROR, "%a - Failed to register for the Memory Attribute Protocol notification: %r\n", __func__, Status));

+    ASSERT_EFI_ERROR (Status);

+  }

+

   //

   // Register a callback to disable NULL pointer detection at EndOfDxe

   //