[PATCH v3 0/4] ArmPkg, ArmVirtpkg ARM: enable strict memory protection

[PATCH v3 0/4] ArmPkg, ArmVirtpkg ARM: enable strict memory protection

[Ard Biesheuvel]

This series makes the prerequisite modifications to the ARM version of
the CpuDxe driver so we can enable PE/COFF image and NX memory protection
for ARM platforms, including ArmVirtPkg (#4)

Patch #1 refactors CpuSetMemoryAttributes() so it no longer splits section
mappings into page mappings unnecessarily.

Patch #2 removes some unnecessary cache/TLB maintenance, which becomes very
costly when CpuSetMemoryAttributes() is used in anger as is the case with
memory protections enabled.

Patch #3 wires up the EFI_MEMORY_RO/EFI_MEMORY_XP attributes, which were
ignored before.

Patch #4 enables the protection features for ArmVirtPkg platforms when
built for 32-bit ARM.

Changes since v2:
- minor code tweaks for style and correctness
- add R-b's from Leif to #1 and #2

Changes since v1:
- trigger full TLB flush when UpdatePageEntries() results in a section split
- Make cache maintenance of the remapped regions conditional on whether the
  memory type changed. This prevents an inadvertent cache clean/invalidate by
  VA of the entire RAM area when the NX attribute is applied to it.
- remove DEBUG_INFO attribute from SetMemoryAttributes DEBUG output
- add Laszlo's R-b to #4

Ard Biesheuvel (4):
  ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily
  ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance
  ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes()
  ArmVirtPkg: enable PE/COFF image and memory protection for ARM
    platforms

 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 277 +++++++++++---------
 ArmVirtPkg/ArmVirt.dsc.inc      |   9 +-
 2 files changed, 161 insertions(+), 125 deletions(-)

-- 
2.7.4

[PATCH v3 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily

[Ard Biesheuvel]

Currently, any range passed to CpuArchProtocol::SetMemoryAttributes is
fully broken down into page mappings if the start or the size of the
region happens to be misaliged relative to the section size of 1 MB.

This is going to result in memory being wasted on second level page tables
when we enable strict memory permissions, given that we remap the entire
RAM space non-executable (modulo the code bits) when the CpuArchProtocol
is installed.

So refactor the code to iterate over the range in a way that ensures
that all naturally aligned section sized subregions are not broken up.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 51 +++++++++++++++++---
 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 89e429925ba9..16d6fcef9f1c 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -679,6 +679,11 @@ SetMemoryAttributes (
   )
 {
   EFI_STATUS    Status;
+  UINT64        ChunkLength;
+
+  if (Length == 0) {
+    return EFI_SUCCESS;
+  }
 
   //
   // Ignore invocations that only modify permission bits
@@ -687,14 +692,44 @@ SetMemoryAttributes (
     return EFI_SUCCESS;
   }
 
-  if(((BaseAddress & 0xFFFFF) == 0) && ((Length & 0xFFFFF) == 0)) {
-    // Is the base and length a multiple of 1 MB?
-    DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
-    Status = UpdateSectionEntries (BaseAddress, Length, Attributes, VirtualMask);
-  } else {
-    // Base and/or length is not a multiple of 1 MB
-    DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU page 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
-    Status = UpdatePageEntries (BaseAddress, Length, Attributes, VirtualMask);
+  while (Length > 0) {
+    if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
+        Length >= TT_DESCRIPTOR_SECTION_SIZE) {
+
+      ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
+
+      DEBUG ((DEBUG_PAGE,
+        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
+        BaseAddress, ChunkLength, Attributes));
+
+      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
+                 VirtualMask);
+    } else {
+
+      //
+      // Process page by page until the next section boundary, but only if
+      // we have more than a section's worth of area to deal with after that.
+      //
+      ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
+                    (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
+      if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
+        ChunkLength = Length;
+      }
+
+      DEBUG ((DEBUG_PAGE,
+        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
+        BaseAddress, ChunkLength, Attributes));
+
+      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
+                 VirtualMask);
+    }
+
+    if (EFI_ERROR (Status)) {
+      break;
+    }
+
+    BaseAddress += ChunkLength;
+    Length -= ChunkLength;
   }
 
   // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
-- 
2.7.4

[PATCH v3 2/4] ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance

[Ard Biesheuvel]

Page and section entries in the page tables are updated using the
helper ArmUpdateTranslationTableEntry(), which cleans the page
table entry to the PoC, and invalidates the TLB entry covering
the page described by the entry being updated.

Since we may be updating section entries, we might be leaving stale
TLB entries at this point (for all pages in the section except the
first one), which will be invalidated wholesale at the end of
SetMemoryAttributes(). At that point, all caches are cleaned *and*
invalidated as well.

This cache maintenance is costly and unnecessary. The TLB maintenance
is only necessary if we updated any section entries, since any page
by page entries that have been updated will have been invalidated
individually by ArmUpdateTranslationTableEntry().

So drop the clean/invalidate of the caches, and only perform the
full TLB flush if UpdateSectionEntries() was called, or if sections
were split by UpdatePageEntries(). Finally, make the cache maintenance
on the remapped regions themselves conditional on whether any memory
type attributes were modified.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 60 +++++++++++---------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 16d6fcef9f1c..a2993cf16a35 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -347,10 +347,11 @@ SyncCacheConfig (
 
 EFI_STATUS
 UpdatePageEntries (
-  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
-  IN UINT64                    Length,
-  IN UINT64                    Attributes,
-  IN EFI_PHYSICAL_ADDRESS      VirtualMask
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length,
+  IN  UINT64                    Attributes,
+  IN  EFI_PHYSICAL_ADDRESS      VirtualMask,
+  OUT BOOLEAN                   *FlushTlbs OPTIONAL
   )
 {
   EFI_STATUS    Status;
@@ -446,6 +447,9 @@ UpdatePageEntries (
 
       // Re-read descriptor
       Descriptor = FirstLevelTable[FirstLevelIdx];
+      if (FlushTlbs != NULL) {
+        *FlushTlbs = TRUE;
+      }
     }
 
     // Obtain page table base address
@@ -471,15 +475,16 @@ UpdatePageEntries (
 
     if (CurrentPageTableEntry  != PageTableEntry) {
       Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
-      if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
-        // The current section mapping is cacheable so Clean/Invalidate the MVA of the page
-        // Note assumes switch(Attributes), not ARMv7 possibilities
-        WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
-      }
 
       // Only need to update if we are changing the entry
       PageTable[PageTableIndex] = PageTableEntry;
       ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
+
+      // Clean/invalidate the cache for this page, but only
+      // if we are modifying the memory type attributes
+      if (((CurrentPageTableEntry ^ PageTableEntry) & TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK) != 0) {
+        WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
+      }
     }
 
     Status = EFI_SUCCESS;
@@ -581,7 +586,12 @@ UpdateSectionEntries (
     // has this descriptor already been coverted to pages?
     if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
       // forward this 1MB range to page table function instead
-      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
+      Status = UpdatePageEntries (
+                 (FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT,
+                 TT_DESCRIPTOR_SECTION_SIZE,
+                 Attributes,
+                 VirtualMask,
+                 NULL);
     } else {
       // still a section entry
 
@@ -596,15 +606,16 @@ UpdateSectionEntries (
 
       if (CurrentDescriptor  != Descriptor) {
         Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
-        if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) {
-          // The current section mapping is cacheable so Clean/Invalidate the MVA of the section
-          // Note assumes switch(Attributes), not ARMv7 possabilities
-          WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
-        }
 
         // Only need to update if we are changing the descriptor
         FirstLevelTable[FirstLevelIdx + i] = Descriptor;
         ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
+
+        // Clean/invalidate the cache for this section, but only
+        // if we are modifying the memory type attributes
+        if (((CurrentDescriptor ^ Descriptor) & TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK) != 0) {
+          WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
+        }
       }
 
       Status = EFI_SUCCESS;
@@ -680,6 +691,7 @@ SetMemoryAttributes (
 {
   EFI_STATUS    Status;
   UINT64        ChunkLength;
+  BOOLEAN       FlushTlbs;
 
   if (Length == 0) {
     return EFI_SUCCESS;
@@ -692,6 +704,7 @@ SetMemoryAttributes (
     return EFI_SUCCESS;
   }
 
+  FlushTlbs = FALSE;
   while (Length > 0) {
     if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
         Length >= TT_DESCRIPTOR_SECTION_SIZE) {
@@ -704,6 +717,8 @@ SetMemoryAttributes (
 
       Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
                  VirtualMask);
+
+      FlushTlbs = TRUE;
     } else {
 
       //
@@ -721,7 +736,7 @@ SetMemoryAttributes (
         BaseAddress, ChunkLength, Attributes));
 
       Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
-                 VirtualMask);
+                 VirtualMask, &FlushTlbs);
     }
 
     if (EFI_ERROR (Status)) {
@@ -732,16 +747,9 @@ SetMemoryAttributes (
     Length -= ChunkLength;
   }
 
-  // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
-  // flush and invalidate pages
-  //TODO: Do we really need to invalidate the caches everytime we change the memory attributes ?
-  ArmCleanInvalidateDataCache ();
-
-  ArmInvalidateInstructionCache ();
-
-  // Invalidate all TLB entries so changes are synced
-  ArmInvalidateTlb ();
-
+  if (FlushTlbs) {
+    ArmInvalidateTlb ();
+  }
   return Status;
 }
 
-- 
2.7.4

[PATCH v3 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes()

[Ard Biesheuvel]

Enable the use of strict memory permissions on ARM by processing the
EFI_MEMORY_RO and EFI_MEMORY_XP rather than ignoring them. As before,
calls to CpuArchProtocol::SetMemoryAttributes that only set RO/XP
bits will preserve the cacheability attributes. Permissions attributes
are not preserved when setting the memory type only: the way the memory
permission attributes are defined does not allows for that, and so this
situation does not deviate from other architectures.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 178 ++++++++++----------
 1 file changed, 86 insertions(+), 92 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index a2993cf16a35..d3c307f48317 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -19,6 +19,13 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Library/MemoryAllocationLib.h>
 #include "CpuDxe.h"
 
+#define CACHE_ATTRIBUTE_MASK   (EFI_MEMORY_UC | \
+                                EFI_MEMORY_WC | \
+                                EFI_MEMORY_WT | \
+                                EFI_MEMORY_WB | \
+                                EFI_MEMORY_UCE | \
+                                EFI_MEMORY_WP)
+
 // First Level Descriptors
 typedef UINT32    ARM_FIRST_LEVEL_DESCRIPTOR;
 
@@ -374,50 +381,48 @@ UpdatePageEntries (
 
   // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
   // EntryValue: values at bit positions specified by EntryMask
-  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK;
-  EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
-  // Although the PI spec is unclear on this the GCD guarantees that only
-  // one Attribute bit is set at a time, so we can safely use a switch statement
-  switch (Attributes) {
-    case EFI_MEMORY_UC:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
-      // map to strongly ordered
-      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
-      break;
-
-    case EFI_MEMORY_WC:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
-      // map to normal non-cachable
-      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
-      break;
-
-    case EFI_MEMORY_WT:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
-      // write through with no-allocate
-      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
-      break;
-
-    case EFI_MEMORY_WB:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
-      // write back (with allocate)
-      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
-      break;
+  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;
+  if ((Attributes & EFI_MEMORY_XP) != 0) {
+    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;
+  } else {
+    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
+  }
 
-    case EFI_MEMORY_WP:
-    case EFI_MEMORY_XP:
-    case EFI_MEMORY_UCE:
-      // cannot be implemented UEFI definition unclear for ARM
-      // Cause a page fault if these ranges are accessed.
-      EntryValue = TT_DESCRIPTOR_PAGE_TYPE_FAULT;
-      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting page %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
-      break;
+  // Although the PI spec is unclear on this, the GCD guarantees that only
+  // one Attribute bit is set at a time, so the order of the conditionals below
+  // is irrelevant. If no memory attribute is specified, we preserve whatever
+  // memory type is set in the page tables, and update the permission attributes
+  // only.
+  if (Attributes & EFI_MEMORY_UC) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+    // map to strongly ordered
+    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
+  } else if (Attributes & EFI_MEMORY_WC) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+    // map to normal non-cachable
+    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
+  } else if (Attributes & EFI_MEMORY_WT) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+    // write through with no-allocate
+    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
+  } else if (Attributes & EFI_MEMORY_WB) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+    // write back (with allocate)
+    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
+  } else if (Attributes & CACHE_ATTRIBUTE_MASK) {
+    // catch unsupported memory type attributes
+    ASSERT (FALSE);
+    return EFI_UNSUPPORTED;
+  }
 
-    default:
-      return EFI_UNSUPPORTED;
+  if ((Attributes & EFI_MEMORY_RO) != 0) {
+    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
+  } else {
+    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
   }
 
   // Obtain page table base
@@ -520,53 +525,49 @@ UpdateSectionEntries (
   // EntryValue: values at bit positions specified by EntryMask
 
   // Make sure we handle a section range that is unmapped
-  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK;
+  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
+              TT_DESCRIPTOR_SECTION_AP_MASK;
   EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
 
-  // Although the PI spec is unclear on this the GCD guarantees that only
-  // one Attribute bit is set at a time, so we can safely use a switch statement
-  switch(Attributes) {
-    case EFI_MEMORY_UC:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
-      // map to strongly ordered
-      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
-      break;
-
-    case EFI_MEMORY_WC:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
-      // map to normal non-cachable
-      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
-      break;
-
-    case EFI_MEMORY_WT:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
-      // write through with no-allocate
-      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
-      break;
-
-    case EFI_MEMORY_WB:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
-      // write back (with allocate)
-      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
-      break;
-
-    case EFI_MEMORY_WP:
-    case EFI_MEMORY_XP:
-    case EFI_MEMORY_RP:
-    case EFI_MEMORY_UCE:
-      // cannot be implemented UEFI definition unclear for ARM
-      // Cause a page fault if these ranges are accessed.
-      EntryValue = TT_DESCRIPTOR_SECTION_TYPE_FAULT;
-      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting section %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
-      break;
+  // Although the PI spec is unclear on this, the GCD guarantees that only
+  // one Attribute bit is set at a time, so the order of the conditionals below
+  // is irrelevant. If no memory attribute is specified, we preserve whatever
+  // memory type is set in the page tables, and update the permission attributes
+  // only.
+  if (Attributes & EFI_MEMORY_UC) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+    // map to strongly ordered
+    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
+  } else if (Attributes & EFI_MEMORY_WC) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+    // map to normal non-cachable
+    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
+  } else if (Attributes & EFI_MEMORY_WT) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+    // write through with no-allocate
+    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
+  } else if (Attributes & EFI_MEMORY_WB) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+    // write back (with allocate)
+    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
+  } else if (Attributes & CACHE_ATTRIBUTE_MASK) {
+    // catch unsupported memory type attributes
+    ASSERT (FALSE);
+    return EFI_UNSUPPORTED;
+  }
 
+  if (Attributes & EFI_MEMORY_RO) {
+    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
+  } else {
+    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
+  }
 
-    default:
-      return EFI_UNSUPPORTED;
+  if (Attributes & EFI_MEMORY_XP) {
+    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
   }
 
   // obtain page table base
@@ -697,13 +698,6 @@ SetMemoryAttributes (
     return EFI_SUCCESS;
   }
 
-  //
-  // Ignore invocations that only modify permission bits
-  //
-  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
-    return EFI_SUCCESS;
-  }
-
   FlushTlbs = FALSE;
   while (Length > 0) {
     if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
-- 
2.7.4

Re: [PATCH v3 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes()

[Leif Lindholm]

On Mon, Mar 06, 2017 at 06:32:14PM +0100, Ard Biesheuvel wrote:
> Enable the use of strict memory permissions on ARM by processing the
> EFI_MEMORY_RO and EFI_MEMORY_XP rather than ignoring them. As before,
> calls to CpuArchProtocol::SetMemoryAttributes that only set RO/XP
> bits will preserve the cacheability attributes. Permissions attributes
> are not preserved when setting the memory type only: the way the memory
> permission attributes are defined does not allows for that, and so this
> situation does not deviate from other architectures.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 178 ++++++++++----------
>  1 file changed, 86 insertions(+), 92 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index a2993cf16a35..d3c307f48317 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -19,6 +19,13 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Library/MemoryAllocationLib.h>
>  #include "CpuDxe.h"
>  
> +#define CACHE_ATTRIBUTE_MASK   (EFI_MEMORY_UC | \
> +                                EFI_MEMORY_WC | \
> +                                EFI_MEMORY_WT | \
> +                                EFI_MEMORY_WB | \
> +                                EFI_MEMORY_UCE | \
> +                                EFI_MEMORY_WP)
> +
>  // First Level Descriptors
>  typedef UINT32    ARM_FIRST_LEVEL_DESCRIPTOR;
>  
> @@ -374,50 +381,48 @@ UpdatePageEntries (
>  
>    // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
>    // EntryValue: values at bit positions specified by EntryMask
> -  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK;
> -  EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
> -  // Although the PI spec is unclear on this the GCD guarantees that only
> -  // one Attribute bit is set at a time, so we can safely use a switch statement
> -  switch (Attributes) {
> -    case EFI_MEMORY_UC:
> -      // modify cacheability attributes
> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> -      // map to strongly ordered
> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> -      break;
> -
> -    case EFI_MEMORY_WC:
> -      // modify cacheability attributes
> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> -      // map to normal non-cachable
> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> -      break;
> -
> -    case EFI_MEMORY_WT:
> -      // modify cacheability attributes
> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> -      // write through with no-allocate
> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> -      break;
> -
> -    case EFI_MEMORY_WB:
> -      // modify cacheability attributes
> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> -      // write back (with allocate)
> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> -      break;
> +  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;
> +  if ((Attributes & EFI_MEMORY_XP) != 0) {
> +    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;
> +  } else {
> +    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
> +  }
>  
> -    case EFI_MEMORY_WP:
> -    case EFI_MEMORY_XP:
> -    case EFI_MEMORY_UCE:
> -      // cannot be implemented UEFI definition unclear for ARM
> -      // Cause a page fault if these ranges are accessed.
> -      EntryValue = TT_DESCRIPTOR_PAGE_TYPE_FAULT;
> -      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting page %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
> -      break;
> +  // Although the PI spec is unclear on this, the GCD guarantees that only
> +  // one Attribute bit is set at a time, so the order of the conditionals below
> +  // is irrelevant. If no memory attribute is specified, we preserve whatever
> +  // memory type is set in the page tables, and update the permission attributes
> +  // only.
> +  if (Attributes & EFI_MEMORY_UC) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> +    // map to strongly ordered
> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> +  } else if (Attributes & EFI_MEMORY_WC) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> +    // map to normal non-cachable
> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> +  } else if (Attributes & EFI_MEMORY_WT) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> +    // write through with no-allocate
> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> +  } else if (Attributes & EFI_MEMORY_WB) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> +    // write back (with allocate)
> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> +  } else if (Attributes & CACHE_ATTRIBUTE_MASK) {
> +    // catch unsupported memory type attributes
> +    ASSERT (FALSE);
> +    return EFI_UNSUPPORTED;
> +  }
>  
> -    default:
> -      return EFI_UNSUPPORTED;
> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
> +  } else {
> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
>    }
>  
>    // Obtain page table base
> @@ -520,53 +525,49 @@ UpdateSectionEntries (
>    // EntryValue: values at bit positions specified by EntryMask
>  
>    // Make sure we handle a section range that is unmapped
> -  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK;
> +  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
> +              TT_DESCRIPTOR_SECTION_AP_MASK;
>    EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
>  
> -  // Although the PI spec is unclear on this the GCD guarantees that only
> -  // one Attribute bit is set at a time, so we can safely use a switch statement
> -  switch(Attributes) {
> -    case EFI_MEMORY_UC:
> -      // modify cacheability attributes
> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> -      // map to strongly ordered
> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> -      break;
> -
> -    case EFI_MEMORY_WC:
> -      // modify cacheability attributes
> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> -      // map to normal non-cachable
> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> -      break;
> -
> -    case EFI_MEMORY_WT:
> -      // modify cacheability attributes
> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> -      // write through with no-allocate
> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> -      break;
> -
> -    case EFI_MEMORY_WB:
> -      // modify cacheability attributes
> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> -      // write back (with allocate)
> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> -      break;
> -
> -    case EFI_MEMORY_WP:
> -    case EFI_MEMORY_XP:
> -    case EFI_MEMORY_RP:
> -    case EFI_MEMORY_UCE:
> -      // cannot be implemented UEFI definition unclear for ARM
> -      // Cause a page fault if these ranges are accessed.
> -      EntryValue = TT_DESCRIPTOR_SECTION_TYPE_FAULT;
> -      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting section %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
> -      break;
> +  // Although the PI spec is unclear on this, the GCD guarantees that only
> +  // one Attribute bit is set at a time, so the order of the conditionals below
> +  // is irrelevant. If no memory attribute is specified, we preserve whatever
> +  // memory type is set in the page tables, and update the permission attributes
> +  // only.
> +  if (Attributes & EFI_MEMORY_UC) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> +    // map to strongly ordered
> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> +  } else if (Attributes & EFI_MEMORY_WC) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> +    // map to normal non-cachable
> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> +  } else if (Attributes & EFI_MEMORY_WT) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> +    // write through with no-allocate
> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> +  } else if (Attributes & EFI_MEMORY_WB) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> +    // write back (with allocate)
> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> +  } else if (Attributes & CACHE_ATTRIBUTE_MASK) {
> +    // catch unsupported memory type attributes
> +    ASSERT (FALSE);
> +    return EFI_UNSUPPORTED;
> +  }
>  
> +  if (Attributes & EFI_MEMORY_RO) {
> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
> +  } else {
> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
> +  }
>  
> -    default:
> -      return EFI_UNSUPPORTED;
> +  if (Attributes & EFI_MEMORY_XP) {
> +    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
>    }
>  
>    // obtain page table base
> @@ -697,13 +698,6 @@ SetMemoryAttributes (
>      return EFI_SUCCESS;
>    }
>  
> -  //
> -  // Ignore invocations that only modify permission bits
> -  //
> -  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
> -    return EFI_SUCCESS;
> -  }
> -
>    FlushTlbs = FALSE;
>    while (Length > 0) {
>      if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
> -- 
> 2.7.4
> 

[PATCH v3 4/4] ArmVirtPkg: enable PE/COFF image and memory protection for ARM platforms

[Ard Biesheuvel]

Like for AARCH64, enable PE/COFF image and NX memory protection for all
32-bit ARM virt platforms.

Note that this does not [yet] protect EfiLoaderData regions, due to
compatibility issues with GRUB.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/ArmVirt.dsc.inc | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index a91b27f13cf2..acfb71d3ff6c 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -18,7 +18,7 @@ [Defines]
   DEFINE TTY_TERMINAL            = FALSE
 
 [BuildOptions.common.EDKII.DXE_DRIVER,BuildOptions.common.EDKII.UEFI_DRIVER,BuildOptions.common.EDKII.UEFI_APPLICATION]
-  GCC:*_*_AARCH64_DLINK_FLAGS = -z common-page-size=0x1000
+  GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
 
 [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
   GCC:*_*_ARM_DLINK_FLAGS = -z common-page-size=0x1000
@@ -373,10 +373,6 @@ [PcdsFixedAtBuild.common]
   gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x80, 0x6d, 0x91, 0x7d, 0xb1, 0x5b, 0x8c, 0x45, 0xa4, 0x8f, 0xe2, 0x5f, 0xdd, 0x51, 0xef, 0x94}
 !endif
 
-[PcdsFixedAtBuild.ARM]
-  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
-
-[PcdsFixedAtBuild.AARCH64]
   #
   # Enable strict image permissions for all images. (This applies
   # only to images that were built with >= 4 KB section alignment.)
@@ -390,6 +386,9 @@ [PcdsFixedAtBuild.AARCH64]
   #
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
 
+[PcdsFixedAtBuild.ARM]
+  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
+
 [Components.common]
   #
   # Networking stack
-- 
2.7.4