Re: SetMemorySpaceAttributes with EFI_MEMORY_XP

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

On 20 March 2017 at 15:24, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/20/17 15:08, Ard Biesheuvel wrote:
>> On 20 March 2017 at 11:38, Laszlo Ersek <lersek@redhat.com> wrote:
>>> 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?
>>>
>>
>> I think it may not map the memory at all in this case, so we need to
>> do something. It looks like calling mCpu->SetMemoryAttributes() should
>> be sufficient here, and so I wonder whether we violate anything by
>> replacing gDS->SetMemorySpaceAttributes with mCpu->SetMemoryAttributes
>> here.
>
> Earlier you mentioned that we cannot get some piece of information from
> the GCD map, which limits what we can do here.
>
> Looking at GetMemorySpaceDescriptor() and GetMemorySpaceMap(), they
> should return both Capabilities and Attributes.
>
> Also, looking at vol2 in PI 1.5, I find:
>
> * GetMemorySpaceDescriptor() returns EFI_NOT_AVAILABLE_YET if "The
> attributes cannot be set because CPU architectural protocol is not
> available yet."
>
> * 9.7.3.2 SetMemorySpaceAttributes() -- When the DXE Foundation is
> notified that the EFI_CPU_ARCH_PROTOCOL has been installed, then the DXE
> Service SetMemorySpaceAttributes() can be made available. The DXE
> Foundation can then use the SetMemoryAttributes() service of the
> EFI_CPU_ARCH_PROTOCOL to implement the DXE Service
> SetMemorySpaceAttributes().
>
> * 9.7.3.3 GetMemorySpaceMap() -- When the DXE Foundation is notified
> that the EFI_CPU_ARCH_PROTOCOL has been installed, then the DXE Service
> GetMemorySpaceMap() is fully functional. This function is made available
> when the memory-based services are initialized. However, the Attributes
> field of the array of EFI_GCD_MEMORY_SPACE_DESCRIPTORs is not valid
> until the EFI_CPU_ARCH_PROTOCOL is installed.
>

OK, that's the theory. The reality (as Jiewen confirms) is that the
RO/XP capability and attribute bits are never used on the GCD side,
because they may cause compatibility issues.

The problem is that UEFI memory map entries inherit all attributes
from the GCD memory space entries they are carved out of. This means
that all UEFI memory map entries will have RO and XP set, which is
clearly not what we want.

I guess it would be feasible to filter out RO and XP when calling
CoreAddRange(), but that does not happen currently.

> So, assuming that you have tested GetMemorySpaceMap() earlier, and have
> found Attributes useless for the protection (or other) purposes, may
> that have happened because the CPU arch protocol was not available yet?
>

No

> ... I guess my email is a bit confusing. My points are, (a) we should
> likely not mess directly with the CPU arch protocol if we know (and we
> do know) that the GCD services use them internally, (b) are we
> absolutely sure that the GCD services can't help us here?
>
> If nothing else works, I agree we can mess with the CPU arch protocol
> directly.
>

I'm afraid that is our only option.