[PATCH 0/3] ArmPkg: some memory attribute lib followup changes

[PATCH 0/3] ArmPkg: some memory attribute lib followup changes

[Ard Biesheuvel]

A couple of tweaks for the memory attribute handling code to go on top
of the memory attribute PPI changes.

Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>

Ard Biesheuvel (3):
  StandaloneMmPkg: Include correct MmuLib header
  ArmPkg: Drop individual memory permission helpers
  ArmPkg/OpteeLib: Map shared communication buffer non-executable

 ArmPkg/Drivers/CpuDxe/CpuDxe.c                                                                      |   6 +-
 ArmPkg/Include/Library/ArmMmuLib.h                                                                  |  62 ---------
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c                                                    | 136 --------------------
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c                                                      | 104 ---------------
 ArmPkg/Library/OpteeLib/Optee.c                                                                     |   7 +-
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c                 |   2 +-
 StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c |   2 +-
 7 files changed, 12 insertions(+), 307 deletions(-)

-- 
2.39.2

[PATCH 1/3] StandaloneMmPkg: Include correct MmuLib header

[Ard Biesheuvel]

StandaloneMm has its own version of the ArmMmuLib library class, but
includes the ArmMmuLib header. This happens to work because the
prototypes that are referenced are the same, but this will no longer be
the case after a future patch. So correct the #includes.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c                 | 2 +-
 StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
index e78b2acacb5873cd..96de10405af829c6 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
@@ -15,13 +15,13 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Guid/MmramMemoryReserve.h>
 #include <Guid/MpInformation.h>
 
-#include <Library/ArmMmuLib.h>
 #include <Library/ArmSvcLib.h>
 #include <Library/DebugLib.h>
 #include <Library/HobLib.h>
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/SerialPortLib.h>
+#include <Library/StandaloneMmMmuLib.h>
 #include <Library/PcdLib.h>
 
 #include <IndustryStandard/ArmStdSmc.h>
diff --git a/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c b/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c
index 492252843e12ff69..7a5bb412d0d2d789 100644
--- a/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c
+++ b/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c
@@ -10,13 +10,13 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include <PiDxe.h>
 
-#include <Library/ArmMmuLib.h>
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
 #include <Library/PcdLib.h>
 #include <Library/PeCoffLib.h>
 #include <Library/PeCoffExtraActionLib.h>
+#include <Library/StandaloneMmMmuLib.h>
 
 typedef RETURN_STATUS (*REGION_PERMISSION_UPDATE_FUNC) (
   IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-- 
2.39.2

[PATCH 2/3] ArmPkg: Drop individual memory permission helpers

[Ard Biesheuvel]

Now that we have a sane API to set and clear memory permissions that
works the same on ARM and AArch64, we no longer have a need for the
individual set/clear no-access/read-only/no-exec helpers so let's drop
them.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Drivers/CpuDxe/CpuDxe.c                   |   6 +-
 ArmPkg/Include/Library/ArmMmuLib.h               |  62 ---------
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 136 --------------------
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c   | 104 ---------------
 4 files changed, 4 insertions(+), 304 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
index f820f3f621891365..fc63e527846acba9 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
@@ -288,9 +288,11 @@ RemapUnusedMemoryNx (
   MemoryMapEnd   = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + MemoryMapSize);
   while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
     if (MemoryMapEntry->Type == EfiConventionalMemory) {
-      ArmSetMemoryRegionNoExec (
+      ArmSetMemoryAttributes (
         MemoryMapEntry->PhysicalStart,
-        EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages)
+        EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages),
+        EFI_MEMORY_XP,
+        EFI_MEMORY_XP
         );
     }
 
diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h
index 91d112314fdf4859..2ce948e8db1d34e5 100644
--- a/ArmPkg/Include/Library/ArmMmuLib.h
+++ b/ArmPkg/Include/Library/ArmMmuLib.h
@@ -21,68 +21,6 @@ ArmConfigureMmu (
   OUT UINTN                         *TranslationTableSize  OPTIONAL
   );
 
