[RFC PATCH 01/10] ArmPkg/ArmMmuLib: Extend API to manage memory permissions better

["Ard Biesheuvel" <ardb@kernel.org>]

Currently, ArmSetMemoryAttributes () takes a combination of
EFI_MEMORY_xx constants describing the memory type and permission
attributes that should be set on a region of memory. In cases where the
memory type is omitted, we assume that the memory permissions being set
are final, and that existing memory permissions can be discarded.

This is problematic, because we aim to map memory non-executable
(EFI_MEMORY_XP) by default, and only relax this requirement for code
regions that are mapped read-only (EFI_MEMORY_RO). Currently, setting
one permission clears the other, and so code managing these permissions
has to be aware of the existing permissions in order to be able to
preserve them, and this is not always tractable (e.g., the UEFI memory
attribute protocol implements an abstraction that promises to preserve
memory permissions that it is not operating on explicitly).

So let's add an AttributeMask parameter to ArmSetMemoryAttributes(),
which is permitted to be non-zero if no memory type is being provided,
in which case only memory permission attributes covered in the mask will
be affected by the update.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |  2 +-
 ArmPkg/Include/Library/ArmMmuLib.h               | 36 +++++++-
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 52 +++++++++++-
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c   | 88 +++++++++++++++++---
 ArmPkg/Library/OpteeLib/Optee.c                  |  2 +-
 5 files changed, 165 insertions(+), 15 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
index 2e73719dce04ceb5..2d60c7d24dc05ee9 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -217,7 +217,7 @@ CpuSetMemoryAttributes (
   if (EFI_ERROR (Status) || (RegionArmAttributes != ArmAttributes) ||
       ((BaseAddress + Length) > (RegionBaseAddress + RegionLength)))
   {
-    return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes);
+    return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes, 0);
   } else {
     return EFI_SUCCESS;
   }
diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h
index 4cf59a1e376b123c..91d112314fdf4859 100644
--- a/ArmPkg/Include/Library/ArmMmuLib.h
+++ b/ArmPkg/Include/Library/ArmMmuLib.h
@@ -92,11 +92,45 @@ ArmReplaceLiveTranslationEntry (
   IN  BOOLEAN  DisableMmu
   );
 
+/**
+  Set the requested memory permission attributes on a region of memory.
+
+  BaseAddress and Length must be aligned to EFI_PAGE_SIZE.
+
+  If Attributes contains a memory type attribute (EFI_MEMORY_UC/WC/WT/WB), the
+  region is mapped according to this memory type, and additional memory
+  permission attributes (EFI_MEMORY_RP/RO/XP) are taken into account as well,
+  discarding any permission attributes that are currently set for the region.
+  AttributeMask is ignored in this case, and must be set to 0x0.
+
+  If Attributes contains only a combination of memory permission attributes
+  (EFI_MEMORY_RP/RO/XP), each page in the region will retain its existing
+  memory type, even if it is not uniformly set across the region. In this case,
+  AttributesMask may be set to a mask of permission attributes, and memory
+  permissions omitted from this mask will not be updated for any page in the
+  region. All attributes appearing in Attributes must appear in AttributeMask
+  as well. (Attributes & ~AttributeMask must produce 0x0)
+
+  @param[in]  BaseAddress     The physical address that is the start address of
+                              a memory region.
+  @param[in]  Length          The size in bytes of the memory region.
+  @param[in]  Attributes      Mask of memory attributes to set.
+  @param[in]  AttributeMask   Mask of memory attributes to take into account.
+
+  @retval EFI_SUCCESS           The attributes were set for the memory region.
+  @retval EFI_INVALID_PARAMETER BaseAddress or Length is not suitably aligned.
+                                Invalid combination of Attributes and
+                                AttributeMask.
+  @retval EFI_OUT_OF_RESOURCES  Requested attributes cannot be applied due to
+                                lack of system resources.
+
+**/
 EFI_STATUS
 ArmSetMemoryAttributes (
   IN EFI_PHYSICAL_ADDRESS  BaseAddress,
   IN UINT64                Length,
-  IN UINT64                Attributes
+  IN UINT64                Attributes,
+  IN UINT64                AttributeMask
   );
 
 #endif // ARM_MMU_LIB_H_
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 7ed758fbbc699732..22623572b9cb931c 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -469,11 +469,45 @@ GcdAttributeToPageAttribute (
   return PageAttributes;
 }
 
