Hey Ard,

Have you encountered complications which stem from the lack of pre-allocated page table memory on ARM devices utilizing the memory protection policy?

My observation is the call stack can end up something like:

1. MemoryAttributeProtocol->SetMemoryAttributes(EFI_MEMORY_RO)
2. ArmSetMemoryRegionReadOnly ()
3. SetMemoryRegionAttribute()
4. UpdateRegionMapping()
5. UpdateRegionMappingRecursive()
6. AllocatePages() -> Need memory for a translation table entry
7. CoreAllocatePages()
8. ApplyMemoryProtectionPolicy() -> Policy says new page should be XN
9. gCpu->SetMemoryAttributes()
10. Back to 3 and loop infinitely

Thanks!
-Taylor

On 1/31/2023 2:35 PM, Ard Biesheuvel wrote:
Expose the protocol introduced in v2.10 that permits the caller to
manage mapping permissions in the page tables.

Signed-off-by: Ard Biesheuvel <a...@kernel.org>
---
  ArmPkg/Drivers/CpuDxe/CpuDxe.c          |   2 +
  ArmPkg/Drivers/CpuDxe/CpuDxe.h          |   3 +
  ArmPkg/Drivers/CpuDxe/CpuDxe.inf        |   2 +
  ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 242 ++++++++++++++++++++
  4 files changed, 249 insertions(+)

diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
index e6742f0a25fc..d04958e79e52 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
@@ -244,6 +244,8 @@ CpuDxeInitialize (
                    &mCpuHandle,

                    &gEfiCpuArchProtocolGuid,

                    &mCpu,

+                  &gEfiMemoryAttributeProtocolGuid,

+                  &mMemoryAttribute,

                    NULL

                    );

diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index 8933fa90c4ed..f45d2bc101a3 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -30,9 +30,12 @@
  #include <Protocol/Cpu.h>

  #include <Protocol/DebugSupport.h>

  #include <Protocol/LoadedImage.h>

+#include <Protocol/MemoryAttribute.h>

  extern BOOLEAN  mIsFlushingGCD;

+extern EFI_MEMORY_ATTRIBUTE_PROTOCOL  mMemoryAttribute;

+

  /**

    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/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
index 10792b393fc8..e732e21cb94a 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
@@ -23,6 +23,7 @@ [Sources.Common]
    CpuDxe.h

    CpuMmuCommon.c

    Exception.c

+  MemoryAttribute.c

  [Sources.ARM]

    Arm/Mmu.c

@@ -53,6 +54,7 @@ [LibraryClasses]
  [Protocols]

    gEfiCpuArchProtocolGuid

+  gEfiMemoryAttributeProtocolGuid

  [Guids]

    gEfiDebugImageInfoTableGuid

diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c 
b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
new file mode 100644
index 000000000000..9aeb83fff1b9
--- /dev/null
+++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
@@ -0,0 +1,242 @@
+/** @file

+

+  Copyright (c) 2023, Google LLC. All rights reserved.

+

+  SPDX-License-Identifier: BSD-2-Clause-Patent

+

+**/

+

+#include "CpuDxe.h"

+

+/**

+  This function retrieves the attributes of the memory region specified by

+  BaseAddress and Length. If different attributes are got from different part

+  of the memory region, EFI_NO_MAPPING will be returned.

+

+  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.

+  @param  BaseAddress       The physical address that is the start address of

+                            a memory region.

+  @param  Length            The size in bytes of the memory region.

+  @param  Attributes        Pointer to attributes returned.

+

+  @retval EFI_SUCCESS           The attributes got for the memory region.

+  @retval EFI_INVALID_PARAMETER Length is zero.

+                                Attributes is NULL.

+  @retval EFI_NO_MAPPING        Attributes are not consistent cross the memory

+                                region.

+  @retval EFI_UNSUPPORTED       The processor does not support one or more

+                                bytes of the memory resource range specified

+                                by BaseAddress and Length.

+

+**/

+STATIC

+EFI_STATUS

+GetMemoryAttributes (

+  IN  EFI_MEMORY_ATTRIBUTE_PROTOCOL       *This,

+  IN  EFI_PHYSICAL_ADDRESS                BaseAddress,

+  IN  UINT64                              Length,

+  OUT UINT64                              *Attributes

+  )

+{

+  UINTN       RegionAddress;

+  UINTN       RegionLength;

+  UINTN       RegionAttributes;

+  UINTN       Union;

+  UINTN       Intersection;

+  EFI_STATUS  Status;

+

+  if ((Length == 0) || (Attributes == NULL)) {

+    return EFI_INVALID_PARAMETER;

+  }

+

+  DEBUG ((DEBUG_INFO,

+          "%a: BaseAddress == 0x%lx, Length == 0x%lx\n",

+          __FUNCTION__,

+          (UINTN)BaseAddress,

+          (UINTN)Length

+          ));

+

+  Union         = 0;

+  Intersection  = MAX_UINTN;

+

+  for (RegionAddress = (UINTN)BaseAddress;

+       RegionAddress < (UINTN)(BaseAddress + Length);

+       RegionAddress += RegionLength) {

+

+    Status = GetMemoryRegion (&RegionAddress,

+                              &RegionLength,

+                              &RegionAttributes

+                              );

+

+    DEBUG ((DEBUG_INFO,

+            "%a: RegionAddress == 0x%lx, RegionLength == 0x%lx, RegionAttributes == 
0x%lx\n",

+            __FUNCTION__,

+            RegionAddress,

+            RegionLength,

+            RegionAttributes

+            ));

+

+    if (EFI_ERROR (Status)) {

+      return EFI_NO_MAPPING;

+    }

+

+    Union         |= RegionAttributes;

+    Intersection  &= RegionAttributes;

+  }

+

+  DEBUG ((DEBUG_INFO,

+          "%a: Union == %lx, Intersection == %lx\n",

+          __FUNCTION__,

+          Union,

+          Intersection

+          ));

+

+  if (Union != Intersection) {

+    return EFI_NO_MAPPING;

+  }

+

+  *Attributes = PageAttributeToGcdAttribute (Union) & (EFI_MEMORY_RO | 
EFI_MEMORY_XP);

+  return EFI_SUCCESS;

+}

+

+/**

+  This function set given attributes of the memory region specified by

+  BaseAddress and Length.

+

+  The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.

+

+  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.

+  @param  BaseAddress       The physical address that is the start address of

+                            a memory region.

+  @param  Length            The size in bytes of the memory region.

+  @param  Attributes        The bit mask of attributes to set for the memory

+                            region.

+

+  @retval EFI_SUCCESS           The attributes were set for the memory region.

+  @retval EFI_INVALID_PARAMETER Length is zero.

+                                Attributes specified an illegal combination of

+                                attributes that cannot be set together.

+  @retval EFI_UNSUPPORTED       The processor does not support one or more

+                                bytes of the memory resource range specified

+                                by BaseAddress and Length.

+                                The bit mask of attributes is not supported for

+                                the memory resource range specified by

+                                BaseAddress and Length.

+

+**/

+STATIC

+EFI_STATUS

+SetMemoryAttributes (

+  IN  EFI_MEMORY_ATTRIBUTE_PROTOCOL       *This,

+  IN  EFI_PHYSICAL_ADDRESS                BaseAddress,

+  IN  UINT64                              Length,

+  IN  UINT64                              Attributes

+  )

+{

+  EFI_STATUS  Status;

+

+  DEBUG ((DEBUG_INFO,

+          "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",

+          __FUNCTION__,

+          (UINTN)BaseAddress,

+          (UINTN)Length,

+          (UINTN)Attributes

+          ));

+

+  if ((Length == 0) ||

+      ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0)) {

+    return EFI_INVALID_PARAMETER;

+  }

+

+  if ((Attributes & EFI_MEMORY_RP) != 0) {

+    return EFI_UNSUPPORTED;

+  }

+

+  if ((Attributes & EFI_MEMORY_RO) != 0) {

+    Status = ArmSetMemoryRegionReadOnly (BaseAddress, Length);

+    if (EFI_ERROR (Status)) {

+      return EFI_UNSUPPORTED;

+    }

+  }

+

+  if ((Attributes & EFI_MEMORY_XP) != 0) {

+    Status = ArmSetMemoryRegionNoExec (BaseAddress, Length);

+    if (EFI_ERROR (Status)) {

+      return EFI_UNSUPPORTED;

+    }

+  }

+

+  return EFI_SUCCESS;

+}

+

+/**

+  This function clears given attributes of the memory region specified by

+  BaseAddress and Length.

+

+  The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.

+

+  @param  This              The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.

+  @param  BaseAddress       The physical address that is the start address of

+                            a memory region.

+  @param  Length            The size in bytes of the memory region.

+  @param  Attributes        The bit mask of attributes to clear for the memory

+                            region.

+

+  @retval EFI_SUCCESS           The attributes were cleared for the memory 
region.

+  @retval EFI_INVALID_PARAMETER Length is zero.

+                                Attributes specified an illegal combination of

+                                attributes that cannot be cleared together.

+  @retval EFI_UNSUPPORTED       The processor does not support one or more

+                                bytes of the memory resource range specified

+                                by BaseAddress and Length.

+                                The bit mask of attributes is not supported for

+                                the memory resource range specified by

+                                BaseAddress and Length.

+

+**/

+STATIC

+EFI_STATUS

+ClearMemoryAttributes (

+  IN  EFI_MEMORY_ATTRIBUTE_PROTOCOL       *This,

+  IN  EFI_PHYSICAL_ADDRESS                BaseAddress,

+  IN  UINT64                              Length,

+  IN  UINT64                              Attributes

+  )

+{

+  EFI_STATUS  Status;

+

+  DEBUG ((DEBUG_INFO,

+          "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",

+          __FUNCTION__,

+          (UINTN)BaseAddress,

+          (UINTN)Length,

+          (UINTN)Attributes

+          ));

+

+  if ((Length == 0) ||

+      ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0)) {

+    return EFI_INVALID_PARAMETER;

+  }

+

+  if ((Attributes & EFI_MEMORY_RO) != 0) {

+    Status = ArmClearMemoryRegionReadOnly (BaseAddress, Length);

+    if (EFI_ERROR (Status)) {

+      return EFI_UNSUPPORTED;

+    }

+  }

+

+  if ((Attributes & EFI_MEMORY_XP) != 0) {

+    Status = ArmClearMemoryRegionNoExec (BaseAddress, Length);

+    if (EFI_ERROR (Status)) {

+      return EFI_UNSUPPORTED;

+    }

+  }

+

+  return EFI_SUCCESS;

+}

+

+EFI_MEMORY_ATTRIBUTE_PROTOCOL  mMemoryAttribute = {

+  GetMemoryAttributes,

+  SetMemoryAttributes,

+  ClearMemoryAttributes

+};


--
Taylor Beebe
Software Engineer @ Microsoft


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99415): https://edk2.groups.io/g/devel/message/99415
Mute This Topic: https://groups.io/mt/96664071/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to