Re: SetMemorySpaceAttributes with EFI_MEMORY_XP

[Laszlo Ersek <lersek@redhat.com>]

On 03/20/17 12:20, Ard Biesheuvel wrote:
> 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?

Won't that turn off caching for the memory just added?

Thanks
Laszlo

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