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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 28 February 2017 at 21:14, Laszlo Ersek <lersek@redhat.com> wrote:
> 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".
>

I opted for this solution, given that the RHS is a constant < 32
(EfiConventionalMemory)

> With those fixed up:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

Pushed, thanks.