[PATCH v2 0/4] ArmPkg: add groundwork for DXE image protection

[PATCH v2 0/4] ArmPkg: add groundwork for DXE image protection

[Ard Biesheuvel]

The upcoming DXE image protection feature expects the EFI_CPU_ARCH_PROTOCOL
method SetMemoryAttributes() to deal with invocations that only modify
permission attributes, but leave the cacheability attributes alone. This
requires some groundwork to be performed in the MMU code for ARM.

Patch #1 is Jiewen's patch to retire EFI_MEMORY_WP, which is no longer
used as a permission attribute.

Patch #2 updates EfiAttributeToArmAttribute () so it can deal with
unspecified caching modes.

Patch #3 makes ARM deal with EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes()
calls that do not specify memory attributes. On ARM, we don't have code
that manages the permission bits in the page tables, so this does little
more than ignore such attributes.

Patch #4 implements the handling for AARCH64 to manage the permissions
bits without touching or caring about the memory type attributes.

Changes since v1:
- add Leif's and my R-b to #1
- add Leif's R-b to #3
- fix reference to TT_ATTR_INDX_MASK in commit log (#2)
- move rather than redefine EFI_MEMORY_CACHETYPE_MASK macro (#4)

Ard Biesheuvel (3):
  ArmPkg/CpuDxe: translate invalid memory types in
    EfiAttributeToArmAttribute
  ArmPkg/CpuDxe: ARM: ignore page table updates that only change
    permissions
  ArmPkg/ArmMmuLib: AARCH64: add support for modifying only permissions

Jiewen Yao (1):
  ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage

 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c              |  7 +-
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  | 24 ++---
 ArmPkg/Drivers/CpuDxe/CpuDxe.h                   |  8 --
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |  5 +-
 ArmPkg/Include/Library/ArmLib.h                  |  4 +
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 94 ++++++++++++++------
 6 files changed, 88 insertions(+), 54 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage

[Ard Biesheuvel]

From: Jiewen Yao <jiewen.yao@intel.com>

Current Arm CpuDxe driver uses EFI_MEMORY_WP for write protection,
according to UEFI spec, we should use EFI_MEMORY_RO for write protection.
The EFI_MEMORY_WP is the cache attribute instead of memory attribute.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c              |  3 ++-
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  | 14 ++++++--------
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |  5 +++--
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  3 ++-
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index d8bb41978066..15d5a8173233 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -3,6 +3,7 @@
 Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
 Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
 Portions copyright (c) 2011-2013, ARM Ltd. All rights reserved.<BR>
+Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
 
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
@@ -224,7 +225,7 @@ EfiAttributeToArmAttribute (
   ArmAttributes |= TT_AF;
 
   // Determine protection attributes
-  if (EfiAttributes & EFI_MEMORY_WP) {
+  if (EfiAttributes & EFI_MEMORY_RO) {
     ArmAttributes |= TT_AP_RO_RO;
   }
 
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 14fc22d7a59f..6dcfba69e879 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -3,6 +3,7 @@
 Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
 Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
 Portions copyright (c) 2013, ARM Ltd. All rights reserved.<BR>
+Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
 
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
@@ -62,7 +63,7 @@ SectionToGcdAttributes (
   // determine protection attributes
   switch(SectionAttributes & TT_DESCRIPTOR_SECTION_AP_MASK) {
     case TT_DESCRIPTOR_SECTION_AP_NO_NO: // no read, no write
-      //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP;
+      //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;
       break;
 
     case TT_DESCRIPTOR_SECTION_AP_RW_NO:
@@ -73,7 +74,7 @@ SectionToGcdAttributes (
     // read only cases map to write-protect
     case TT_DESCRIPTOR_SECTION_AP_RO_NO:
     case TT_DESCRIPTOR_SECTION_AP_RO_RO:
-      *GcdAttributes |= EFI_MEMORY_WP;
+      *GcdAttributes |= EFI_MEMORY_RO;
       break;
 
     default:
@@ -126,7 +127,7 @@ PageToGcdAttributes (
   // determine protection attributes
   switch(PageAttributes & TT_DESCRIPTOR_PAGE_AP_MASK) {
     case TT_DESCRIPTOR_PAGE_AP_NO_NO: // no read, no write
-      //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP;
+      //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;
       break;
 
     case TT_DESCRIPTOR_PAGE_AP_RW_NO:
@@ -137,7 +138,7 @@ PageToGcdAttributes (
     // read only cases map to write-protect
     case TT_DESCRIPTOR_PAGE_AP_RO_NO:
     case TT_DESCRIPTOR_PAGE_AP_RO_RO:
-      *GcdAttributes |= EFI_MEMORY_WP;
+      *GcdAttributes |= EFI_MEMORY_RO;
       break;
 
     default:
@@ -730,9 +731,6 @@ EfiAttributeToArmAttribute (
       ArmAttributes = 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:
     default:
       // Cannot be implemented UEFI definition unclear for ARM
@@ -743,7 +741,7 @@ EfiAttributeToArmAttribute (
   }
 
   // Determine protection attributes
-  if (EfiAttributes & EFI_MEMORY_WP) {
+  if (EfiAttributes & EFI_MEMORY_RO) {
     ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
   } else {
     ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
index 723604d1df96..54d9b0163331 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -1,6 +1,7 @@
 /** @file
 *
 *  Copyright (c) 2013, ARM Limited. All rights reserved.
+*  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD License
@@ -236,7 +237,7 @@ CpuConvertPagesToUncachedVirtualAddress (
   // be the PCI address. Code should always use the CPU address, and we will or in VirtualMask
   // to that address.
   //
-  Status = SetMemoryAttributes (Address, Length, EFI_MEMORY_WP, 0);
+  Status = SetMemoryAttributes (Address, Length, EFI_MEMORY_RO, 0);
   if (!EFI_ERROR (Status)) {
     Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_UC, VirtualMask);
   }
@@ -264,7 +265,7 @@ CpuReconvertPages (
   //
   // Unmap the aliased Address
   //
-  Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_WP, 0);
+  Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_RO, 0);
   if (!EFI_ERROR (Status)) {
     //
     // Restore atttributes
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 540069a59b2e..6aa970bc0514 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -3,6 +3,7 @@
 *
 *  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
 *  Copyright (c) 2016, Linaro Limited. All rights reserved.
+*  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD License
@@ -89,7 +90,7 @@ PageAttributeToGcdAttribute (
   // Determine protection attributes
   if (((PageAttributes & TT_AP_MASK) == TT_AP_NO_RO) || ((PageAttributes & TT_AP_MASK) == TT_AP_RO_RO)) {
     // Read only cases map to write-protect
-    GcdAttributes |= EFI_MEMORY_WP;
+    GcdAttributes |= EFI_MEMORY_RO;
   }
 
   // Process eXecute Never attribute
-- 
2.7.4

[PATCH v2 2/4] ArmPkg/CpuDxe: translate invalid memory types in EfiAttributeToArmAttribute

[Ard Biesheuvel]

The single user of EfiAttributeToArmAttribute () is the protocol
method EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(), which uses the
return value to compare against the ARM attributes of an existing mapping,
to infer whether it is actually necessary to change anything, or whether
the requested update is redundant. This saves some cache and TLB
maintenance on 32-bit ARM systems that use uncached translation tables.

However, EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() may be invoked with
only permission bits set, in which case the implied requested action is to
update the permissions of the region without modifying the cacheability
attributes. This is currently not possible, because
EfiAttributeToArmAttribute () ASSERT()s [on AArch64] on Attributes arguments
that lack a cacheability bit.

So let's simply return TT_ATTR_INDX_MASK (AArch64) or
TT_DESCRIPTOR_SECTION_TYPE_FAULT (ARM) in these cases (or'ed with the
appropriate permission bits). This way, the return value is equally
suitable for checking whether the attributes need to be modified, but
in a way that accommodates the use without a cacheability bit set.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 4 +---
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c     | 3 ---
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 15d5a8173233..7688846e70cb 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -216,9 +216,7 @@ EfiAttributeToArmAttribute (
     ArmAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK;
     break;
   default:
-    DEBUG ((EFI_D_ERROR, "EfiAttributeToArmAttribute: 0x%lX attributes is not supported.\n", EfiAttributes));
-    ASSERT (0);
-    ArmAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
+    ArmAttributes = TT_ATTR_INDX_MASK;
   }
 
   // Set the access flag to match the block attributes
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 6dcfba69e879..b6ba975b353a 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -733,10 +733,7 @@ EfiAttributeToArmAttribute (
 
     case EFI_MEMORY_UCE:
     default:
-      // Cannot be implemented UEFI definition unclear for ARM
-      // Cause a page fault if these ranges are accessed.
       ArmAttributes = TT_DESCRIPTOR_SECTION_TYPE_FAULT;
-      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): Unsupported attribute %x will page fault on access\n", EfiAttributes));
       break;
   }
 
-- 
2.7.4

[PATCH v2 3/4] ArmPkg/CpuDxe: ARM: ignore page table updates that only change permissions

[Ard Biesheuvel]

Currently, we have not implemented support on 32-bit ARM for managing
permission bits in the page tables. Since the new DXE page protection
for PE/COFF images may invoke EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes()
with only permission attributes set, let's simply ignore those for now.

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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index b6ba975b353a..89e429925ba9 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -680,6 +680,13 @@ SetMemoryAttributes (
 {
   EFI_STATUS    Status;
 
+  //
+  // Ignore invocations that only modify permission bits
+  //
+  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
+    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));
-- 
2.7.4

[PATCH v2 4/4] ArmPkg/ArmMmuLib: AARCH64: add support for modifying only permissions

[Ard Biesheuvel]

Since the new DXE page protection for PE/COFF images may invoke
EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() with only permission
attributes set, add support for this in the AARCH64 MMU code.

Move the EFI_MEMORY_CACHETYPE_MASK macro to a shared location between
CpuDxe and ArmMmuLib so we don't have to introduce yet another
definition.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/CpuDxe.h                   |  8 --
 ArmPkg/Include/Library/ArmLib.h                  |  4 +
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 91 ++++++++++++++------
 3 files changed, 67 insertions(+), 36 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index d16abe400ef3..80c305d53dd1 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -39,14 +39,6 @@
 #include <Protocol/LoadedImage.h>
 
 
-#define EFI_MEMORY_CACHETYPE_MASK     (EFI_MEMORY_UC  | \
-                                       EFI_MEMORY_WC  | \
-                                       EFI_MEMORY_WT  | \
-                                       EFI_MEMORY_WB  | \
-                                       EFI_MEMORY_UCE   \
-                                       )
-
-
 /**
   This function registers and enables the handler specified by InterruptHandler for a processor
   interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the
diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index 19501efa991f..24ffe9f1aaa7 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -26,6 +26,10 @@
  #error "Unknown chipset."
 #endif
 
+#define EFI_MEMORY_CACHETYPE_MASK   (EFI_MEMORY_UC | EFI_MEMORY_WC | \
+                                     EFI_MEMORY_WT | EFI_MEMORY_WB | \
+                                     EFI_MEMORY_UCE)
+
 /**
  * The UEFI firmware must not use the ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_* attributes.
  *
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 6aa970bc0514..9e0593ce598b 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -101,27 +101,6 @@ PageAttributeToGcdAttribute (
   return GcdAttributes;
 }
 
-ARM_MEMORY_REGION_ATTRIBUTES
-GcdAttributeToArmAttribute (
-  IN UINT64 GcdAttributes
-  )
-{
-  switch (GcdAttributes & 0xFF) {
-  case EFI_MEMORY_UC:
-    return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
-  case EFI_MEMORY_WC:
-    return ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
-  case EFI_MEMORY_WT:
-    return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH;
-  case EFI_MEMORY_WB:
-    return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
-  default:
-    DEBUG ((EFI_D_ERROR, "GcdAttributeToArmAttribute: 0x%lX attributes is not supported.\n", GcdAttributes));
-    ASSERT (0);
-    return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
-  }
-}
-
 #define MIN_T0SZ        16
 #define BITS_PER_LEVEL  9
 
@@ -425,6 +404,48 @@ FillTranslationTable (
            );
 }
 
+STATIC
+UINT64
+GcdAttributeToPageAttribute (
+  IN UINT64 GcdAttributes
+  )
+{
+  UINT64 PageAttributes;
+
+  switch (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) {
+  case EFI_MEMORY_UC:
+    PageAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
+    break;
+  case EFI_MEMORY_WC:
+    PageAttributes = TT_ATTR_INDX_MEMORY_NON_CACHEABLE;
+    break;
+  case EFI_MEMORY_WT:
+    PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_THROUGH | TT_SH_INNER_SHAREABLE;
+    break;
+  case EFI_MEMORY_WB:
+    PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE;
+    break;
+  default:
+    PageAttributes = TT_ATTR_INDX_MASK;
+    break;
+  }
+
+  if ((GcdAttributes & EFI_MEMORY_XP) != 0 ||
+      (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) == EFI_MEMORY_UC) {
+    if (ArmReadCurrentEL () == AARCH64_EL2) {
+      PageAttributes |= TT_XN_MASK;
+    } else {
+      PageAttributes |= TT_UXN_MASK | TT_PXN_MASK;
+    }
+  }
+
+  if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
+    PageAttributes |= TT_AP_RO_RO;
+  }
+
+  return PageAttributes | TT_AF;
+}
+
 RETURN_STATUS
 SetMemoryAttributes (
   IN EFI_PHYSICAL_ADDRESS      BaseAddress,
@@ -434,17 +455,31 @@ SetMemoryAttributes (
   )
 {
   RETURN_STATUS                Status;
-  ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;
   UINT64                      *TranslationTable;
-
-  MemoryRegion.PhysicalBase = BaseAddress;
-  MemoryRegion.VirtualBase = BaseAddress;
-  MemoryRegion.Length = Length;
-  MemoryRegion.Attributes = GcdAttributeToArmAttribute (Attributes);
+  UINT64                       PageAttributes;
+  UINT64                       PageAttributeMask;
+
+  PageAttributes = GcdAttributeToPageAttribute (Attributes);
+  PageAttributeMask = 0;
+
+  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
+    //
+    // No memory type was set in Attributes, so we are going to update the
+    // permissions only.
+    //
+    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK;
+    PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |
+                          TT_PXN_MASK | TT_XN_MASK);
+  }
 
   TranslationTable = ArmGetTTBR0BaseAddress ();
 
-  Status = FillTranslationTable (TranslationTable, &MemoryRegion);
+  Status = UpdateRegionMapping (
+             TranslationTable,
+             BaseAddress,
+             Length,
+             PageAttributes,
+             PageAttributeMask);
   if (RETURN_ERROR (Status)) {
     return Status;
   }
-- 
2.7.4

Re: [PATCH v2 0/4] ArmPkg: add groundwork for DXE image protection

[Ard Biesheuvel]

Hi Leif,

On 15 February 2017 at 17:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The upcoming DXE image protection feature expects the EFI_CPU_ARCH_PROTOCOL
> method SetMemoryAttributes() to deal with invocations that only modify
> permission attributes, but leave the cacheability attributes alone. This
> requires some groundwork to be performed in the MMU code for ARM.
>
> Patch #1 is Jiewen's patch to retire EFI_MEMORY_WP, which is no longer
> used as a permission attribute.
>
> Patch #2 updates EfiAttributeToArmAttribute () so it can deal with
> unspecified caching modes.
>
> Patch #3 makes ARM deal with EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes()
> calls that do not specify memory attributes. On ARM, we don't have code
> that manages the permission bits in the page tables, so this does little
> more than ignore such attributes.
>
> Patch #4 implements the handling for AARCH64 to manage the permissions
> bits without touching or caring about the memory type attributes.
>
> Changes since v1:
> - add Leif's and my R-b to #1
> - add Leif's R-b to #3
> - fix reference to TT_ATTR_INDX_MASK in commit log (#2)
> - move rather than redefine EFI_MEMORY_CACHETYPE_MASK macro (#4)
>

I'm aware that you have been off sick, so I'm sure you have quite the
todo list, but could you have a look this, please? Jiewen sent out the
next version of the DXE memory protection feature, which looks
finished to me, and I'd like to get this in first.

Cheers,

> Ard Biesheuvel (3):
>   ArmPkg/CpuDxe: translate invalid memory types in
>     EfiAttributeToArmAttribute
>   ArmPkg/CpuDxe: ARM: ignore page table updates that only change
>     permissions
>   ArmPkg/ArmMmuLib: AARCH64: add support for modifying only permissions
>
> Jiewen Yao (1):
>   ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage
>
>  ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c              |  7 +-
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  | 24 ++---
>  ArmPkg/Drivers/CpuDxe/CpuDxe.h                   |  8 --
>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |  5 +-
>  ArmPkg/Include/Library/ArmLib.h                  |  4 +
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 94 ++++++++++++++------
>  6 files changed, 88 insertions(+), 54 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCH v2 2/4] ArmPkg/CpuDxe: translate invalid memory types in EfiAttributeToArmAttribute

[Leif Lindholm]

On Wed, Feb 15, 2017 at 05:11:54PM +0000, Ard Biesheuvel wrote:
> The single user of EfiAttributeToArmAttribute () is the protocol
> method EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(), which uses the
> return value to compare against the ARM attributes of an existing mapping,
> to infer whether it is actually necessary to change anything, or whether
> the requested update is redundant. This saves some cache and TLB
> maintenance on 32-bit ARM systems that use uncached translation tables.
> 
> However, EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() may be invoked with
> only permission bits set, in which case the implied requested action is to
> update the permissions of the region without modifying the cacheability
> attributes. This is currently not possible, because
> EfiAttributeToArmAttribute () ASSERT()s [on AArch64] on Attributes arguments
> that lack a cacheability bit.
> 
> So let's simply return TT_ATTR_INDX_MASK (AArch64) or
> TT_DESCRIPTOR_SECTION_TYPE_FAULT (ARM) in these cases (or'ed with the
> appropriate permission bits). This way, the return value is equally
> suitable for checking whether the attributes need to be modified, but
> in a way that accommodates the use without a cacheability bit set.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

With this updated commit message:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 4 +---
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c     | 3 ---
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> index 15d5a8173233..7688846e70cb 100644
> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> @@ -216,9 +216,7 @@ EfiAttributeToArmAttribute (
>      ArmAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK;
>      break;
>    default:
> -    DEBUG ((EFI_D_ERROR, "EfiAttributeToArmAttribute: 0x%lX attributes is not supported.\n", EfiAttributes));
> -    ASSERT (0);
> -    ArmAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
> +    ArmAttributes = TT_ATTR_INDX_MASK;
>    }
>  
>    // Set the access flag to match the block attributes
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index 6dcfba69e879..b6ba975b353a 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -733,10 +733,7 @@ EfiAttributeToArmAttribute (
>  
>      case EFI_MEMORY_UCE:
>      default:
> -      // Cannot be implemented UEFI definition unclear for ARM
> -      // Cause a page fault if these ranges are accessed.
>        ArmAttributes = TT_DESCRIPTOR_SECTION_TYPE_FAULT;
> -      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): Unsupported attribute %x will page fault on access\n", EfiAttributes));
>        break;
>    }
>  
> -- 
> 2.7.4
> 

Re: [PATCH v2 4/4] ArmPkg/ArmMmuLib: AARCH64: add support for modifying only permissions

[Leif Lindholm]

On Wed, Feb 15, 2017 at 05:11:56PM +0000, Ard Biesheuvel wrote:
> Since the new DXE page protection for PE/COFF images may invoke
> EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() with only permission
> attributes set, add support for this in the AARCH64 MMU code.
> 
> Move the EFI_MEMORY_CACHETYPE_MASK macro to a shared location between
> CpuDxe and ArmMmuLib so we don't have to introduce yet another
> definition.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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

(This diff is also a lot less confusing :)

> ---
>  ArmPkg/Drivers/CpuDxe/CpuDxe.h                   |  8 --
>  ArmPkg/Include/Library/ArmLib.h                  |  4 +
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 91 ++++++++++++++------
>  3 files changed, 67 insertions(+), 36 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> index d16abe400ef3..80c305d53dd1 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> @@ -39,14 +39,6 @@
>  #include <Protocol/LoadedImage.h>
>  
>  
> -#define EFI_MEMORY_CACHETYPE_MASK     (EFI_MEMORY_UC  | \
> -                                       EFI_MEMORY_WC  | \
> -                                       EFI_MEMORY_WT  | \
> -                                       EFI_MEMORY_WB  | \
> -                                       EFI_MEMORY_UCE   \
> -                                       )
> -
> -
>  /**
>    This function registers and enables the handler specified by InterruptHandler for a processor
>    interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the
> diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
> index 19501efa991f..24ffe9f1aaa7 100644
> --- a/ArmPkg/Include/Library/ArmLib.h
> +++ b/ArmPkg/Include/Library/ArmLib.h
> @@ -26,6 +26,10 @@
>   #error "Unknown chipset."
>  #endif
>  
> +#define EFI_MEMORY_CACHETYPE_MASK   (EFI_MEMORY_UC | EFI_MEMORY_WC | \
> +                                     EFI_MEMORY_WT | EFI_MEMORY_WB | \
> +                                     EFI_MEMORY_UCE)
> +
>  /**
>   * The UEFI firmware must not use the ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_* attributes.
>   *
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 6aa970bc0514..9e0593ce598b 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -101,27 +101,6 @@ PageAttributeToGcdAttribute (
>    return GcdAttributes;
>  }
>  
> -ARM_MEMORY_REGION_ATTRIBUTES
> -GcdAttributeToArmAttribute (
> -  IN UINT64 GcdAttributes
> -  )
> -{
> -  switch (GcdAttributes & 0xFF) {
> -  case EFI_MEMORY_UC:
> -    return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> -  case EFI_MEMORY_WC:
> -    return ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> -  case EFI_MEMORY_WT:
> -    return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH;
> -  case EFI_MEMORY_WB:
> -    return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> -  default:
> -    DEBUG ((EFI_D_ERROR, "GcdAttributeToArmAttribute: 0x%lX attributes is not supported.\n", GcdAttributes));
> -    ASSERT (0);
> -    return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> -  }
> -}
> -
>  #define MIN_T0SZ        16
>  #define BITS_PER_LEVEL  9
>  
> @@ -425,6 +404,48 @@ FillTranslationTable (
>             );
>  }
>  
> +STATIC
> +UINT64
> +GcdAttributeToPageAttribute (
> +  IN UINT64 GcdAttributes
> +  )
> +{
> +  UINT64 PageAttributes;
> +
> +  switch (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) {
> +  case EFI_MEMORY_UC:
> +    PageAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
> +    break;
> +  case EFI_MEMORY_WC:
> +    PageAttributes = TT_ATTR_INDX_MEMORY_NON_CACHEABLE;
> +    break;
> +  case EFI_MEMORY_WT:
> +    PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_THROUGH | TT_SH_INNER_SHAREABLE;
> +    break;
> +  case EFI_MEMORY_WB:
> +    PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE;
> +    break;
> +  default:
> +    PageAttributes = TT_ATTR_INDX_MASK;
> +    break;
> +  }
> +
> +  if ((GcdAttributes & EFI_MEMORY_XP) != 0 ||
> +      (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) == EFI_MEMORY_UC) {
> +    if (ArmReadCurrentEL () == AARCH64_EL2) {
> +      PageAttributes |= TT_XN_MASK;
> +    } else {
> +      PageAttributes |= TT_UXN_MASK | TT_PXN_MASK;
> +    }
> +  }
> +
> +  if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> +    PageAttributes |= TT_AP_RO_RO;
> +  }
> +
> +  return PageAttributes | TT_AF;
> +}
> +
>  RETURN_STATUS
>  SetMemoryAttributes (
>    IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> @@ -434,17 +455,31 @@ SetMemoryAttributes (
>    )
>  {
>    RETURN_STATUS                Status;
> -  ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;
>    UINT64                      *TranslationTable;
> -
> -  MemoryRegion.PhysicalBase = BaseAddress;
> -  MemoryRegion.VirtualBase = BaseAddress;
> -  MemoryRegion.Length = Length;
> -  MemoryRegion.Attributes = GcdAttributeToArmAttribute (Attributes);
> +  UINT64                       PageAttributes;
> +  UINT64                       PageAttributeMask;
> +
> +  PageAttributes = GcdAttributeToPageAttribute (Attributes);
> +  PageAttributeMask = 0;
> +
> +  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
> +    //
> +    // No memory type was set in Attributes, so we are going to update the
> +    // permissions only.
> +    //
> +    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK;
> +    PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |
> +                          TT_PXN_MASK | TT_XN_MASK);
> +  }
>  
>    TranslationTable = ArmGetTTBR0BaseAddress ();
>  
> -  Status = FillTranslationTable (TranslationTable, &MemoryRegion);
> +  Status = UpdateRegionMapping (
> +             TranslationTable,
> +             BaseAddress,
> +             Length,
> +             PageAttributes,
> +             PageAttributeMask);
>    if (RETURN_ERROR (Status)) {
>      return Status;
>    }
> -- 
> 2.7.4
>