Re: [PATCH v3 3/4] Add a new hypercall to get the ESRT

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.04.2022 19:06, Demi Marie Obenour wrote:
> On Fri, Apr 29, 2022 at 10:40:42AM +0200, Jan Beulich wrote:
>> On 29.04.2022 00:54, Demi Marie Obenour wrote:
>>> On Thu, Apr 28, 2022 at 08:47:49AM +0200, Jan Beulich wrote:
>>>> On 27.04.2022 21:08, Demi Marie Obenour wrote:
>>>>> On further thought, I think the hypercall approach is actually better
>>>>> than reserving the ESRT.  I really do not want XEN_FW_EFI_MEM_INFO to
>>>>> return anything other than the actual firmware-provided memory
>>>>> information, and the current approach seems to require more and more
>>>>> special-casing of the ESRT, not to mention potentially wasting memory
>>>>> and splitting a potentially large memory region into two smaller ones.
>>>>> By copying the entire ESRT into memory owned by Xen, the logic becomes
>>>>> significantly simpler on both the Xen and dom0 sides.
>>>>
>>>> I actually did consider the option of making a private copy when you did
>>>> send the initial version of this, but I'm not convinced this simplifies
>>>> things from a kernel perspective: They'd now need to discover the table
>>>> by some entirely different means. In Linux at least such divergence
>>>> "just for Xen" hasn't been liked in the past.
>>>>
>>>> There's also the question of how to propagate the information across
>>>> kexec. But I guess that question exists even outside of Xen, with the
>>>> area living in memory which the OS is expected to recycle.
>>>
>>> Indeed it does.  A simple rule might be, “Only trust the ESRT if it is
>>> in memory of type EfiRuntimeServicesData.”  That is easy to achieve by
>>> monkeypatching the config table as you suggested below.
>>>
>>> I *am* worried that the config table might be mapped read-only on some
>>> systems, in which case the overwrite would cause a fatal page fault.  Is
>>> there a way for Xen to check for this?
>>
>> While in boot mode, aiui page tables aren't supposed to be enforcing
>> access restrictions. Recall that on other architectures EFI even runs
>> with paging disabled; this simply is not possible for x86-64.
> 
> Yikes!  No wonder firmware has nonexistent exploit mitigations.  They
> really ought to start porting UEFI to Rust, with ASLR, NX, stack
> canaries, a hardened allocator, and support for de-priviliged services
> that run in user mode.
> 
> That reminds me: Can Xen itself run from ROM?

I guess that could be possible in principle, but would certainly require
some work.

>  Xen is being ported to
> POWER for use in Qubes OS, and one approach under consideration is to
> have Xen and a mini-dom0 be part of the firmware.  Personally, I really
> like this approach, as it makes untrusted storage domains much simpler.
> If this should be a separate email thread, let me know.

It probably should be.

>> So
>> portable firmware shouldn't map anything r/o. In principle the pointer
>> could still be in ROM; I consider this unlikely, but we could check
>> for that (just like we could do a page table walk to figure out
>> whether a r/o mapping would prevent us from updating the field).
> 
> Is there a utility function that could be used for this?

I don't think there is.

>>>  It could also be undefined behavior to modify it.
>>
>> That's the bigger worry I have.
> 
> Turns out that it is *not* undefined behavior, so long as
> ExitBootServices() has not been called.  This is becaues EFI drivers
> will modify the config table, so firmware cannot assume it to be
> read-only.

Ah, right - we could even use InstallConfigurationTable() ourselves
to make the adjustment.

>>>>> Is using ebmalloc() to allocate a copy of the ESRT a reasonable option?
>>>>
>>>> I'd suggest to try hard to avoid ebmalloc(). It ought to be possible to
>>>> make the copy before ExitBootServices(), via normal EFI allocation. If
>>>> replacing a pointer in the config table was okay(ish), this could even
>>>> be utilized to overcome the kexec problem.
>>>
>>> What type should I use for the allocation?  EfiLoaderData looks like the
>>> most consistent choice, but I am not sure if memory so allocated remains
>>> valid when Xen hands off to the OS, so EfiRuntimeServicesData might be a
>>> better choice.
>>
>> It definitely is. We do recycle EfiLoaderData ourselves.
> 
> I wonder why the ESRT was not in EfiRuntimeServicesData to begin with.

So do I.

>>>  To avoid memory leaks from repeated kexec(), this could
>>> be made conditional on the ESRT not being in memory of type
>>> EfiRuntimeServicesData to begin with.
>>
>> Of course - there's no point relocating the blob when it already is
>> immune to recycling.
> 
> Yup.  Is it reasonable for dom0 to check that the ESRT is in
> EfiRuntimeServicesData when under Xen?

I think it is, but kernel folks may not like Xen specific code in this
(or about any) area.

Jan