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

[Demi Marie Obenour <demi@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 5625 bytes --]

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 Wed, Apr 27, 2022 at 10:56:34AM +0200, Jan Beulich wrote:
> >>>> On 19.04.2022 17:49, Demi Marie Obenour wrote:
> >>>>> This hypercall can be used to get the ESRT from the hypervisor.  It
> >>>>> returning successfully also indicates that Xen has reserved the ESRT and
> >>>>> it can safely be parsed by dom0.
> >>>>
> >>>> I'm not convinced of the need, and I view such an addition as inconsistent
> >>>> with the original intentions. The pointer comes from the config table,
> >>>> which Dom0 already has access to. All a Dom0 kernel may need to know in
> >>>> addition is whether the range was properly reserved. This could be achieved
> >>>> by splitting the EFI memory map entry in patch 2, instead of only splitting
> >>>> the E820 derivation, as then XEN_FW_EFI_MEM_INFO can be used to find out
> >>>> the range's type. Another way to find out would be for Dom0 to attempt to
> >>>> map this area as MMIO, after first checking that no part of the range is in
> >>>> its own memory allocation. This 2nd approach may, however, not really be
> >>>> suitable for PVH Dom0, I think.
> >>>
> >>> 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?  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.

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

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

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

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

-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]