+/**
+  Set the requested memory permission attributes on a region of memory.
+
+  BaseAddress and Length must be aligned to EFI_PAGE_SIZE.
+
+  If Attributes contains a memory type attribute (EFI_MEMORY_UC/WC/WT/WB), the
+  region is mapped according to this memory type, and additional memory
+  permission attributes (EFI_MEMORY_RP/RO/XP) are taken into account as well,
+  discarding any permission attributes that are currently set for the region.
+  AttributeMask is ignored in this case, and must be set to 0x0.
+
+  If Attributes contains only a combination of memory permission attributes
+  (EFI_MEMORY_RP/RO/XP), each page in the region will retain its existing
+  memory type, even if it is not uniformly set across the region. In this case,
+  AttributesMask may be set to a mask of permission attributes, and memory
+  permissions omitted from this mask will not be updated for any page in the
+  region. All attributes appearing in Attributes must appear in AttributeMask
+  as well. (Attributes & ~AttributeMask must produce 0x0)
+
+  @param[in]  BaseAddress     The physical address that is the start address of
+                              a memory region.
+  @param[in]  Length          The size in bytes of the memory region.
+  @param[in]  Attributes      Mask of memory attributes to set.
+  @param[in]  AttributeMask   Mask of memory attributes to take into account.
+
+  @retval EFI_SUCCESS           The attributes were set for the memory region.
+  @retval EFI_INVALID_PARAMETER BaseAddress or Length is not suitably aligned.
+                                Invalid combination of Attributes and
+                                AttributeMask.
+  @retval EFI_OUT_OF_RESOURCES  Requested attributes cannot be applied due to
+                                lack of system resources.
+
+**/
 EFI_STATUS
 ArmSetMemoryAttributes (
   IN EFI_PHYSICAL_ADDRESS  BaseAddress,
   IN UINT64                Length,
-  IN UINT64                Attributes
+  IN UINT64                Attributes,
+  IN UINT64                AttributeMask
   )
 {
   UINT64  PageAttributes;
@@ -490,6 +524,22 @@ ArmSetMemoryAttributes (
     PageAttributes   &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF;
     PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |
                           TT_PXN_MASK | TT_XN_MASK | TT_AF);
+    if (AttributeMask != 0) {
+      if (((AttributeMask & ~(UINT64)(EFI_MEMORY_RP|EFI_MEMORY_RO|EFI_MEMORY_XP)) != 0) ||
+          ((Attributes & ~AttributeMask) != 0))
+      {
+        return EFI_INVALID_PARAMETER;
+      }
+
+      // Add attributes omitted from AttributeMask to the set of attributes to preserve
+      PageAttributeMask |= GcdAttributeToPageAttribute (~AttributeMask) &
+                           (TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF);
+    }
+  } else {
+    ASSERT (AttributeMask == 0);
+    if (AttributeMask != 0) {
+      return EFI_INVALID_PARAMETER;
+    }
   }
 
   return UpdateRegionMapping (
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
index 299d38ad07e85059..61405965a73eaeb8 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -10,6 +10,7 @@
 #include <Uefi.h>
 
 #include <Library/ArmLib.h>
+#include <Library/ArmMmuLib.h>
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
@@ -451,31 +452,96 @@ SetMemoryAttributes (
 }
 
 /**
-  Update the permission or memory type attributes on a range of memory.
+  Set the requested memory permission attributes on a region of memory.
 
-  @param  BaseAddress           The start of the region.
-  @param  Length                The size of the region.
-  @param  Attributes            A mask of EFI_MEMORY_xx constants.
+  BaseAddress and Length must be aligned to EFI_PAGE_SIZE.
 
-  @retval EFI_SUCCESS           The attributes were set successfully.
-  @retval EFI_OUT_OF_RESOURCES  The operation failed due to insufficient memory.
+  If Attributes contains a memory type attribute (EFI_MEMORY_UC/WC/WT/WB), the
+  region is mapped according to this memory type, and additional memory
+  permission attributes (EFI_MEMORY_RP/RO/XP) are taken into account as well,
+  discarding any permission attributes that are currently set for the region.
+  AttributeMask is ignored in this case, and must be set to 0x0.
+
+  If Attributes contains only a combination of memory permission attributes
+  (EFI_MEMORY_RP/RO/XP), each page in the region will retain its existing
+  memory type, even if it is not uniformly set across the region. In this case,
+  AttributesMask may be set to a mask of permission attributes, and memory
+  permissions omitted from this mask will not be updated for any page in the
+  region. All attributes appearing in Attributes must appear in AttributeMask
+  as well. (Attributes & ~AttributeMask must produce 0x0)
+
+  @param[in]  BaseAddress     The physical address that is the start address of
+                              a memory region.
+  @param[in]  Length          The size in bytes of the memory region.
+  @param[in]  Attributes      Mask of memory attributes to set.
+  @param[in]  AttributeMask   Mask of memory attributes to take into account.
+
+  @retval EFI_SUCCESS           The attributes were set for the memory region.
+  @retval EFI_INVALID_PARAMETER BaseAddress or Length is not suitably aligned.
+                                Invalid combination of Attributes and
+                                AttributeMask.
+  @retval EFI_OUT_OF_RESOURCES  Requested attributes cannot be applied due to
+                                lack of system resources.
 
 **/
 EFI_STATUS
 ArmSetMemoryAttributes (
   IN EFI_PHYSICAL_ADDRESS  BaseAddress,
   IN UINT64                Length,
-  IN UINT64                Attributes
+  IN UINT64                Attributes,
+  IN UINT64                AttributeMask
   )
 {
+  UINT32  TtEntryMask;
+
+  if (((BaseAddress | Length) & EFI_PAGE_MASK) != 0) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
+    //
+    // No memory type was set in Attributes, so we are going to update the
+    // permissions only.
+    //
+    if (AttributeMask != 0) {
+      if (((AttributeMask & ~(UINT64)(EFI_MEMORY_RP|EFI_MEMORY_RO|EFI_MEMORY_XP)) != 0) ||
+          ((Attributes & ~AttributeMask) != 0))
+      {
+        return EFI_INVALID_PARAMETER;
+      }
+    } else {
+      AttributeMask = EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP;
+    }
+
+    TtEntryMask = 0;
+    if ((AttributeMask & EFI_MEMORY_RP) != 0) {
+      TtEntryMask |= TT_DESCRIPTOR_SECTION_AF;
+    }
+
+    if ((AttributeMask & EFI_MEMORY_RO) != 0) {
+      TtEntryMask |= TT_DESCRIPTOR_SECTION_AP_MASK;
+    }
+
+    if ((AttributeMask & EFI_MEMORY_XP) != 0) {
+      TtEntryMask |= TT_DESCRIPTOR_SECTION_XN_MASK;
+    }
+  } else {
+    ASSERT (AttributeMask == 0);
+    if (AttributeMask != 0) {
+      return EFI_INVALID_PARAMETER;
+    }
+
+    TtEntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK |
+                  TT_DESCRIPTOR_SECTION_XN_MASK |
+                  TT_DESCRIPTOR_SECTION_AP_MASK |
+                  TT_DESCRIPTOR_SECTION_AF;
+  }
+
   return SetMemoryAttributes (
            BaseAddress,
            Length,
            Attributes,
-           TT_DESCRIPTOR_SECTION_TYPE_MASK |
-           TT_DESCRIPTOR_SECTION_XN_MASK |
-           TT_DESCRIPTOR_SECTION_AP_MASK |
-           TT_DESCRIPTOR_SECTION_AF
+           TtEntryMask
            );
 }
 
diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
index 48e33cb3d5ee4ab6..46464f17ef06653e 100644
--- a/ArmPkg/Library/OpteeLib/Optee.c
+++ b/ArmPkg/Library/OpteeLib/Optee.c
@@ -86,7 +86,7 @@ OpteeSharedMemoryRemap (
     return EFI_BUFFER_TOO_SMALL;
   }
 
-  Status = ArmSetMemoryAttributes (PhysicalAddress, Size, EFI_MEMORY_WB);
+  Status = ArmSetMemoryAttributes (PhysicalAddress, Size, EFI_MEMORY_WB, 0);
   if (EFI_ERROR (Status)) {
     return Status;
   }
-- 
2.39.2