-/**
-  Convert a region of memory to read-protected, by clearing the access flag.
-
-  @param  BaseAddress           The start of the region.
-  @param  Length                The size of the region.
-
-  @retval EFI_SUCCESS           The attributes were set successfully.
-  @retval EFI_OUT_OF_RESOURCES  The operation failed due to insufficient memory.
-
-**/
-EFI_STATUS
-EFIAPI
-ArmSetMemoryRegionNoAccess (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  );
-
-/**
-  Convert a region of memory to read-enabled, by setting the access flag.
-
-  @param  BaseAddress           The start of the region.
-  @param  Length                The size of the region.
-
-  @retval EFI_SUCCESS           The attributes were set successfully.
-  @retval EFI_OUT_OF_RESOURCES  The operation failed due to insufficient memory.
-
-**/
-EFI_STATUS
-EFIAPI
-ArmClearMemoryRegionNoAccess (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  );
-
-EFI_STATUS
-EFIAPI
-ArmSetMemoryRegionNoExec (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  );
-
-EFI_STATUS
-EFIAPI
-ArmClearMemoryRegionNoExec (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  );
-
-EFI_STATUS
-EFIAPI
-ArmSetMemoryRegionReadOnly (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  );
-
-EFI_STATUS
-EFIAPI
-ArmClearMemoryRegionReadOnly (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  );
-
 VOID
 EFIAPI
 ArmReplaceLiveTranslationEntry (
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 22623572b9cb931c..1e57e589977e6c39 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -552,142 +552,6 @@ ArmSetMemoryAttributes (
            );
 }
 
-STATIC
-EFI_STATUS
-SetMemoryRegionAttribute (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length,
-  IN  UINT64                Attributes,
-  IN  UINT64                BlockEntryMask
-  )
-{
-  return UpdateRegionMapping (
-           BaseAddress,
-           Length,
-           Attributes,
-           BlockEntryMask,
-           ArmGetTTBR0BaseAddress (),
-           TRUE
-           );
-}
-
-EFI_STATUS
-ArmSetMemoryRegionNoExec (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  )
-{
-  UINT64  Val;
-
-  if (ArmReadCurrentEL () == AARCH64_EL1) {
-    Val = TT_PXN_MASK | TT_UXN_MASK;
-  } else {
-    Val = TT_XN_MASK;
-  }
-
-  return SetMemoryRegionAttribute (
-           BaseAddress,
-           Length,
-           Val,
-           ~TT_ADDRESS_MASK_BLOCK_ENTRY
-           );
-}
-
-EFI_STATUS
-ArmClearMemoryRegionNoExec (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  )
-{
-  UINT64  Mask;
-
-  // XN maps to UXN in the EL1&0 translation regime
-  Mask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_PXN_MASK | TT_XN_MASK);
-
-  return SetMemoryRegionAttribute (
-           BaseAddress,
-           Length,
-           0,
-           Mask
-           );
-}
-
-/**
-  Convert a region of memory to read-protected, by clearing the access flag.
-
-  @param  BaseAddress           The start of the region.
-  @param  Length                The size of the region.
-
-  @retval EFI_SUCCESS           The attributes were set successfully.
-  @retval EFI_OUT_OF_RESOURCES  The operation failed due to insufficient memory.
-
-**/
-EFI_STATUS
-ArmSetMemoryRegionNoAccess (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  )
-{
-  return SetMemoryRegionAttribute (
-           BaseAddress,
-           Length,
-           0,
-           ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AF)
-           );
-}
-
-/**
-  Convert a region of memory to read-enabled, by setting the access flag.
-
-  @param  BaseAddress           The start of the region.
-  @param  Length                The size of the region.
-
-  @retval EFI_SUCCESS           The attributes were set successfully.
-  @retval EFI_OUT_OF_RESOURCES  The operation failed due to insufficient memory.
-
-**/
-EFI_STATUS
-ArmClearMemoryRegionNoAccess (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  )
-{
-  return SetMemoryRegionAttribute (
-           BaseAddress,
-           Length,
-           TT_AF,
-           ~TT_ADDRESS_MASK_BLOCK_ENTRY
-           );
-}
-
-EFI_STATUS
-ArmSetMemoryRegionReadOnly (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  )
-{
-  return SetMemoryRegionAttribute (
-           BaseAddress,
-           Length,
-           TT_AP_NO_RO,
-           ~TT_ADDRESS_MASK_BLOCK_ENTRY
-           );
-}
-
-EFI_STATUS
-ArmClearMemoryRegionReadOnly (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  )
-{
-  return SetMemoryRegionAttribute (
-           BaseAddress,
-           Length,
-           TT_AP_NO_RW,
-           ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK)
-           );
-}
-
 EFI_STATUS
 EFIAPI
 ArmConfigureMmu (
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
index 61405965a73eaeb8..548ee1303870782a 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -544,107 +544,3 @@ ArmSetMemoryAttributes (
            TtEntryMask
            );
 }
