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

[Ryan Harkin <ryan.harkin@linaro.org>]

Hi Ard,

Thanks for the quick turnaround as always.

On 14 March 2017 at 20:20, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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>

Tested-by: Ryan Harkin <ryan.harkin@linaro.org>

FVPs and TC2 worked before this change, Juno was hanging on boot as
per an earlier email thread.

I tested this change on FVP Foundation & AEMv8, TC2 and Juno R0/1/2
and they all work fine after it's applied to the current HEAD of EDK2
(commit 8057622).


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