public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD Capabilities With Page Table Attributes
@ 2023-04-26  0:09 Oliver Smith-Denny
  2023-05-01 13:02 ` Ard Biesheuvel
  0 siblings, 1 reply; 26+ messages in thread
From: Oliver Smith-Denny @ 2023-04-26  0:09 UTC (permalink / raw)
  To: devel
  Cc: Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Michael Kubacki,
	Sean Brogan

When ArmPkg's CpuDxe driver initializes, it attempts to sync the
GCD with the page table. However, unlike when the UefiCpuPkg's
CpuDxe initializes, the Arm version does not update the GCD
capabilities with EFI_MEMORY_[RO|RP|XP] (this could also set
the capabilities to be the existing page table attributes for
this range, but the UefiCpuPkg CpuDxe sets the above attributes
as they are software constructs, possible to set for any memory
hardware).

As a result, when the GCD attributes are attempted to be set
against the old GCD capabilities, attributes that are set in the
page table can get lost because the new attributes are not in the
old GCD capabilities (and yet they are already set in the page
table) meaning that the attempted sync between the GCD and the
page table was a failure and drivers querying one vs the other
will see different state. This can lead to RWX memory regions
even with the no-execute policy set, because core drivers (such
as NonDiscoverablePciDeviceIo, to be fixed up in a later patchset)
allocate pages, query the GCD attributes, attempt to set a new
cache attribute and end up clearing the XP bit in the page table
because the GCD attributes do not have XP set.

This patch follows the UefiCpuPkg pattern and adds
EFI_MEMORY_[RO|RP|XP] to the GCD capabilities during CpuDxe
initialization. This ensures that memory regions which already have
these attributes set get them set in the GCD attributes, properly
syncing between the GCD and the page table.

This mitigates the issue seen, however, additional investigations
into setting the GCD attributes earlier and maintaining a better
sync between the GCD and the page table are being done.

Feedback on this proposal is greatly appreciated, particularly
any pitfalls or more architectural solutions to issues seen
with syncing the GCD and the page table.

PR: https://github.com/tianocore/edk2/pull/4311
Personal branch: https://github.com/os-d/edk2/tree/osde/sync_aarch64_gcd_capabilities_v1

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>

Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
---
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 55 +++++++++++++++++---
 1 file changed, 49 insertions(+), 6 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
index 2e73719dce04..3ef0380e084f 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -90,6 +90,7 @@ SetGcdMemorySpaceAttributes (
   UINTN                 EndIndex;
   EFI_PHYSICAL_ADDRESS  RegionStart;
   UINT64                RegionLength;
+  UINT64                Capabilities;
 
   DEBUG ((
     DEBUG_GCD,
@@ -146,14 +147,56 @@ SetGcdMemorySpaceAttributes (
       RegionLength = MemorySpaceMap[Index].BaseAddress + MemorySpaceMap[Index].Length - RegionStart;
     }
 
+    // Always add RO, RP, and XP as all memory is capable of supporting these types (they are software
+    // constructs, not hardware features) and they are critical to maintaining a security boundary
+    Capabilities = MemorySpaceMap[Index].Capabilities | EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP;
+
     //
-    // Set memory attributes according to MTRR attribute and the original attribute of descriptor
+    // Update GCD capabilities as these may have changed in the page table since the GCD was created
+    // this follows the same pattern as x86 GCD and Page Table syncing
     //
-    gDS->SetMemorySpaceAttributes (
-           RegionStart,
-           RegionLength,
-           (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) | (MemorySpaceMap[Index].Capabilities & Attributes)
-           );
+    Status = gDS->SetMemorySpaceCapabilities (
+                    RegionStart,
+                    RegionLength,
+                    Capabilities
+                    );
+
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a - failed to update GCD capabilities: 0x%llx on memory region: 0x%llx length: 0x%llx Status: %r\n",
+        __func__,
+        Capabilities,
+        RegionStart,
+        RegionLength,
+        Status
+        ));
+      ASSERT_EFI_ERROR (Status);
+      continue;
+    }
+
+    //
+    // Set memory attributes according to the page table attribute and the original attribute of descriptor
+    //
+    Status = gDS->SetMemorySpaceAttributes (
+                    RegionStart,
+                    RegionLength,
+                    (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) | (Attributes & Capabilities)
+                    );
+
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a - failed to update GCD attributes: 0x%llx on memory region: 0x%llx length: 0x%llx Status: %r\n",
+        __func__,
+        Attributes,
+        RegionStart,
+        RegionLength,
+        Status
+        ));
+      ASSERT_EFI_ERROR (Status);
+      continue;
+    }
   }
 
   return EFI_SUCCESS;
-- 
2.40.0


^ permalink raw reply related	[flat|nested] 26+ messages in thread
[parent not found: <1759538694580A69.7408@groups.io>]

end of thread, other threads:[~2023-07-07 20:13 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-26  0:09 [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD Capabilities With Page Table Attributes Oliver Smith-Denny
2023-05-01 13:02 ` Ard Biesheuvel
2023-05-01 17:03   ` Oliver Smith-Denny
2023-05-01 17:49     ` [edk2-devel] [PATCH " Michael D Kinney
2023-05-01 17:50       ` Ard Biesheuvel
2023-05-01 17:53         ` Oliver Smith-Denny
2023-05-01 17:59           ` Michael D Kinney
2023-05-09  1:35             ` Ni, Ray
2023-05-09  2:03               ` Oliver Smith-Denny
2023-05-09  2:04                 ` Michael D Kinney
2023-05-09  6:59                   ` Ard Biesheuvel
2023-05-09 14:59                     ` Oliver Smith-Denny
2023-05-10 16:10                       ` Taylor Beebe
2023-05-16  2:53                         ` Ni, Ray
2023-05-16 17:11                           ` Oliver Smith-Denny
2023-05-17  7:14                             ` Ni, Ray
2023-06-02  2:24                               ` Michael Kubacki
2023-06-02  2:42                                 ` Ni, Ray
2023-06-02  3:09                                   ` Michael Kubacki
2023-06-02  9:31                                     ` Ard Biesheuvel
2023-06-06  2:13                                     ` Michael Kubacki
2023-06-06  3:00                                       ` Ni, Ray
     [not found] <1759538694580A69.7408@groups.io>
2023-06-07 16:10 ` [edk2-devel][PATCH " Oliver Smith-Denny
2023-06-07 17:31   ` Ard Biesheuvel
2023-06-07 18:03     ` Oliver Smith-Denny
     [not found]     ` <176672827230FBF7.32008@groups.io>
2023-07-07 20:13       ` Oliver Smith-Denny

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox