On 2/2/2023 1:43 AM, Ard Biesheuvel wrote:
On Wed, 1 Feb 2023 at 19:41, Taylor Beebe <t...@taylorbeebe.com> wrote:

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?


Interesting. No I haven't, but I agree it is a potential concern.

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


So one thing that makes this scenario unlikely (I think) is that by
default, all unallocated space has the XP attribute set already, and
so allocating a page table with XP attributes should not result in the
need for a block entry split, and therefore no allocation for a next
level table. Perhaps we need some asserts to diagnose this at runtime?

Ah yes, you're right. On Project Mu, we make an explicit check to the attributes of the region in ApplyMemoryProtectionPolicy() to determine if the attributes need to be updated. We do this to account for the introduction of the memory attribute protocol and to mitigate a rare issue where an allocation might not have consistent attributes when allocating a type other than BootServicesData. This change to ApplyMemoryProtectionPolicy() means an infinite loop is possible during the memory protection initialization phase.


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






--
Taylor Beebe
Software Engineer @ Microsoft


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99616): https://edk2.groups.io/g/devel/message/99616
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