Re: SetMemorySpaceAttributes with EFI_MEMORY_XP

[Laszlo Ersek <lersek@redhat.com>]

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.

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?

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

Thanks
Laszlo