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

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

That is even simpler than I thought!  I was worried that
InstallConfigurationTable() would assume that memory for the table was
allocated a certain way and cause invalid free errors, but at least
TianoCore does not do that.

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

I suspect the assumption was that the ESRT would be parsed by the OS
before ExitBootServices(), and that the OS would have no need for the
ESRT after that.

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

There is PVops et al already :)

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