Re: SetMemorySpaceAttributes with EFI_MEMORY_XP

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

On 20 March 2017 at 11:16, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
> Ard, why is SetMSetMemorySpaceAttributes being called in first place?
> (ignoring the recent NX patch)
> Looking at the initial GCD, it looks like unused memory usually
> doesn't have any attributes set anyway.
>

Originally, we added the new memory with
EFI_MEMORY_WB|EFI_MEMORY_WT|EFI_MEMORY_WC|EFI_MEMORY_UC capabilities,
and selected the EFI_MEMORY_WB attribute with the call to
gDS->SetMemorySpaceAttributes. Later, we removed all capabilities
expect EFI_MEMORY_WB, since the other ones cannot be supported under
virtualization with KVM.

Whether that makes the SetMemorySpaceAttributes redundant is not
entirely clear to me, but it does appear the adding the memory does
the right thing wrt non-exec permissions if the policy is enabled. So
perhaps we can simply drop this call?


> On Mon, Mar 20, 2017 at 12:04 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 20 March 2017 at 10:32, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
>>> Hi,
>>>
>>> I didn't test ArmVirtQemuKernel but I'm trying to use some of the code
>>> for another platform.
>>> So does this call ever succeed with PcdDxeNxMemoryProtectionPolicy
>>> being enabled?
>>> https://github.com/tianocore/edk2/blob/76874be3d411bf8daac051718e20932e0bf97d70/ArmVirtPkg/HighMemDxe/HighMemDxe.c#L95
>>> Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);
>>>
>>> Neither the memory that was added by this Dxe nor the one added
>>> automatically by GCD has the EFI_MEMORY_XP capability which causes
>>> SetMemorySpaceAttributes to return EFI_UNSUPPORTED.
>>>
>>
>> That is a very good point. I have been caught by this more than once
>> already (and I did test this, but not as thoroughly as I should have,
>> apparently)
>>
>> This is caused by the unfortunate situation in EDK2 that GCD
>> permission attributes are ambiguous: it does not distinguish between
>> 'the memory controller allows this range to be configured as
>> non-executable' and 'the nature of the contents of this memory region
>> allows it to be mapped without executable attributes', and therefore,
>> RO/XP are never used in the GCD memory space map.
>>
>> The solution is to use the CPU_ARCH_PROTOCOL interface explicitly to
>> set the XP attribute on the memory itself (but not on the descriptors
>> in the GCD or UEFI memory maps). I will spin a patch to fix this.
>>
>> Thanks,
>> Ard.