[RFC 00/13] Hardware enforced W^X memory protections

[RFC 00/13] Hardware enforced W^X memory protections

[Ard Biesheuvel]

The ARM architecture has an interesting feature in its virtual memory
controls called 'WXN', which puts the MMU in a mode where all mappings
of memory that are writable are implicitly non-executable as well.

While EDK2 implements a couple of memory protection features already, in
some places it still relies fundamentally on mappings that are both
writable and executable at the same time, which is not great from a
robustness and code safety point of view.

This series is a proof-of-concept for ArmVirtQemu that addresses each of
those issues, allowing it to boot into the OS (Linux) successfully with
the WXN control enabled.

The following issues are being addressed:
- the flash region must be mapped read-only explicitly, so that its code
  is executable
- the DXE IPL must not run shadowed so it executes in place from the
  executable mapping of the FV (and PEIM shadowing must be off in
  general)
- the DXE IPL must map the DXE core code region read-only explicitly so
  it can execute
- the DXE core must be equipped with a preliminary version of the
  SetMemoryAttributes member of the CPU arch protocol so that it can
  manipulate memory permissions before the CPU arch protocol driver is
  dispatched
- need to use XP mappings for all DRAM regions out of reset - this is
  to avoid unbounded recursion in the page table handling code, which
  may now be called before the CPU arch protocol driver remaps all
  unused regions with XP attributes
- limit the memory regions that are remapped writable+executable during
  ExitBootServices() to those pages that are actually subject to
  relocation fixups
- ensure that AArch64 runtime DXE driver images do not carry code and
  relocatable quantities in the same 4k page, so that clearing the RO
  bit does not remove its executable permissions

(patch #1 is preparatory cleanup and not relevant to the above)

Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Rebecca Cran <quic_rcran@quicinc.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Taylor Beebe <t@taylorbeebe.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Peter Jones <pjones@redhat.com>
Cc: Kees Cook <keescook@chromium.org>

Ard Biesheuvel (13):
  ArmPkg/Mmu: Remove handling of NONSECURE memory regions
  ArmPkg/ArmMmuLib: Introduce region types for RO/XP WB cached memory
  MdePkg/BasePeCoffLib: Add API to keep track of relocation range
  MdeModulePkg/DxeIpl: Avoid shadowing IPL PEIM by default
  MdeModulePkg/DxeIpl AARCH64: Remap DXE core code section before launch
  MdeModulePkg/DxeCore: Reduce range of W+X remaps at EBS time
  MdeModulePkg/DxeCore: Permit preliminary CPU arch fallback
  ArmPkg: Implement ArmSetMemoryOverrideLib
  ArmVirtPkg/ArmVirtQemu: Use XP memory mappings by default
  ArmVirtPkg/ArmVirtQemu: Use PEI flavor of ArmMmuLib for all PEIMs
  ArmVirtPkg/ArmVirtQemu: Use read-only memory region type for code
    flash
  BaseTools/GccBase AARCH64: Avoid page sharing between code and data
  ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory
    permissions

 ArmPkg/Include/Chipset/ArmV7Mmu.h                                  | 51 +++++-------
 ArmPkg/Include/Library/ArmLib.h                                    | 17 ++--
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c                   | 34 +++++---
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c                       | 49 ++++++------
 ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.c   | 56 +++++++++++++
 ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf | 25 ++++++
 ArmVirtPkg/ArmVirt.dsc.inc                                         |  1 +
 ArmVirtPkg/ArmVirtQemu.dsc                                         | 11 ++-
 ArmVirtPkg/ArmVirtQemuKernel.dsc                                   |  1 +
 ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S  |  2 +-
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c         |  4 +-
 BaseTools/Scripts/GccBase.lds                                      | 13 ++-
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c                      | 33 ++++++--
 MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c                     | 69 ++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf                            |  6 +-
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c                             | 24 +++---
 MdePkg/Include/Library/PeCoffLib.h                                 | 25 ++++++
 MdePkg/Library/BasePeCoffLib/BasePeCoff.c                          | 83 +++++++++++++++++++-
 18 files changed, 392 insertions(+), 112 deletions(-)
 create mode 100644 ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.c
 create mode 100644 ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf

-- 
2.39.1

[RFC 01/13] ArmPkg/Mmu: Remove handling of NONSECURE memory regions

[Ard Biesheuvel]

Non-secure memory is a distinction that only matters when executing code
in the secure world that reasons about the secure vs non-secure address
spaces. EDK2 was not designed for that, and the AArch64 version of the
MMU handling library already treats them as identical, so let's just
drop the ARM memory region types that mark memory as 'non-secure'
explicitly.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Include/Chipset/ArmV7Mmu.h                | 51 +++++++-------------
 ArmPkg/Include/Library/ArmLib.h                  | 11 -----
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  5 --
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c     | 33 +++----------
 4 files changed, 24 insertions(+), 76 deletions(-)

diff --git a/ArmPkg/Include/Chipset/ArmV7Mmu.h b/ArmPkg/Include/Chipset/ArmV7Mmu.h
index da4f3160f8ff..89b81e33d004 100644
--- a/ArmPkg/Include/Chipset/ArmV7Mmu.h
+++ b/ArmPkg/Include/Chipset/ArmV7Mmu.h
@@ -157,39 +157,24 @@
 #define TT_DESCRIPTOR_PAGE_BASE_ADDRESS(a)  ((a) & TT_DESCRIPTOR_PAGE_BASE_ADDRESS_MASK)
 #define TT_DESCRIPTOR_PAGE_BASE_SHIFT  12
 
-#define TT_DESCRIPTOR_SECTION_WRITE_BACK(NonSecure)     (TT_DESCRIPTOR_SECTION_TYPE_SECTION                                                           |     \
-                                                            ((NonSecure) ?  TT_DESCRIPTOR_SECTION_NS : 0)    | \
-                                                            TT_DESCRIPTOR_SECTION_NG_GLOBAL                         | \
-                                                            TT_DESCRIPTOR_SECTION_S_SHARED                          | \
-                                                            TT_DESCRIPTOR_SECTION_DOMAIN(0)                         | \
-                                                            TT_DESCRIPTOR_SECTION_AP_RW_RW                          | \
-                                                            TT_DESCRIPTOR_SECTION_AF                                | \
-                                                            TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC)
-#define TT_DESCRIPTOR_SECTION_WRITE_THROUGH(NonSecure)  (TT_DESCRIPTOR_SECTION_TYPE_SECTION                                                           |     \
-                                                            ((NonSecure) ?  TT_DESCRIPTOR_SECTION_NS : 0)    | \
-                                                            TT_DESCRIPTOR_SECTION_NG_GLOBAL                         | \
-                                                            TT_DESCRIPTOR_SECTION_S_SHARED                          | \
-                                                            TT_DESCRIPTOR_SECTION_DOMAIN(0)                         | \
-                                                            TT_DESCRIPTOR_SECTION_AP_RW_RW                          | \
-                                                            TT_DESCRIPTOR_SECTION_AF                                | \
-                                                            TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC)
-#define TT_DESCRIPTOR_SECTION_DEVICE(NonSecure)         (TT_DESCRIPTOR_SECTION_TYPE_SECTION                                                           |     \
-                                                            ((NonSecure) ?  TT_DESCRIPTOR_SECTION_NS : 0)    | \
-                                                            TT_DESCRIPTOR_SECTION_NG_GLOBAL                         | \
-                                                            TT_DESCRIPTOR_SECTION_S_NOT_SHARED                      | \
-                                                            TT_DESCRIPTOR_SECTION_DOMAIN(0)                         | \
-                                                            TT_DESCRIPTOR_SECTION_AP_RW_RW                          | \
-                                                            TT_DESCRIPTOR_SECTION_XN_MASK                           | \
-                                                            TT_DESCRIPTOR_SECTION_AF                                | \
-                                                            TT_DESCRIPTOR_SECTION_CACHE_POLICY_SHAREABLE_DEVICE)
-#define TT_DESCRIPTOR_SECTION_UNCACHED(NonSecure)       (TT_DESCRIPTOR_SECTION_TYPE_SECTION                                                           |    \
-                                                           ((NonSecure) ?  TT_DESCRIPTOR_SECTION_NS : 0)    | \
-                                                           TT_DESCRIPTOR_SECTION_NG_GLOBAL                         | \
-                                                           TT_DESCRIPTOR_SECTION_S_NOT_SHARED                      | \
-                                                           TT_DESCRIPTOR_SECTION_DOMAIN(0)                         | \
-                                                           TT_DESCRIPTOR_SECTION_AP_RW_RW                          | \
-                                                            TT_DESCRIPTOR_SECTION_AF                                | \
-                                                           TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE)
+#define TT_DESCRIPTOR_SECTION_DEFAULT  (TT_DESCRIPTOR_SECTION_TYPE_SECTION | \
+                                        TT_DESCRIPTOR_SECTION_NG_GLOBAL    | \
+                                        TT_DESCRIPTOR_SECTION_S_SHARED     | \
+                                        TT_DESCRIPTOR_SECTION_DOMAIN(0)    | \
+                                        TT_DESCRIPTOR_SECTION_AP_RW_RW     | \
+                                        TT_DESCRIPTOR_SECTION_AF)
+
+#define TT_DESCRIPTOR_SECTION_WRITE_BACK  (TT_DESCRIPTOR_SECTION_DEFAULT | \
+                                           TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC)
+
+#define TT_DESCRIPTOR_SECTION_WRITE_THROUGH  (TT_DESCRIPTOR_SECTION_DEFAULT | \
+                                              TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC)
+
+#define TT_DESCRIPTOR_SECTION_DEVICE  (TT_DESCRIPTOR_SECTION_DEFAULT | \
+                                       TT_DESCRIPTOR_SECTION_CACHE_POLICY_SHAREABLE_DEVICE)
+
+#define TT_DESCRIPTOR_SECTION_UNCACHED  (TT_DESCRIPTOR_SECTION_DEFAULT | \
+                                         TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE)
 
 #define TT_DESCRIPTOR_PAGE_WRITE_BACK     (TT_DESCRIPTOR_PAGE_TYPE_PAGE                                                           |          \
                                                         TT_DESCRIPTOR_PAGE_NG_GLOBAL                                                      | \
diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index fa605f128bfd..a53f60d98852 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -25,29 +25,18 @@
                                      EFI_MEMORY_WT | EFI_MEMORY_WB | \
                                      EFI_MEMORY_UCE)
 
-/**
- * The UEFI firmware must not use the ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_* attributes.
- *
- * The Non Secure memory attribute (ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_*) should only
- * be used in Secure World to distinguished Secure to Non-Secure memory.
- */
 typedef enum {
   ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED = 0,
-  ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_UNCACHED_UNBUFFERED,
   ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK,
-  ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK,
 
   // On some platforms, memory mapped flash region is designed as not supporting
   // shareable attribute, so WRITE_BACK_NONSHAREABLE is added for such special
   // need.
   // Do NOT use below two attributes if you are not sure.
   ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE,
-  ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK_NONSHAREABLE,
 
   ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH,
-  ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH,
   ARM_MEMORY_REGION_ATTRIBUTE_DEVICE,
-  ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_DEVICE
 } ARM_MEMORY_REGION_ATTRIBUTES;
 
 #define IS_ARM_MEMORY_REGION_ATTRIBUTES_SECURE(attr)  ((UINT32)(attr) & 1)
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 1ce200c43c72..ee4c5c995ce8 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -39,26 +39,21 @@ ArmMemoryAttributeToPageAttribute (
 {
   switch (Attributes) {
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE:
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK_NONSHAREABLE:
       return TT_ATTR_INDX_MEMORY_WRITE_BACK;
 
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK:
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK:
       return TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE;
 
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH:
       return TT_ATTR_INDX_MEMORY_WRITE_THROUGH | TT_SH_INNER_SHAREABLE;
 
     // Uncached and device mappings are treated as outer shareable by default,
     case ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED:
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_UNCACHED_UNBUFFERED:
       return TT_ATTR_INDX_MEMORY_NON_CACHEABLE;
 
     default:
       ASSERT (0);
     case ARM_MEMORY_REGION_ATTRIBUTE_DEVICE:
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_DEVICE:
       if (ArmReadCurrentEL () == AARCH64_EL2) {
         return TT_ATTR_INDX_DEVICE_MEMORY | TT_XN_MASK;
       } else {
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index 28cc9b2fe058..154298357460 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -100,24 +100,19 @@ PopulateLevel2PageTable (
 
   switch (Attributes) {
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK:
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK:
       PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_BACK;
       break;
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE:
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK_NONSHAREABLE:
       PageAttributes  = TT_DESCRIPTOR_PAGE_WRITE_BACK;
       PageAttributes &= ~TT_DESCRIPTOR_PAGE_S_SHARED;
       break;
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH:
       PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_THROUGH;
       break;
     case ARM_MEMORY_REGION_ATTRIBUTE_DEVICE:
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_DEVICE:
       PageAttributes = TT_DESCRIPTOR_PAGE_DEVICE;
       break;
     case ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED:
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_UNCACHED_UNBUFFERED:
       PageAttributes = TT_DESCRIPTOR_PAGE_UNCACHED;
       break;
     default:
@@ -239,39 +234,23 @@ FillTranslationTable (
 
   switch (MemoryRegion->Attributes) {
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK:
-      Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK (0);
+      Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK;
       break;
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE:
-      Attributes  = TT_DESCRIPTOR_SECTION_WRITE_BACK (0);
+      Attributes  = TT_DESCRIPTOR_SECTION_WRITE_BACK;
       Attributes &= ~TT_DESCRIPTOR_SECTION_S_SHARED;
       break;
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
-      Attributes = TT_DESCRIPTOR_SECTION_WRITE_THROUGH (0);
+      Attributes = TT_DESCRIPTOR_SECTION_WRITE_THROUGH;
       break;
     case ARM_MEMORY_REGION_ATTRIBUTE_DEVICE:
-      Attributes = TT_DESCRIPTOR_SECTION_DEVICE (0);
+      Attributes = TT_DESCRIPTOR_SECTION_DEVICE;
       break;
     case ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED:
-      Attributes = TT_DESCRIPTOR_SECTION_UNCACHED (0);
-      break;
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK:
-      Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK (1);
-      break;
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK_NONSHAREABLE:
-      Attributes  = TT_DESCRIPTOR_SECTION_WRITE_BACK (1);
-      Attributes &= ~TT_DESCRIPTOR_SECTION_S_SHARED;
-      break;
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH:
-      Attributes = TT_DESCRIPTOR_SECTION_WRITE_THROUGH (1);
-      break;
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_DEVICE:
-      Attributes = TT_DESCRIPTOR_SECTION_DEVICE (1);
-      break;
-    case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_UNCACHED_UNBUFFERED:
-      Attributes = TT_DESCRIPTOR_SECTION_UNCACHED (1);
+      Attributes = TT_DESCRIPTOR_SECTION_UNCACHED;
       break;
     default:
-      Attributes = TT_DESCRIPTOR_SECTION_UNCACHED (0);
+      Attributes = TT_DESCRIPTOR_SECTION_UNCACHED;
       break;
   }
 
-- 
2.39.1

[RFC 02/13] ArmPkg/ArmMmuLib: Introduce region types for RO/XP WB cached memory

[Ard Biesheuvel]

To prepare for the enablement of booting EFI with the SCTLR.WXN control
enabled, which makes all writeable memory regions non-executable by
default, introduce a memory type that we will use to describe the flash
region that carries the SEC and PEIM modules that execute in place. Even
if these are implicitly read-only due to the ROM nature, they need to be
mapped with read-only attributes in the page tables to be able to
execute from them.

Also add the XP counterpart which will be used for all normal DRAM right
at the outset.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Include/Library/ArmLib.h                  |  6 ++++
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 29 ++++++++++++++++----
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c     | 16 +++++++++++
 3 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index a53f60d98852..fb1ae57b3522 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -35,6 +35,12 @@ typedef enum {
   // Do NOT use below two attributes if you are not sure.
   ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE,
 
+  // Special region types for memory that must be mapped with read-only or
+  // non-execute permissions from the very start, e.g., to support the use
+  // of the WXN virtual memory control.
+  ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO,
+  ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_XP,
+
   ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH,
   ARM_MEMORY_REGION_ATTRIBUTE_DEVICE,
 } ARM_MEMORY_REGION_ATTRIBUTES;
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index ee4c5c995ce8..9cdaa8b32c62 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -37,12 +37,33 @@ ArmMemoryAttributeToPageAttribute (
   IN ARM_MEMORY_REGION_ATTRIBUTES  Attributes
   )
 {
+  UINT64  Permissions = 0;
+
+  switch (Attributes) {
+    case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO:
+      Permissions = TT_AP_NO_RO;
+      break;
+
+    case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_XP:
+    case ARM_MEMORY_REGION_ATTRIBUTE_DEVICE:
+      if (ArmReadCurrentEL () == AARCH64_EL2) {
+        Permissions = TT_XN_MASK;
+      } else {
+        Permissions = TT_UXN_MASK | TT_PXN_MASK;
+      }
+      break;
+    default:
+      break;
+  }
+
   switch (Attributes) {
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE:
       return TT_ATTR_INDX_MEMORY_WRITE_BACK;
 
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK:
-      return TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE;
+    case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO:
+    case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_XP:
+      return TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE | Permissions;
 
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
       return TT_ATTR_INDX_MEMORY_WRITE_THROUGH | TT_SH_INNER_SHAREABLE;
@@ -54,11 +75,7 @@ ArmMemoryAttributeToPageAttribute (
     default:
       ASSERT (0);
     case ARM_MEMORY_REGION_ATTRIBUTE_DEVICE:
-      if (ArmReadCurrentEL () == AARCH64_EL2) {
-        return TT_ATTR_INDX_DEVICE_MEMORY | TT_XN_MASK;
-      } else {
-        return TT_ATTR_INDX_DEVICE_MEMORY | TT_UXN_MASK | TT_PXN_MASK;
-      }
+      return TT_ATTR_INDX_DEVICE_MEMORY | Permissions;
   }
 }
 
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index 154298357460..00c5f42cd91a 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -106,6 +106,14 @@ PopulateLevel2PageTable (
       PageAttributes  = TT_DESCRIPTOR_PAGE_WRITE_BACK;
       PageAttributes &= ~TT_DESCRIPTOR_PAGE_S_SHARED;
       break;
+    case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO:
+      PageAttributes  = TT_DESCRIPTOR_PAGE_WRITE_BACK;
+      PageAttributes |= TT_DESCRIPTOR_PAGE_AP_NO_RO;
+      break;
+    case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_XP:
+      PageAttributes  = TT_DESCRIPTOR_PAGE_WRITE_BACK;
+      PageAttributes |= TT_DESCRIPTOR_PAGE_XN_MASK;
+      break;
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
       PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_THROUGH;
       break;
@@ -240,6 +248,14 @@ FillTranslationTable (
       Attributes  = TT_DESCRIPTOR_SECTION_WRITE_BACK;
       Attributes &= ~TT_DESCRIPTOR_SECTION_S_SHARED;
       break;
+    case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO:
+      Attributes  = TT_DESCRIPTOR_SECTION_WRITE_BACK;
+      Attributes |= TT_DESCRIPTOR_SECTION_AP_NO_RO;
+      break;
+    case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_XP:
+      Attributes  = TT_DESCRIPTOR_SECTION_WRITE_BACK;
+      Attributes |= TT_DESCRIPTOR_SECTION_XN_MASK;
+      break;
     case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
       Attributes = TT_DESCRIPTOR_SECTION_WRITE_THROUGH;
       break;
-- 
2.39.1

[RFC 03/13] MdePkg/BasePeCoffLib: Add API to keep track of relocation range

[Ard Biesheuvel]

Add a library call to obtain the start and end of the region covered by
relocation fixups. This will be used in a future patch to limit the
range of memory that needs to be remapped with read-write-execute
permissions at ExitBootServices() time.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdePkg/Include/Library/PeCoffLib.h        | 25 ++++++
 MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 83 +++++++++++++++++++-
 2 files changed, 107 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Library/PeCoffLib.h b/MdePkg/Include/Library/PeCoffLib.h
index b45879453785..3706c8a4e858 100644
--- a/MdePkg/Include/Library/PeCoffLib.h
+++ b/MdePkg/Include/Library/PeCoffLib.h
@@ -382,4 +382,29 @@ PeCoffLoaderUnloadImage (
   IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
   );
 
+/**
+  Retrieve the range subject to relocation fixups from the recorded fixup data
+  of a runtime image
+
+  @param       ImageBase           The base address of a PE/COFF image that has been loaded
+                                   and relocated into system memory.
+  @param       VirtImageBase       The request virtual address that the PE/COFF image is to
+                                   be fixed up for.
+  @param       ImageSize           The size, in bytes, of the PE/COFF image.
+  @param       RelocationData      A pointer to the relocation data that was collected when the
+                                   PE/COFF image was relocated using PeCoffLoaderRelocateImage().
+  @param[out]  RelocationRangeMin  The start of the relocated range.
+  @param[out]  RelocationRangeMax  The end of the relocated range.
+
+**/
+VOID
+EFIAPI
+PeCoffLoaderGetRelocationRange (
+  IN  PHYSICAL_ADDRESS  ImageBase,
+  IN  UINTN             ImageSize,
+  IN  VOID              *RelocationData,
+  OUT PHYSICAL_ADDRESS  *RelocationRangeMin,
+  OUT PHYSICAL_ADDRESS  *RelocationRangeMax
+  );
+
 #endif
diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 97a8aaf8c73d..10f3d04d2490 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -936,6 +936,8 @@ PeCoffLoaderRelocateImage (
   PHYSICAL_ADDRESS                     BaseAddress;
   UINT32                               NumberOfRvaAndSizes;
   UINT32                               TeStrippedOffset;
+  PHYSICAL_ADDRESS                     *RelocRangeStart;
+  PHYSICAL_ADDRESS                     *RelocRangeEnd;
 
   ASSERT (ImageContext != NULL);
 
@@ -1043,6 +1045,21 @@ PeCoffLoaderRelocateImage (
     // Run the relocation information and apply the fixups
     //
     FixupData = ImageContext->FixupData;
+    if (FixupData != NULL) {
+      FixupData = ALIGN_POINTER (FixupData, sizeof (PHYSICAL_ADDRESS));
+
+      //
+      // Use the first two UINT64s in the fixup data to keep track of the start
+      // and end of the region that is subject to relocation fixups.
+      //
+      RelocRangeStart = (PHYSICAL_ADDRESS *)FixupData;
+      RelocRangeEnd   = RelocRangeStart + 1;
+      FixupData      += 2 * sizeof (PHYSICAL_ADDRESS);
+
+      *RelocRangeStart = MAX_UINT64;
+      *RelocRangeEnd   = 0;
+    }
+
     while ((UINTN)RelocBase < (UINTN)RelocBaseEnd) {
       Reloc = (UINT16 *)((CHAR8 *)RelocBase + sizeof (EFI_IMAGE_BASE_RELOCATION));
       //
@@ -1070,6 +1087,14 @@ PeCoffLoaderRelocateImage (
         return RETURN_LOAD_ERROR;
       }
 
+      //
+      // Capture this page in the recorded relocation range
+      //
+      if (FixupData != NULL) {
+        *RelocRangeStart = MIN (*RelocRangeStart, (UINTN)FixupBase);
+        *RelocRangeEnd   = MAX (*RelocRangeEnd, (UINTN)FixupBase + SIZE_4KB);
+      }
+
       //
       // Run this relocation record
       //
@@ -1470,6 +1495,9 @@ PeCoffLoaderLoadImage (
   //
   ImageContext->FixupData = NULL;
 
+  // Add two UINT64s at the start to carry the min/max of the relocated range
+  ImageContext->FixupDataSize += 2 * sizeof (PHYSICAL_ADDRESS);
+
   //
   // Load the Codeview information if present
   //
@@ -1824,7 +1852,8 @@ PeCoffLoaderRelocateImageForRuntime (
     // by code will not be fixed up, since that would set them back to
     // defaults.
     //
-    FixupData     = RelocationData;
+    FixupData     = ALIGN_POINTER (RelocationData, sizeof (PHYSICAL_ADDRESS));
+    FixupData    += 2 * sizeof (PHYSICAL_ADDRESS);
     RelocBaseOrig = RelocBase;
     while ((UINTN)RelocBase < (UINTN)RelocBaseEnd) {
       //
@@ -1994,3 +2023,55 @@ PeCoffLoaderUnloadImage (
   PeCoffLoaderUnloadImageExtraAction (ImageContext);
   return RETURN_SUCCESS;
 }
+
+/**
+  Retrieve the range subject to relocation fixups from the recorded fixup data
+  of a runtime image
+
+  @param       ImageBase           The base address of a PE/COFF image that has been loaded
+                                   and relocated into system memory.
+  @param       VirtImageBase       The request virtual address that the PE/COFF image is to
+                                   be fixed up for.
+  @param       ImageSize           The size, in bytes, of the PE/COFF image.
+  @param       RelocationData      A pointer to the relocation data that was collected when the
+                                   PE/COFF image was relocated using PeCoffLoaderRelocateImage().
+  @param[out]  RelocationRangeMin  The start of the relocated range.
+  @param[out]  RelocationRangeMax  The end of the relocated range.
+
+**/
+VOID
+EFIAPI
+PeCoffLoaderGetRelocationRange (
+  IN  PHYSICAL_ADDRESS  ImageBase,
+  IN  UINTN             ImageSize,
+  IN  VOID              *RelocationData,
+  OUT PHYSICAL_ADDRESS  *RelocationRangeMin,
+  OUT PHYSICAL_ADDRESS  *RelocationRangeMax
+  )
+{
+  PHYSICAL_ADDRESS  *FixupData;
+
+  if ((RelocationData == NULL) || (ImageBase == 0x0)) {
+    return;
+  }
+
+  FixupData = ALIGN_POINTER (RelocationData, sizeof (PHYSICAL_ADDRESS));
+
+  if ((FixupData[0] == MAX_UINT64) && (FixupData[1] == 0)) {
+    // No fixups recorded
+    *RelocationRangeMin = ImageBase;
+    *RelocationRangeMax = ImageBase;
+    return;
+  }
+
+  if ((FixupData[0] < ImageBase) ||
+      (FixupData[1] > (ImageBase + ImageSize))) {
+    ASSERT (FALSE);
+    *RelocationRangeMin = ImageBase;
+    *RelocationRangeMax = ImageBase + ImageSize;
+    return;
+  }
+
+  *RelocationRangeMin = FixupData[0];
+  *RelocationRangeMax = FixupData[1];
+}
-- 
2.39.1

[RFC 04/13] MdeModulePkg/DxeIpl: Avoid shadowing IPL PEIM by default

[Ard Biesheuvel]

Currently, the DXE IPL relies on permanent memory being available, but
does not DEPEX on the associated PPI. Instead, it registers for PEIM
shadowing, and only proceeds when running shadowed, and this implies
that permanent memory has been installed.

While PEIM shadowing is typically good for performance, there are
reasons why we might prefer to avoid it, e.g., when running under
virtualization in a mode where the write protection of the ROM is an
advantage from a safety PoV, and where the performance is identical.

This is especially true when code executing from ordinary RAM needs some
additional work to be executable, like when enabling WXN on ARM, which
only permits execution from memory that is mapped read-only.

So permit DXE IPL to run unshadowed, based on the existing PCD that
decides whether or not shadowing is preferred. While making this
behavior depend on this PCD is strictly redundant (as the IPL PEIM will
be shadowed anyway, even if RegisterForShadow() is not called), let's
test it anyway to avoid modifying the behavior on existing platforms.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf |  5 +++-
 MdeModulePkg/Core/DxeIplPeim/DxeLoad.c  | 24 +++++++++++---------
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 052ea0ec1a6f..62821477d012 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -112,6 +112,9 @@ [FeaturePcd.X64]
 [FeaturePcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress ## CONSUMES
 
+[Pcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot            ## CONSUMES
+
 [Pcd.IA32,Pcd.X64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable                      ## SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask    ## CONSUMES
@@ -128,7 +131,7 @@ [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
   gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy       ## SOMETIMES_CONSUMES
 
 [Depex]
-  gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
+  gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid AND gEfiPeiMemoryDiscoveredPpiGuid
 
 #
 # [BootMode]
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
index 2c19f1a507ba..228d39a618d3 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
@@ -77,18 +77,20 @@ PeimInitializeDxeIpl (
   BootMode = GetBootModeHob ();
 
   if (BootMode != BOOT_ON_S3_RESUME) {
-    Status = PeiServicesRegisterForShadow (FileHandle);
-    if (Status == EFI_SUCCESS) {
-      //
-      // EFI_SUCESS means it is the first time to call register for shadow.
-      //
-      return Status;
-    }
+    if (PcdGetBool (PcdShadowPeimOnBoot)) {
+      Status = PeiServicesRegisterForShadow (FileHandle);
+      if (Status == EFI_SUCCESS) {
+        //
+        // EFI_SUCESS means it is the first time to call register for shadow.
+        //
+        return Status;
+      }
 
-    //
-    // Ensure that DXE IPL is shadowed to permanent memory.
-    //
-    ASSERT (Status == EFI_ALREADY_STARTED);
+      //
+      // Ensure that DXE IPL is shadowed to permanent memory.
+      //
+      ASSERT (Status == EFI_ALREADY_STARTED);
+    }
 
     //
     // DXE core load requires permanent memory.
-- 
2.39.1

[RFC 05/13] MdeModulePkg/DxeIpl AARCH64: Remap DXE core code section before launch

[Ard Biesheuvel]

To permit the platform to adopt a stricter policy when it comes to
memory protections, and map all memory XP by default, add the necessary
handling to the DXE IPL PEIM to ensure that the DXE core code section is
mapped executable before invoking the DXE core.

It is up to the DXE core itself to manage the executable permissions on
other DXE and UEFI drivers and applications that it dispatches.

Note that this requires that the DXE IPL executes non-shadowed from a FV
that is mapped executable.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c | 69 ++++++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf        |  1 +
 2 files changed, 70 insertions(+)

diff --git a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
index f62b6dcb38a7..21eac2851554 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
@@ -11,6 +11,69 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include "DxeIpl.h"
 
 #include <Library/ArmMmuLib.h>
+#include <Library/PeCoffLib.h>
+
+STATIC
+VOID
+RemapDxeCoreCodeReadOnly (
+  IN EFI_PHYSICAL_ADDRESS  DxeCoreEntryPoint,
+  IN EFI_PEI_HOB_POINTERS  HobList
+  )
+{
+  EFI_PEI_HOB_POINTERS                  Hob;
+  EFI_HOB_MEMORY_ALLOCATION             *ModuleHob;
+  PE_COFF_LOADER_IMAGE_CONTEXT          ImageContext;
+  RETURN_STATUS                         Status;
+  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION   Hdr;
+  EFI_IMAGE_SECTION_HEADER              *Section;
+  UINTN                                 Index;
+
+  ImageContext.ImageRead = PeCoffLoaderImageReadFromMemory;
+  ImageContext.Handle    = NULL;
+
+  //
+  // Find the module HOB for the DXE core
+  //
+  for (Hob.Raw = HobList.Raw; !END_OF_HOB_LIST (Hob); Hob.Raw = GET_NEXT_HOB (Hob)) {
+    if (GET_HOB_TYPE (Hob) == EFI_HOB_TYPE_MEMORY_ALLOCATION) {
+      ModuleHob = Hob.MemoryAllocation;
+      if ((ModuleHob->AllocDescriptor.MemoryBaseAddress <= DxeCoreEntryPoint &&
+          ((ModuleHob->AllocDescriptor.MemoryBaseAddress + ModuleHob->AllocDescriptor.MemoryLength) > DxeCoreEntryPoint)))
+      {
+        ImageContext.Handle = (VOID *)(UINTN)ModuleHob->AllocDescriptor.MemoryBaseAddress;
+        break;
+      }
+    }
+  }
+
+  ASSERT (ImageContext.Handle != NULL);
+
+  Status = PeCoffLoaderGetImageInfo (&ImageContext);
+  ASSERT_RETURN_ERROR (Status);
+
+  Hdr.Union = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)((UINT8 *)ImageContext.Handle +
+                                                  ImageContext.PeCoffHeaderOffset);
+  ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
+
+  Section = (EFI_IMAGE_SECTION_HEADER *)((UINT8 *)Hdr.Union + sizeof (UINT32) +
+                                         sizeof (EFI_IMAGE_FILE_HEADER) +
+                                         Hdr.Pe32->FileHeader.SizeOfOptionalHeader
+                                         );
+
+  for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; Index++) {
+    if ((Section[Index].Characteristics & EFI_IMAGE_SCN_CNT_CODE) != 0) {
+      ArmSetMemoryRegionReadOnly (
+          (UINTN)((UINT8 *)ImageContext.Handle + Section[Index].VirtualAddress),
+          Section[Index].Misc.VirtualSize
+          );
+
+      ArmClearMemoryRegionNoExec (
+          (UINTN)((UINT8 *)ImageContext.Handle + Section[Index].VirtualAddress),
+          Section[Index].Misc.VirtualSize
+          );
+    }
+  }
+}
 
 /**
    Transfers control to DxeCore.
@@ -33,6 +96,12 @@ HandOffToDxeCore (
   VOID        *TopOfStack;
   EFI_STATUS  Status;
 
+  //
+  // DRAM may be mapped with non-executable permissions by default, so
+  // we'll need to map the DXE core code region executable explicitly.
+  //
+  RemapDxeCoreCodeReadOnly (DxeCoreEntryPoint, HobList);
+
   //
   // Allocate 128KB for the Stack
   //
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 62821477d012..d85ca79dc0c3 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -82,6 +82,7 @@ [LibraryClasses]
 
 [LibraryClasses.ARM, LibraryClasses.AARCH64]
   ArmMmuLib
+  PeCoffLib
 
 [Ppis]
   gEfiDxeIplPpiGuid                      ## PRODUCES
-- 
2.39.1

[RFC 06/13] MdeModulePkg/DxeCore: Reduce range of W+X remaps at EBS time

[Ard Biesheuvel]

Instead of remapping all DXE runtime drivers with read-write-execute
permissions entirely when ExitBootServices() is called, remap only the
parts of those images that require writable access for applying
relocation fixups at SetVirtualAddressMap() time.

As illustrated below, this greatly reduces the footprint of those
regions, which is important for safe execution. And given that the most
important ISAs and toolchains split executable code from relocatable
quantities, the remapped pages in question are generally not the ones
that contain code as well.

On a ArmVirtQemu build, the footprint of those RWX pages is shown below.

As future work, we might investigate whether we can find a way to
guarantee in general that images are built in a way where executable
code and relocatable data never share a 4 KiB page, in which case we
could apply EFI_MEMORY_XP permissions here instead of allowing RWX.

Before:

  SetUefiImageMemoryAttributes - 0x0000000047600000 - 0x0000000000050000 (0x0000000000000008)
  SetUefiImageMemoryAttributes - 0x0000000044290000 - 0x0000000000050000 (0x0000000000000008)
  SetUefiImageMemoryAttributes - 0x0000000044230000 - 0x0000000000050000 (0x0000000000000008)
  SetUefiImageMemoryAttributes - 0x00000000441D0000 - 0x0000000000050000 (0x0000000000000008)
  SetUefiImageMemoryAttributes - 0x00000000440D0000 - 0x0000000000050000 (0x0000000000000008)
  SetUefiImageMemoryAttributes - 0x0000000043F90000 - 0x0000000000040000 (0x0000000000000008)
  SetUefiImageMemoryAttributes - 0x0000000043F40000 - 0x0000000000040000 (0x0000000000000008)
  SetUefiImageMemoryAttributes - 0x0000000043EF0000 - 0x0000000000040000 (0x0000000000000008)

After:

  SetUefiImageMemoryAttributes - 0x0000000047630000 - 0x0000000000001000 (0x0000000000000008)
  SetUefiImageMemoryAttributes - 0x00000000442C0000 - 0x0000000000001000 (0x0000000000000008)
  SetUefiImageMemoryAttributes - 0x0000000044260000 - 0x0000000000001000 (0x0000000000000008)
  SetUefiImageMemoryAttributes - 0x0000000044200000 - 0x0000000000001000 (0x0000000000000008)
  SetUefiImageMemoryAttributes - 0x0000000044100000 - 0x0000000000001000 (0x0000000000000008)

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 5a82eee80781..854651556de4 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -1060,6 +1060,8 @@ MemoryProtectionExitBootServicesCallback (
 {
   EFI_RUNTIME_IMAGE_ENTRY  *RuntimeImage;
   LIST_ENTRY               *Link;
+  PHYSICAL_ADDRESS         RelocationRangeStart;
+  PHYSICAL_ADDRESS         RelocationRangeEnd;
 
   //
   // We need remove the RT protection, because RT relocation need write code segment
@@ -1073,7 +1075,22 @@ MemoryProtectionExitBootServicesCallback (
   if (mImageProtectionPolicy != 0) {
     for (Link = gRuntime->ImageHead.ForwardLink; Link != &gRuntime->ImageHead; Link = Link->ForwardLink) {
       RuntimeImage = BASE_CR (Link, EFI_RUNTIME_IMAGE_ENTRY, Link);
-      SetUefiImageMemoryAttributes ((UINT64)(UINTN)RuntimeImage->ImageBase, ALIGN_VALUE (RuntimeImage->ImageSize, EFI_PAGE_SIZE), 0);
+
+      PeCoffLoaderGetRelocationRange (
+        (PHYSICAL_ADDRESS)(UINTN)RuntimeImage->ImageBase,
+        ALIGN_VALUE (RuntimeImage->ImageSize, EFI_PAGE_SIZE),
+        RuntimeImage->RelocationData,
+        &RelocationRangeStart,
+        &RelocationRangeEnd
+        );
+
+      if (RelocationRangeEnd > RelocationRangeStart) {
+        SetUefiImageMemoryAttributes (
+          RelocationRangeStart,
+          (UINTN)(RelocationRangeEnd - RelocationRangeStart),
+          0
+          );
+      }
     }
   }
 }
-- 
2.39.1

[RFC 07/13] MdeModulePkg/DxeCore: Permit preliminary CPU arch fallback

[Ard Biesheuvel]

Store the address of the SetMemoryAttributes() member of the CPU arch
protocol in a global variable, and invoke it via this variable. This
by itself should have not result in functional changes, but it permits
platforms to provide an preliminary implementation of this member at
link time, allowing the DXE core to enforce strict memory permissions
even before dispatching the CPU arch protocol driver itself.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 854651556de4..c29985ad3116 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -66,6 +66,8 @@ extern LIST_ENTRY  mGcdMemorySpaceMap;
 
 STATIC LIST_ENTRY  mProtectedImageRecordList;
 
+EFI_CPU_SET_MEMORY_ATTRIBUTES  gCpuSetMemoryAttributes;
+
 /**
   Sort code section in image record, based upon CodeSegmentBase from low to high.
 
@@ -224,8 +226,8 @@ SetUefiImageMemoryAttributes (
 
   DEBUG ((DEBUG_INFO, "SetUefiImageMemoryAttributes - 0x%016lx - 0x%016lx (0x%016lx)\n", BaseAddress, Length, FinalAttributes));
 
-  ASSERT (gCpu != NULL);
-  gCpu->SetMemoryAttributes (gCpu, BaseAddress, Length, FinalAttributes);
+  ASSERT (gCpuSetMemoryAttributes != NULL);
+  gCpuSetMemoryAttributes (gCpu, BaseAddress, Length, FinalAttributes);
 }
 
 /**
@@ -408,7 +410,7 @@ ProtectUefiImage (
   DEBUG ((DEBUG_INFO, "ProtectUefiImageCommon - 0x%x\n", LoadedImage));
   DEBUG ((DEBUG_INFO, "  - 0x%016lx - 0x%016lx\n", (EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->ImageBase, LoadedImage->ImageSize));
 
-  if (gCpu == NULL) {
+  if (gCpuSetMemoryAttributes == NULL) {
     return;
   }
 
@@ -995,6 +997,8 @@ MemoryProtectionCpuArchProtocolNotify (
     goto Done;
   }
 
+  gCpuSetMemoryAttributes = gCpu->SetMemoryAttributes;
+
   //
   // Apply the memory protection policy on non-BScode/RTcode regions.
   //
@@ -1278,7 +1282,7 @@ ApplyMemoryProtectionPolicy (
   // permission attributes, and it is the job of the driver that installs this
   // protocol to set the permissions on existing allocations.
   //
-  if (gCpu == NULL) {
+  if (gCpuSetMemoryAttributes == NULL) {
     return EFI_SUCCESS;
   }
 
@@ -1318,5 +1322,5 @@ ApplyMemoryProtectionPolicy (
   //
   NewAttributes = GetPermissionAttributeForMemoryType (NewType);
 
-  return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
+  return gCpuSetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
 }
-- 
2.39.1

[RFC 08/13] ArmPkg: Implement ArmSetMemoryOverrideLib

[Ard Biesheuvel]

Implement the ARM version of a NULL class library that can be overlaid
on top of the DXE core to equip it right from its launch with an
implementation of the CPU arch protocol member that sets type and
permission attributes on memory regions.

This bridges the gap between dispatch of DXE core and dispatch of the
DXE driver that implements the CPU arch protocol, removing the need to
rely on memory mappings that are writable and executable at the same
time.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.c   | 56 ++++++++++++++++++++
 ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf | 25 +++++++++
 2 files changed, 81 insertions(+)

diff --git a/ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.c b/ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.c
new file mode 100644
index 000000000000..d2a9bc96be35
--- /dev/null
+++ b/ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.c
@@ -0,0 +1,56 @@
+/** @file
+  Copyright (c) 2023, Google LLC. All rights reserved.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <PiDxe.h>
+
+#include <Library/ArmMmuLib.h>
+#include <Library/DebugLib.h>
+#include <Protocol/Cpu.h>
+
+extern EFI_CPU_SET_MEMORY_ATTRIBUTES  gCpuSetMemoryAttributes;
+
+STATIC UINTN  mRecursionLevel;
+
+STATIC
+EFI_STATUS
+EFIAPI
+EarlyArmSetMemoryAttributes (
+  IN  EFI_CPU_ARCH_PROTOCOL  *This,
+  IN  EFI_PHYSICAL_ADDRESS   BaseAddress,
+  IN  UINT64                 Length,
+  IN  UINT64                 Attributes
+  )
+{
+  EFI_STATUS  Status;
+
+  // There are cases where the use of strict memory permissions may trigger
+  // unbounded recursion in the page table code. This happens when setting
+  // memory permissions results in a page table split and therefore a page
+  // allocation, which could trigger a recursive invocation of this function.
+  ASSERT (mRecursionLevel < 2);
+
+  mRecursionLevel++;
+
+  Status = ArmSetMemoryAttributes (
+             BaseAddress,
+             Length,
+             Attributes
+             );
+
+  mRecursionLevel--;
+  return Status;
+}
+
+RETURN_STATUS
+EFIAPI
+ArmSetMemoryOverrideLibConstructor (
+  VOID
+  )
+{
+  gCpuSetMemoryAttributes = EarlyArmSetMemoryAttributes;
+
+  return RETURN_SUCCESS;
+}
diff --git a/ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf b/ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf
new file mode 100644
index 000000000000..f07da3dd2d15
--- /dev/null
+++ b/ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf
@@ -0,0 +1,25 @@
+#/** @file
+#  Copyright (c) 2023, Google LLC. All rights reserved.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#**/
+
+[Defines]
+  INF_VERSION                    = 1.29
+  BASE_NAME                      = ArmSetMemoryOverrideLib
+  FILE_GUID                      = 849a43c0-6ad9-428e-8a5a-e090f7853bd3
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = NULL|DXE_CORE
+  CONSTRUCTOR                    = ArmSetMemoryOverrideLibConstructor
+
+[Sources.common]
+  ArmSetMemoryOverrideLib.c
+
+[Packages]
+  ArmPkg/ArmPkg.dec
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  ArmMmuLib
+  DebugLib
-- 
2.39.1

[RFC 09/13] ArmVirtPkg/ArmVirtQemu: Use XP memory mappings by default

[Ard Biesheuvel]

Now that all the plumbing is in place, we can switch to a default policy
of XP for all memory mappings straight out of reset. This reduces the
risk of running with memory ranges mapped as both writable and
executable at the same time.

Note this this requires the overlay library to be added to the DXE core,
as otherwise, it will not be able to dispatch the CPU arch protocol DXE
driver (or any other DXE driver for that matter), as it would lack the
ability to grant executable permissions to those executables.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc                                 | 1 +
 ArmVirtPkg/ArmVirtQemuKernel.dsc                           | 1 +
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 0f1c6395488a..dd4c84ae6eb9 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -372,6 +372,7 @@ [Components.common]
   #
   MdeModulePkg/Core/Dxe/DxeMain.inf {
     <LibraryClasses>
+      NULL|ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf
       NULL|MdeModulePkg/Library/DxeCrc32GuidedSectionExtractLib/DxeCrc32GuidedSectionExtractLib.inf
       DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
   }
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index 807c85d48285..1ea49fd32e9c 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -278,6 +278,7 @@ [Components.common]
   #
   MdeModulePkg/Core/Dxe/DxeMain.inf {
     <LibraryClasses>
+      NULL|ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf
       NULL|MdeModulePkg/Library/DxeCrc32GuidedSectionExtractLib/DxeCrc32GuidedSectionExtractLib.inf
       DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
   }
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
index 9cf43f06c073..aa083cec2082 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
@@ -91,7 +91,7 @@ ArmVirtGetMemoryMap (
   VirtualMemoryTable[0].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
   VirtualMemoryTable[0].VirtualBase  = VirtualMemoryTable[0].PhysicalBase;
   VirtualMemoryTable[0].Length       = *(UINT64 *)GET_GUID_HOB_DATA (MemorySizeHob);
-  VirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+  VirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_XP;
 
   DEBUG ((
     DEBUG_INFO,
-- 
2.39.1

[RFC 10/13] ArmVirtPkg/ArmVirtQemu: Use PEI flavor of ArmMmuLib for all PEIMs

[Ard Biesheuvel]

The PEI flavor of the ArmMmuLib will install a HOB that exposes its
implementation of the special helper routine that is used to update live
entries, so that other instantiations of ArmMmuLib can invoke it. This
is needed to ensure that splitting page tables using break-before-make
(BBM) does not unmap the code that is performing the split.

However, the BASE variety of ArmMmuLib discovers the HOB and sets a
global pointer to refer to it, which is not possible in PEIMs, and so
all PEIMs must use the PEI variety of this library if one does.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index dd4c84ae6eb9..69e51d19300d 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -107,6 +107,9 @@ [LibraryClasses.common.PEIM]
   Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
 !endif
 
+[LibraryClasses.AARCH64.PEIM]
+  ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
+
 [LibraryClasses.common.DXE_DRIVER]
   ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
 
@@ -333,12 +336,7 @@ [Components.common]
   ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
   MdeModulePkg/Core/Pei/PeiMain.inf
   ArmPlatformPkg/PlatformPei/PlatformPeim.inf
-  ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf {
-    <LibraryClasses>
-!if $(ARCH) == AARCH64
-      ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
-!endif
-  }
+  ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf
   ArmPkg/Drivers/CpuPei/CpuPei.inf
 
 !if $(TPM2_ENABLE) == TRUE
-- 
2.39.1

[RFC 11/13] ArmVirtPkg/ArmVirtQemu: Use read-only memory region type for code flash

[Ard Biesheuvel]

Map the code flash with read-only attributes so we can execute from it
even under a memory protection regime that enables WXN, making all
writable memory regions non-executable by default.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
index aa083cec2082..a5324b1e4eed 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
@@ -115,7 +115,7 @@ ArmVirtGetMemoryMap (
   VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
   VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
   VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFvSize);
-  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO;
 
   // End of Table
   ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
-- 
2.39.1

[RFC 12/13] BaseTools/GccBase AARCH64: Avoid page sharing between code and data

[Ard Biesheuvel]

The AArch64 ARM architecture supports a hardware enforcement mode for
mutual exclusion between code and data: any page that is mapped writable
is implicitly non-executable as well.

This means that remapping part of a runtime image for reapplying
relocation fixups may result in any code sharing the same page to lose
its executable permissions.

Let's avoid this, by moving all quantities that are subject to
relocation fixups to a separate page if the build is using 64k section
alignment, which is only the case when building a runtime driver for
AArch64.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 BaseTools/Scripts/GccBase.lds | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
index 83cebd29d599..63e097e0727c 100644
--- a/BaseTools/Scripts/GccBase.lds
+++ b/BaseTools/Scripts/GccBase.lds
@@ -21,9 +21,8 @@ SECTIONS {
   . = PECOFF_HEADER_SIZE;
 
   .text : ALIGN(CONSTANT(COMMONPAGESIZE)) {
-    *(.text .text.* .stub .gnu.linkonce.t.*)
+    *(.text .text.* .stub .gnu.linkonce.t.* .plt)
     *(.rodata .rodata.* .gnu.linkonce.r.*)
-    *(.got .got.*)
 
     /*
      * The contents of AutoGen.c files are mostly constant from the POV of the
@@ -34,6 +33,16 @@ SECTIONS {
      * emitted GUIDs here.
      */
     *:AutoGen.obj(.data.g*Guid)
+
+    /*
+     * AArch64 runtime drivers use 64k alignment, and may run in a mode where
+     * mutual exclusion of RO and XP mappings are hardware enforced. In such
+     * cases, the input sections below, which carry any quantities that are
+     * subject to relocation fixups at runtime, must not share a 4 KiB page
+     * with any code content.
+     */
+    . = ALIGN(CONSTANT(COMMONPAGESIZE) > 0x1000 ? 0x1000 : 0x20);
+    *(.got .got.* .data.rel.ro)
   }
 
   /*
-- 
2.39.1

[RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions

[Ard Biesheuvel]

Enable the WXN system control bit straight out of reset when running in
EL1 with the initial ID map from flash. This setting will be inherited
by the page table code after it sets up the permanent boot time page
tables, resulting in all memory mappings that are not explicitly mapped
as read-only to be non-executable.

Note that this requires runtime drivers to be built with position
independent codegen, to ensure that all absolute symbol references are
moved into a separate section in the binary. Otherwise, unmapping the
pages that are subject to relocation fixups at runtime (during the
invocation of SetVirtualAddressMap()) could result in code mappings
losing their executable permissions.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmVirtPkg/ArmVirt.dsc.inc                                        | 1 +
 ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 5b18184be263..928dd6330edb 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -31,6 +31,7 @@ [BuildOptions.common.EDKII.DXE_CORE,BuildOptions.common.EDKII.DXE_DRIVER,BuildOp
 
 [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
   GCC:*_*_ARM_DLINK_FLAGS = -z common-page-size=0x1000
+  GCC:*_*_AARCH64_CC_FLAGS = -fpie
   GCC:*_*_AARCH64_DLINK_FLAGS = -z common-page-size=0x10000
 
 [LibraryClasses.common]
diff --git a/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S b/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
index 5ac7c732f6ec..51c089a45ffc 100644
--- a/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
+++ b/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
@@ -38,7 +38,7 @@
  .set    SCTLR_EL1_ITD,       0x1 << 7
  .set    SCTLR_EL1_RES1,      (0x1 << 11) | (0x1 << 20) | (0x1 << 22) | (0x1 << 28) | (0x1 << 29)
  .set    sctlrval, SCTLR_ELx_M | SCTLR_ELx_C | SCTLR_ELx_SA | SCTLR_EL1_ITD | SCTLR_EL1_SED
- .set    sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES1
+ .set    sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES1 | SCTLR_EL1_WXN
 
 
 ASM_FUNC(ArmPlatformPeiBootAction)
-- 
2.39.1

Re: [edk2-devel] [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 4294 bytes --]

Hey Ard,

*Praise* to you for this series. Comments inline.

On Mon, Feb 13, 2023 at 07:19 AM, Ard Biesheuvel wrote:

> 
> Enable the WXN system control bit straight out of reset when running in
> EL1 with the initial ID map from flash. This setting will be inherited
> by the page table code after it sets up the permanent boot time page
> tables, resulting in all memory mappings that are not explicitly mapped
> as read-only to be non-executable.
> 
> Note that this requires runtime drivers to be built with position
> independent codegen, to ensure that all absolute symbol references are
> moved into a separate section in the binary. Otherwise, unmapping the
> pages that are subject to relocation fixups at runtime (during the
> invocation of SetVirtualAddressMap()) could result in code mappings
> losing their executable permissions.

I never actually thought about this. SetVirtualAddressMap() will have to relocate its own parent binary, causing issues for software W^X when .text relocs are present (like with MSVC builds). :(

> 
> 
> Signed-off-by: Ard Biesheuvel <ardb@...>
> ---
> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
> ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 5b18184be263..928dd6330edb 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -31,6 +31,7 @@
> [BuildOptions.common.EDKII.DXE_CORE,BuildOptions.common.E=
> DKII.DXE_DRIVER,BuildOp
> =0D
> [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]=0D
> GCC:*_*_ARM_DLINK_FLAGS =3D -z common-page-size=3D0x1000=0D
> + GCC:*_*_AARCH64_CC_FLAGS =3D -fpie=0D

Doesn't this mean -pie must be passed to the linker? I saw in the previous patch that .plt was added to the linker script, was there a particular reason -fno-plt wasn't used here? I just read it may have some unexpected side-effects, but I thought it would be safe for our statically-linked UEFI environment.

On another (related) matter, I've been spending my last two days looking into the whole ELF-to-PE process, because GenFw has been becoming unbearable to us downstream. I went through a bunch of old commits which deal with PIE and saw it was usually disabled but for X64. The funny thing with X64 (even currently) is, that -fpie is combined with -q (a.k.a. --emit-relocs), yielding both object file relocs (.rela.sectname) and PIE-related relative relocs (.rela) in the same binary (as documented in GenFw, they may overlap!). It's my understanding that GenFw currently processes exclusively the -q relocs and not the -fpie relocs (which should be safe as done for X64, I have no experience with ARM whatsoever). However, when PIE is involved anyway, it makes most sense to me to use its related relocs for the translation over a dance with the object file relocs. This change will cause the same behaviour for AARCH64 RT drivers now, right?

In an ideal world, I suppose all architectures but IA32 (due to lacking efficient pcrel addressing) should be using PIE, as most (often all with X64) GOT references can be relaxed, as we strictly deal with local symbols. Though I have to wonder how unideal the world really is. :)

Best regards,
Marvin

> 
> GCC:*_*_AARCH64_DLINK_FLAGS =3D -z common-page-size=3D0x10000=0D
> =0D
> [LibraryClasses.common]=0D
> diff --git
> a/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelpe=
> r.S b/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
> index 5ac7c732f6ec..51c089a45ffc 100644
> --- a/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
> +++ b/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
> @@ -38,7 +38,7 @@
> .set SCTLR_EL1_ITD, 0x1 << 7=0D
> .set SCTLR_EL1_RES1, (0x1 << 11) | (0x1 << 20) | (0x1 << 22) | (0=
> x1 << 28) | (0x1 << 29)=0D
> .set sctlrval, SCTLR_ELx_M | SCTLR_ELx_C | SCTLR_ELx_SA | SCTLR_EL1_IT=
> D | SCTLR_EL1_SED=0D
> - .set sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES=
> 1=0D
> + .set sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES=
> 1 | SCTLR_EL1_WXN=0D
> =0D
> =0D
> ASM_FUNC(ArmPlatformPeiBootAction)=0D
> --=20
> 2.39.1

[-- Attachment #2: Type: text/html, Size: 4549 bytes --]

Re: [edk2-devel] [RFC 07/13] MdeModulePkg/DxeCore: Permit preliminary CPU arch fallback

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

Without wanting to blow up your RFC with another one - I discussed this with various people, including Bret when he was still at Project Mu, and there was a consensus among them that integrating the whole CPU arch code right into DxeCore would be a good idea. This would especially remove the hack that queues permission applications till CpuDxe loads for good, rather than requiring pro-active consumption of a library that proves this "fallback". For most of the architectural protocols, especially SecurityStubDxe, I never got the gist why you would want them to be separate from DxeCore. Obviously there should be a level of customizability for IBVs and OEMs, though that can be done statically-linked as well.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3223

What's your take on this?

Best regards,
Marvin

[-- Attachment #2: Type: text/html, Size: 929 bytes --]

Re: [edk2-devel] [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions

[Ard Biesheuvel]

On Mon, 13 Feb 2023 at 22:16, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Hey Ard,
>
> *Praise* to you for this series. Comments inline.
>

Thanks :-)

> On Mon, Feb 13, 2023 at 07:19 AM, Ard Biesheuvel wrote:
>
> Enable the WXN system control bit straight out of reset when running in
> EL1 with the initial ID map from flash. This setting will be inherited
> by the page table code after it sets up the permanent boot time page
> tables, resulting in all memory mappings that are not explicitly mapped
> as read-only to be non-executable.
>
> Note that this requires runtime drivers to be built with position
> independent codegen, to ensure that all absolute symbol references are
> moved into a separate section in the binary. Otherwise, unmapping the
> pages that are subject to relocation fixups at runtime (during the
> invocation of SetVirtualAddressMap()) could result in code mappings
> losing their executable permissions.
>
> I never actually thought about this. SetVirtualAddressMap() will have to relocate its own parent binary, causing issues for software W^X when .text relocs are present (like with MSVC builds). :(
>
>
> Signed-off-by: Ard Biesheuvel <ardb@...>
> ---
> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
> ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 5b18184be263..928dd6330edb 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -31,6 +31,7 @@ [BuildOptions.common.EDKII.DXE_CORE,BuildOptions.common.E=
> DKII.DXE_DRIVER,BuildOp
> =0D
> [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]=0D
> GCC:*_*_ARM_DLINK_FLAGS =3D -z common-page-size=3D0x1000=0D
> + GCC:*_*_AARCH64_CC_FLAGS =3D -fpie=0D
>
> Doesn't this mean -pie must be passed to the linker? I saw in the previous patch that .plt was added to the linker script, was there a particular reason -fno-plt wasn't used here? I just read it may have some unexpected side-effects, but I thought it would be safe for our statically-linked UEFI environment.
>

No, the only reason for adding -fpie here is to ensure that statically
initialized CONST pointers are emitted into .data.rel.ro and not into
.rodata, as this is under the control of the compiler. Although,
thinking about this, I wonder if we need to pass this to the linker
for codegen under LTO as well. But the PIE link itself should be
unnecessary here.

> On another (related) matter, I've been spending my last two days looking into the whole ELF-to-PE process, because GenFw has been becoming unbearable to us downstream. I went through a bunch of old commits which deal with PIE and saw it was usually disabled but for X64. The funny thing with X64 (even currently) is, that -fpie is combined with -q (a.k.a. --emit-relocs), yielding both object file relocs (.rela.sectname) and PIE-related relative relocs (.rela) in the same binary (as documented in GenFw, they may overlap!). It's my understanding that GenFw currently processes exclusively the -q relocs and not the -fpie relocs (which should be safe as done for X64, I have no experience with ARM whatsoever). However, when PIE is involved anyway, it makes most sense to me to use its related relocs for the translation over a dance with the object file relocs. This change will cause the same behaviour for AARCH64 RT drivers now, right?
>

It will if you pass -pie to the linker, which is why I would prefer to
avoid that. The main issue IIRC is that the emit-relocs section does
not cover the entries in the GOT table that also require relocation,
and are only covered by the PIE .rela section. For AArch64, I added
relaxation logic to GenFw to actually patch the instructions instead,
which is always possible given the absence of dynamic linking.
(d2687f23c909475d80cef32cdf9a5d121f0a9ae6,
7b8f69d7e10628d473dd225224d8c2122d25a38d)

This means that we don't have to care about compiler generated symbol
references, and so the relocs emitted by emit-relocs are sufficient,
and the additional ones emitted into .rela are unused anyway. The only
remaining absolute references are the ones resulting from statically
initialized globals, and those will either be in .data or in
.data.rel.ro (if -fpie is being used)

But I agree that not using --emit-relocs and only relying on the .rela
section to populate the PE/COFF reloc section would be far cleaner.

> In an ideal world, I suppose all architectures but IA32 (due to lacking efficient pcrel addressing) should be using PIE, as most (often all with X64) GOT references can be relaxed, as we strictly deal with local symbols. Though I have to wonder how unideal the world really is. :)
>

Re: [edk2-devel] [RFC 07/13] MdeModulePkg/DxeCore: Permit preliminary CPU arch fallback

[Ard Biesheuvel]

On Mon, 13 Feb 2023 at 22:32, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Without wanting to blow up your RFC with another one - I discussed this with various people, including Bret when he was still at Project Mu, and there was a consensus among them that integrating the whole CPU arch code right into DxeCore would be a good idea. This would especially remove the hack that queues permission applications till CpuDxe loads for good, rather than requiring pro-active consumption of a library that proves this "fallback". For most of the architectural protocols, especially SecurityStubDxe, I never got the gist why you would want them to be separate from DxeCore. Obviously there should be a level of customizability for IBVs and OEMs, though that can be done statically-linked as well.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3223
>
> What's your take on this?
>

My take is that page table manipulation should not be part of the CPU
arch protocol. The DXE core loads images and dispatches them, which
also involves cache maintenance, for instance, as code pages need to
be invalidated from the I-cache before you can execute a newly loaded
image. I think it makes perfect sense for the DXE core to be in charge
of updating the page table descriptors as well.

In my approach, the library version is only used before the CPU arch
protocol appears, so it addresses some of the concerns regarding
multiple owners. But I'd prefer to see this removed from the CPU arch
protocol entirely, or at least for the CPU arch protocol interface to
be deprecated, and redefined in terms of a new DXE services, for
instance, that is implemented by the DXE core itself. That way, all
existing users would still see the same protocols, but the way they
depend on each other would be inverted.

Re: [edk2-devel] [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 2188 bytes --]


> On 13. Feb 2023, at 22:59, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> No, the only reason for adding -fpie here is to ensure that statically
> initialized CONST pointers are emitted into .data.rel.ro and not into
> .rodata, as this is under the control of the compiler. Although,
> thinking about this, I wonder if we need to pass this to the linker
> for codegen under LTO as well. But the PIE link itself should be
> unnecessary here.

Oh, what fun. For some reason I thought it would be unsafe to specify -fpie but not -pie, but considering PIE relocs are ignored either way, this actually makes perfect sense. Sorry! About that last part, the docs say: "It is recommended that you compile all the files participating in the same link with the same options and also specify those options at link time." [1], so good catch!

[1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#Optimize-Options

But what about -fno-plt?

> 
> It will if you pass -pie to the linker, which is why I would prefer to
> avoid that. The main issue IIRC is that the emit-relocs section does
> not cover the entries in the GOT table that also require relocation,
> and are only covered by the PIE .rela section. For AArch64, I added
> relaxation logic to GenFw to actually patch the instructions instead,
> which is always possible given the absence of dynamic linking.
> (d2687f23c909475d80cef32cdf9a5d121f0a9ae6,
> 7b8f69d7e10628d473dd225224d8c2122d25a38d)

Yes, seen, very nice. I do wonder though why GOT entries are generated in the first place when symbols are all local and data is within the PC-addressable range. Just today, for a X64 build, I actually saw Clang relax a GOT reference to __stack_chk_guard itself.

> 
> This means that we don't have to care about compiler generated symbol
> references, and so the relocs emitted by emit-relocs are sufficient,
> and the additional ones emitted into .rela are unused anyway. The only
> remaining absolute references are the ones resulting from statically
> initialized globals, and those will either be in .data or in
> .data.rel.ro (if -fpie is being used)

Right. thank you.

Best regards,
Marvin

[-- Attachment #2: Type: text/html, Size: 2832 bytes --]

Re: [edk2-devel] [RFC 07/13] MdeModulePkg/DxeCore: Permit preliminary CPU arch fallback

[Marvin Häuser]

Sounds good to me, thanks!

Best regards,
Marvin

> On 13. Feb 2023, at 23:07, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Mon, 13 Feb 2023 at 22:32, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> Without wanting to blow up your RFC with another one - I discussed this with various people, including Bret when he was still at Project Mu, and there was a consensus among them that integrating the whole CPU arch code right into DxeCore would be a good idea. This would especially remove the hack that queues permission applications till CpuDxe loads for good, rather than requiring pro-active consumption of a library that proves this "fallback". For most of the architectural protocols, especially SecurityStubDxe, I never got the gist why you would want them to be separate from DxeCore. Obviously there should be a level of customizability for IBVs and OEMs, though that can be done statically-linked as well.
>> 
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3223
>> 
>> What's your take on this?
>> 
> 
> My take is that page table manipulation should not be part of the CPU
> arch protocol. The DXE core loads images and dispatches them, which
> also involves cache maintenance, for instance, as code pages need to
> be invalidated from the I-cache before you can execute a newly loaded
> image. I think it makes perfect sense for the DXE core to be in charge
> of updating the page table descriptors as well.
> 
> In my approach, the library version is only used before the CPU arch
> protocol appears, so it addresses some of the concerns regarding
> multiple owners. But I'd prefer to see this removed from the CPU arch
> protocol entirely, or at least for the CPU arch protocol interface to
> be deprecated, and redefined in terms of a new DXE services, for
> instance, that is implemented by the DXE core itself. That way, all
> existing users would still see the same protocols, but the way they
> depend on each other would be inverted.

Re: [edk2-devel] [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions

[Ard Biesheuvel]

On Mon, 13 Feb 2023 at 23:23, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> On 13. Feb 2023, at 22:59, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> No, the only reason for adding -fpie here is to ensure that statically
> initialized CONST pointers are emitted into .data.rel.ro and not into
> .rodata, as this is under the control of the compiler. Although,
> thinking about this, I wonder if we need to pass this to the linker
> for codegen under LTO as well. But the PIE link itself should be
> unnecessary here.
>
>
> Oh, what fun. For some reason I thought it would be unsafe to specify -fpie but not -pie, but considering PIE relocs are ignored either way, this actually makes perfect sense. Sorry! About that last part, the docs say: "It is recommended that you compile all the files participating in the same link with the same options and also specify those options at link time." [1], so good catch!
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#Optimize-Options
>

Yeah, but come to think of it, we pass the CC flags to the linker, so
this should be covered.

> But what about -fno-plt?
>

That doesn't make a difference in codegen on AArch64 - the same BL
instruction is emitted along with the same type of relocation, and it
is left up to the linker whether or not a PLT entry is emitted, and I
don't think it will ever do that when generating a fully linked binary
without -pie.

>
> It will if you pass -pie to the linker, which is why I would prefer to
> avoid that. The main issue IIRC is that the emit-relocs section does
> not cover the entries in the GOT table that also require relocation,
> and are only covered by the PIE .rela section. For AArch64, I added
> relaxation logic to GenFw to actually patch the instructions instead,
> which is always possible given the absence of dynamic linking.
> (d2687f23c909475d80cef32cdf9a5d121f0a9ae6,
> 7b8f69d7e10628d473dd225224d8c2122d25a38d)
>
>
> Yes, seen, very nice. I do wonder though why GOT entries are generated in the first place when symbols are all local and data is within the PC-addressable range. Just today, for a X64 build, I actually saw Clang relax a GOT reference to __stack_chk_guard itself.
>

Yeah there are some strange corner cases, but in general, all symbol
references are relative - it is precisely the 'special' symbol
references that the compiler generates internally that sometimes get
this wrong.

>
> This means that we don't have to care about compiler generated symbol
> references, and so the relocs emitted by emit-relocs are sufficient,
> and the additional ones emitted into .rela are unused anyway. The only
> remaining absolute references are the ones resulting from statically
> initialized globals, and those will either be in .data or in
> .data.rel.ro (if -fpie is being used)
>
>
> Right. thank you.
>
> Best regards,
> Marvin
>