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?
> 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 (#99444): https://edk2.groups.io/g/devel/message/99444
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]
-=-=-=-=-=-=-=-=-=-=-=-
