Re: [PATCH] ArmVirtPkg/HighMemDxe: preserve non-exec permissions on newly added regions

[Laszlo Ersek <lersek@redhat.com>]

On 02/28/17 14:56, Ard Biesheuvel wrote:
> Using DxeServices::SetMemorySpaceAttributes to set cacheability
> attributes has the side effect of stripping permission attributes,
> given that those are bits in the same bitfield, and so setting the
> Attributes argument to EFI_MEMORY_WB implies not setting EFI_MEMORY_XP
> or EFI_MEMORY_RO attributes.
> 
> In fact, the situation is even worse, given that the descriptor returned
> by DxeServices::GetMemorySpaceDescriptor does not reflect the permission
> attributes that may have been set by the preceding call to
> DxeServices::AddMemorySpace if PcdDxeNxMemoryProtectionPolicy has been
> configured to map EfiConventionalMemory with non-executable permissions.
> 
> Note that this applies equally to the non-executable stack and to PE/COFF
> sections that may have been mapped with R-X or RW- permissions. This is
> due to the ambiguity in the meaning of the EFI_MEMORY_RO/EFI_MEMORY_XP
> attributes when used in the GCD memory map, i.e., between signifying
> that an underlying RAM region has the controls to be configures as

s/configures/configured/

> read-only or non-executable, and signifying that the contents of a
> certain UEFI memory region allow them to be mapped with certain
> restricted permissions.
> 
> So let's check the policy in PcdDxeNxMemoryProtectionPolicy directly,
> and set the EFI_MEMORY_XP attribute if appropriate for
> EfiConventionalMemory regions.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 18 ++++++++++++++++--
>  ArmVirtPkg/HighMemDxe/HighMemDxe.inf |  1 +
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> index 22f738279b20..853660437cb0 100644
> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> @@ -37,6 +37,7 @@ InitializeHighMemDxe (
>    UINTN                 AddressCells, SizeCells;
>    UINT64                CurBase;
>    UINT64                CurSize;
> +  UINT64                Attributes;
>  
>    Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
>                    (VOID **)&FdtClient);
> @@ -77,8 +78,21 @@ InitializeHighMemDxe (
>            continue;
>          }
>  
> -        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize,
> -                        EFI_MEMORY_WB);
> +        //
> +        // Take care not to strip any permission attributes that will have been
> +        // set by DxeCore on the region we just added if a strict permission
> +        // policy is in effect for EfiConventionalMemory regions.
> +        // Unfortunately, we cannot interrogate the GCD memory space map for
> +        // those permissions, since they are not recorded there (for historical
> +        // reasons), so check the policy directly.
> +        //
> +        Attributes = EFI_MEMORY_WB;
> +        if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &
> +             (1UL << (UINT32)EfiConventionalMemory)) != 0) {

I think you should be using 1ULL here as constant, for consistency, as
in ARM and (somewhat uselessly here) in Ia32 builds, 1UL won't have type
UINT64 (= unsigned long long).

But then again, 1ULL cannot be portably shifted with << (we can only do
that up to UINTN), so ultimately I suggest LShiftU64() here.

Or else, if we're sure that it's going to fit in a UINT32, use "1U".

With those fixed up:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

> +          Attributes |= EFI_MEMORY_XP;
> +        }
> +
> +        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);
>  
>          if (EFI_ERROR (Status)) {
>            DEBUG ((EFI_D_ERROR,
> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
> index 3661cfd8c80c..89c743ebe058 100644
> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
> @@ -45,6 +45,7 @@ [Protocols]
>  
>  [Pcd]
>    gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy
>  
>  [Depex]
>    gEfiCpuArchProtocolGuid AND gFdtClientProtocolGuid
>