Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol

["Taylor Beebe" <t@taylorbeebe.com>]

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 <ardb@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