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

[Jan Beulich <jbeulich@suse.com>]

On 02.05.2022 09:11, Demi Marie Obenour wrote:
> On Mon, May 02, 2022 at 08:24:30AM +0200, Jan Beulich wrote:
>> 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.
> 
> I will make one at some point.
> 
>>>> 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.
> 
> Then it is good that none is necessary :)
> 
> Also, should the various bug checks I added be replaced by ASSERT()?

You mean those in the earlier patch(es)? Not sure - depends on what you
would be doing for release builds. In the cases where you simply re-
check what was checked earlier on, ASSERT() would probably indeed be
preferable over BUG_ON() (and there I wouldn't even see a strong need
to consider alternatives for release builds).

Jan