-
-EFI_STATUS
-ArmSetMemoryRegionNoExec (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  )
-{
-  return SetMemoryAttributes (
-           BaseAddress,
-           Length,
-           EFI_MEMORY_XP,
-           TT_DESCRIPTOR_SECTION_XN_MASK
-           );
-}
-
-EFI_STATUS
-ArmClearMemoryRegionNoExec (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  )
-{
-  return SetMemoryAttributes (
-           BaseAddress,
-           Length,
-           0,
-           TT_DESCRIPTOR_SECTION_XN_MASK
-           );
-}
-
-EFI_STATUS
-ArmSetMemoryRegionReadOnly (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  )
-{
-  return SetMemoryAttributes (
-           BaseAddress,
-           Length,
-           EFI_MEMORY_RO,
-           TT_DESCRIPTOR_SECTION_AP_MASK
-           );
-}
-
-EFI_STATUS
-ArmClearMemoryRegionReadOnly (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  )
-{
-  return SetMemoryAttributes (
-           BaseAddress,
-           Length,
-           0,
-           TT_DESCRIPTOR_SECTION_AP_MASK
-           );
-}
-
-/**
-  Convert a region of memory to read-protected, by clearing the access flag.
-
-  @param  BaseAddress           The start of the region.
-  @param  Length                The size of the region.
-
-  @retval EFI_SUCCESS           The attributes were set successfully.
-  @retval EFI_OUT_OF_RESOURCES  The operation failed due to insufficient memory.
-
-**/
-EFI_STATUS
-ArmSetMemoryRegionNoAccess (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  )
-{
-  return SetMemoryAttributes (
-           BaseAddress,
-           Length,
-           EFI_MEMORY_RP,
-           TT_DESCRIPTOR_SECTION_AF
-           );
-}
-
-/**
-  Convert a region of memory to read-enabled, by setting the access flag.
-
-  @param  BaseAddress           The start of the region.
-  @param  Length                The size of the region.
-
-  @retval EFI_SUCCESS           The attributes were set successfully.
-  @retval EFI_OUT_OF_RESOURCES  The operation failed due to insufficient memory.
-
-**/
-EFI_STATUS
-ArmClearMemoryRegionNoAccess (
-  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
-  IN  UINT64                Length
-  )
-{
-  return SetMemoryAttributes (
-           BaseAddress,
-           Length,
-           0,
-           TT_DESCRIPTOR_SECTION_AF
-           );
-}
-- 
2.39.2

[PATCH 3/3] ArmPkg/OpteeLib: Map shared communication buffer non-executable

[Ard Biesheuvel]

The OP-TEE secure OS exposes a non-secure memory region for
communication between the secure OS itself and any clients in the
non-secure firmware. This memory is writable by non-secure and is not
used for code only data, and so it should be mapped non-executable.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Library/OpteeLib/Optee.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
index 46464f17ef06653e..3acf172b68a2d34c 100644
--- a/ArmPkg/Library/OpteeLib/Optee.c
+++ b/ArmPkg/Library/OpteeLib/Optee.c
@@ -86,7 +86,12 @@ OpteeSharedMemoryRemap (
     return EFI_BUFFER_TOO_SMALL;
   }
 
-  Status = ArmSetMemoryAttributes (PhysicalAddress, Size, EFI_MEMORY_WB, 0);
+  Status = ArmSetMemoryAttributes (
+             PhysicalAddress,
+             Size,
+             EFI_MEMORY_WB | EFI_MEMORY_XP,
+             0
+             );
   if (EFI_ERROR (Status)) {
     return Status;
   }
-- 
2.39.2

Re: [PATCH 3/3] ArmPkg/OpteeLib: Map shared communication buffer non-executable

[Leif Lindholm]

On Mon, Jun 26, 2023 at 10:36:44 +0200, Ard Biesheuvel wrote:
> The OP-TEE secure OS exposes a non-secure memory region for
> communication between the secure OS itself and any clients in the
> non-secure firmware. This memory is writable by non-secure and is not
> used for code only data, and so it should be mapped non-executable.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

This looks straightforward enough (and an important safety
improvement), but adding Sumit on cc as original author.

>From my side, for the series:
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

> ---
>  ArmPkg/Library/OpteeLib/Optee.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
> index 46464f17ef06653e..3acf172b68a2d34c 100644
> --- a/ArmPkg/Library/OpteeLib/Optee.c
> +++ b/ArmPkg/Library/OpteeLib/Optee.c
> @@ -86,7 +86,12 @@ OpteeSharedMemoryRemap (
>      return EFI_BUFFER_TOO_SMALL;
>    }
>  
> -  Status = ArmSetMemoryAttributes (PhysicalAddress, Size, EFI_MEMORY_WB, 0);
> +  Status = ArmSetMemoryAttributes (
> +             PhysicalAddress,
> +             Size,
> +             EFI_MEMORY_WB | EFI_MEMORY_XP,
> +             0
> +             );
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> -- 
> 2.39.2
>