public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Taylor Beebe" <t@taylorbeebe.com>
To: devel@edk2.groups.io, ardb@kernel.org
Cc: Michael Kinney <michael.d.kinney@intel.com>,
	Liming Gao <gaoliming@byosoft.com.cn>,
	Jiewen Yao <jiewen.yao@intel.com>,
	Michael Kubacki <michael.kubacki@microsoft.com>,
	Sean Brogan <sean.brogan@microsoft.com>,
	Rebecca Cran <quic_rcran@quicinc.com>,
	Leif Lindholm <quic_llindhol@quicinc.com>,
	Sami Mujawar <sami.mujawar@arm.com>
Subject: Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
Date: Wed, 1 Feb 2023 10:41:03 -0800	[thread overview]
Message-ID: <6d3ca765-a196-a2ca-8b42-21a9bf019ae6@taylorbeebe.com> (raw)
In-Reply-To: <20230131223550.1775834-5-ardb@kernel.org>

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

  reply	other threads:[~2023-02-01 18:41 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-31 22:35 [PATCH 0/4] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
2023-01-31 22:35 ` [PATCH 1/4] MdePkg: Add Memory Attribute Protocol definition Ard Biesheuvel
2023-02-02  3:19   ` 回复: " gaoliming
2023-02-02  9:25     ` [edk2-devel] " Ard Biesheuvel
2023-01-31 22:35 ` [PATCH 2/4] MdePkg: Bump implemented UEFI version to v2.10 Ard Biesheuvel
2023-02-01  0:10   ` Michael D Kinney
2023-02-01  7:54     ` Ard Biesheuvel
2023-01-31 22:35 ` [PATCH 3/4] ArmPkg/CpuDxe: Unify PageAttributeToGcdAttribute helper Ard Biesheuvel
2023-01-31 22:35 ` [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol Ard Biesheuvel
2023-02-01 18:41   ` Taylor Beebe [this message]
2023-02-02  9:43     ` [edk2-devel] " Ard Biesheuvel
2023-02-03 19:08       ` Taylor Beebe
2023-02-03 19:58         ` Marvin Häuser
2023-02-07  1:18           ` Taylor Beebe
2023-02-07  8:29             ` Ard Biesheuvel
2023-02-07  8:56               ` Marvin Häuser
2023-02-07  9:16                 ` Ard Biesheuvel
2023-02-07 10:00                   ` Marvin Häuser
2023-02-07 10:01                   ` Ard Biesheuvel
2023-02-07 10:13                     ` Marvin Häuser
2023-02-07 17:56                       ` Ard Biesheuvel
2023-02-07 18:19                         ` Taylor Beebe
2023-02-07 18:50                           ` Marvin Häuser
2023-02-07 18:19                         ` Marvin Häuser

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6d3ca765-a196-a2ca-8b42-21a9bf019ae6@taylorbeebe.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox