Re: [PATCH] ArmPkg/UncachedMemoryAllocationLib: set XP bit via CPU arch protocol

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Mar 14, 2017 at 07:58:15PM +0000, Ard Biesheuvel wrote:
> Commit e7b24ec9785d ("ArmPkg/UncachedMemoryAllocationLib: map uncached
> allocations non-executable") adds code that manipulates the GCD memory
> space attributes of a newly allocated uncached region without checking
> whether this region expose these attributes in its capabilities mask.
> 
> Given that the intent is to remove executable permissions from the region,
> this is a fairly pointless exercise to begin with, regardless of whether
> it is correct or not. The reason is that RO/XP memory attributes in the
> GCD memory space map or the UEFI memory map are completely disconnected
> from the actual mapping permissions used in the page tables.
> 
> So instead, invoke the CPU arch protocol directly, and add the non-exec
> attributes in the page tables directly.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c   | 36 ++++++++++++++++----
>  ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf |  8 ++++-
>  2 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> index b4fbfbcb362b..2530175bab7a 100644
> --- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> +++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c
> @@ -27,6 +27,10 @@
>  #include <Library/DxeServicesTableLib.h>
>  #include <Library/CacheMaintenanceLib.h>
>  
> +#include <Protocol/Cpu.h>
> +
> +STATIC EFI_CPU_ARCH_PROTOCOL    *mCpu;
> +
>  VOID *
>  UncachedInternalAllocatePages (
>    IN EFI_MEMORY_TYPE  MemoryType,
> @@ -150,15 +154,19 @@ AllocatePagesFromList (
>  
>    Status = gDS->GetMemorySpaceDescriptor (Memory, &Descriptor);
>    if (EFI_ERROR (Status)) {
> -    gBS->FreePages (Memory, Pages);
> -    return Status;
> +    goto FreePages;
>    }
>  
>    Status = gDS->SetMemorySpaceAttributes (Memory, EFI_PAGES_TO_SIZE (Pages),
> -                  EFI_MEMORY_WC | EFI_MEMORY_XP);
> +                  EFI_MEMORY_WC);
>    if (EFI_ERROR (Status)) {
> -    gBS->FreePages (Memory, Pages);
> -    return Status;
> +    goto FreePages;
> +  }
> +
> +  Status = mCpu->SetMemoryAttributes (mCpu, Memory, EFI_PAGES_TO_SIZE (Pages),
> +                   EFI_MEMORY_XP);

So, all other changes here are straightforward, but this one relies on
the fact that we don't care about EFI_MEMORY_RO being cleared when we
flip to _XP. As discussed on irc, if you can add a small note on that
(and wait for Ryan's Tested-by):

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> +  if (EFI_ERROR (Status)) {
> +    goto FreePages;
>    }
>  
>    InvalidateDataCacheRange ((VOID *)(UINTN)Memory, EFI_PAGES_TO_SIZE (Pages));
> @@ -166,8 +174,8 @@ AllocatePagesFromList (
>    NewNode = AllocatePool (sizeof (FREE_PAGE_NODE));
>    if (NewNode == NULL) {
>      ASSERT (FALSE);
> -    gBS->FreePages (Memory, Pages);
> -    return EFI_OUT_OF_RESOURCES;
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto FreePages;
>    }
>  
>    NewNode->Base       = Memory;
> @@ -181,6 +189,10 @@ AllocatePagesFromList (
>  
>    *Allocation = NewNode->Allocation;
>    return EFI_SUCCESS;
> +
> +FreePages:
> +  gBS->FreePages (Memory, Pages);
> +  return Status;
>  }
>  
>  /**
> @@ -238,6 +250,16 @@ FreePagesFromList (
>   */
>  EFI_STATUS
>  EFIAPI
> +UncachedMemoryAllocationLibConstructor (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  return gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&mCpu);
> +}
> +
> +EFI_STATUS
> +EFIAPI
>  UncachedMemoryAllocationLibDestructor (
>    IN EFI_HANDLE        ImageHandle,
>    IN EFI_SYSTEM_TABLE  *SystemTable
> diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf
> index d7a0f2f792a1..c637430c9020 100644
> --- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf
> +++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf
> @@ -22,7 +22,7 @@ [Defines]
>    MODULE_TYPE                    = DXE_DRIVER
>    VERSION_STRING                 = 1.0
>    LIBRARY_CLASS                  = UncachedMemoryAllocationLib
> -
> +  CONSTRUCTOR                    = UncachedMemoryAllocationLibConstructor
>    DESTRUCTOR                     = UncachedMemoryAllocationLibDestructor
>  
>  [Sources.common]
> @@ -42,3 +42,9 @@ [LibraryClasses]
>  
>  [Pcd]
>    gArmTokenSpaceGuid.PcdArmFreeUncachedMemorySizeThreshold
> +
> +[Protocols]
> +  gEfiCpuArchProtocolGuid
> +
> +[Depex]
> +  gEfiCpuArchProtocolGuid
> -- 
> 2.7.4
>