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

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, May 02, 2022 at 09:37:39AM +0200, Jan Beulich wrote:
> 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).

Yup, that’s what the BUG_ON()s were for.  I will use ASSERT() in the
next round.

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

Attachment: signature.asc
Description: PGP signature