ArmPkg/UncachedMemoryAllocationLib: set XP bit via CPU arch protocol

Commit e7b24ec9785d ("ArmPkg/UncachedMemoryAllocationLib: map uncached
allocations non-executable") adds code that manipulates the GCD memory
space attributes of a newly allocated uncached region without checking
whether this region expose these attributes in its capabilities mask.

Given that the intent is to remove executable permissions from the region,
this is a fairly pointless exercise to begin with, regardless of whether
it is correct or not. The reason is that RO/XP memory attributes in the
GCD memory space map or the UEFI memory map are completely disconnected
from the actual mapping permissions used in the page tables.

So instead, invoke the CPU arch protocol directly, and add the non-exec
attributes in the page tables directly.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Tested-by: Ryan Harkin <ryan.harkin@linaro.org>

diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
index b4fbfbcb362bf4656e9e931c7453f12cf2d60cca..fdaaf2d706ab1110bd263539a70d9391f9ce84c4 100644 (file)
--- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
+++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
@@ -27,6 +27,10 @@
 #include <Library/DxeServicesTableLib.h>\r
 #include <Library/CacheMaintenanceLib.h>\r
 \r
+#include <Protocol/Cpu.h>\r
+\r
+STATIC EFI_CPU_ARCH_PROTOCOL    *mCpu;\r
+\r
 VOID *\r
 UncachedInternalAllocatePages (\r
   IN EFI_MEMORY_TYPE  MemoryType,\r
@@ -150,15 +154,25 @@ AllocatePagesFromList (
 \r
   Status = gDS->GetMemorySpaceDescriptor (Memory, &Descriptor);\r
   if (EFI_ERROR (Status)) {\r
-    gBS->FreePages (Memory, Pages);\r
-    return Status;\r
+    goto FreePages;\r
   }\r
 \r
   Status = gDS->SetMemorySpaceAttributes (Memory, EFI_PAGES_TO_SIZE (Pages),\r
-                  EFI_MEMORY_WC | EFI_MEMORY_XP);\r
+                  EFI_MEMORY_WC);\r
   if (EFI_ERROR (Status)) {\r
-    gBS->FreePages (Memory, Pages);\r
-    return Status;\r
+    goto FreePages;\r
+  }\r
+\r
+  //\r
+  // EFI_CPU_ARCH_PROTOCOL::SetMemoryAttributes() will preserve the original\r
+  // memory type attribute if no memory type is passed. Permission attributes\r
+  // will be replaced, so EFI_MEMORY_RO will be removed if present (although\r
+  // it would be a bug if that were the case for an AllocatePages() allocation)\r
+  //\r
+  Status = mCpu->SetMemoryAttributes (mCpu, Memory, EFI_PAGES_TO_SIZE (Pages),\r
+                   EFI_MEMORY_XP);\r
+  if (EFI_ERROR (Status)) {\r
+    goto FreePages;\r
   }\r
 \r
   InvalidateDataCacheRange ((VOID *)(UINTN)Memory, EFI_PAGES_TO_SIZE (Pages));\r
@@ -166,8 +180,8 @@ AllocatePagesFromList (
   NewNode = AllocatePool (sizeof (FREE_PAGE_NODE));\r
   if (NewNode == NULL) {\r
     ASSERT (FALSE);\r
-    gBS->FreePages (Memory, Pages);\r
-    return EFI_OUT_OF_RESOURCES;\r
+    Status = EFI_OUT_OF_RESOURCES;\r
+    goto FreePages;\r
   }\r
 \r
   NewNode->Base       = Memory;\r
@@ -181,6 +195,10 @@ AllocatePagesFromList (
 \r
   *Allocation = NewNode->Allocation;\r
   return EFI_SUCCESS;\r
+\r
+FreePages:\r
+  gBS->FreePages (Memory, Pages);\r
+  return Status;\r
 }\r
 \r
 /**\r
@@ -236,6 +254,16 @@ FreePagesFromList (
  * This function is not responsible to free allocated buffer (eg: case of memory leak,\r
  * runtime allocation).\r
  */\r
+EFI_STATUS\r
+EFIAPI\r
+UncachedMemoryAllocationLibConstructor (\r
+  IN EFI_HANDLE        ImageHandle,\r
+  IN EFI_SYSTEM_TABLE  *SystemTable\r
+  )\r
+{\r
+  return gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&mCpu);\r
+}\r
+\r
 EFI_STATUS\r
 EFIAPI\r
 UncachedMemoryAllocationLibDestructor (\r

diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf
index d7a0f2f792a1e36b159cb3af30404be3e26f73d9..c637430c90201908e42057faa24d6cced75d34a0 100644 (file)
--- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf
+++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf
@@ -22,7 +22,7 @@
   MODULE_TYPE                    = DXE_DRIVER\r
   VERSION_STRING                 = 1.0\r
   LIBRARY_CLASS                  = UncachedMemoryAllocationLib\r
-\r
+  CONSTRUCTOR                    = UncachedMemoryAllocationLibConstructor\r
   DESTRUCTOR                     = UncachedMemoryAllocationLibDestructor\r
 \r
 [Sources.common]\r
@@ -42,3 +42,9 @@
 \r
 [Pcd]\r
   gArmTokenSpaceGuid.PcdArmFreeUncachedMemorySizeThreshold\r
+\r
+[Protocols]\r
+  gEfiCpuArchProtocolGuid\r
+\r
+[Depex]\r
+  gEfiCpuArchProtocolGuid\r