Re: [PATCH 2/2] x86/hvm: fix write emulation of RO ranges

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Apr 09, 2025 at 04:08:47PM +0200, Jan Beulich wrote:
> On 09.04.2025 16:01, Roger Pau Monné wrote:
> > On Wed, Apr 09, 2025 at 03:50:13PM +0200, Jan Beulich wrote:
> >> On 09.04.2025 15:33, Roger Pau Monné wrote:
> >>> On Wed, Apr 09, 2025 at 02:59:45PM +0200, Jan Beulich wrote:
> >>>> On 09.04.2025 12:39, Roger Pau Monné wrote:
> >>>>> On Wed, Apr 09, 2025 at 12:00:16PM +0200, Jan Beulich wrote:
> >>>>>> On 09.04.2025 11:07, Roger Pau Monné wrote:
> >>>>>>> On Tue, Apr 08, 2025 at 03:57:17PM +0200, Jan Beulich wrote:
> >>>>>>>> On 08.04.2025 11:31, Roger Pau Monne wrote:
> >>>>>>>>> When running on AMD hardware in HVM mode the guest linear address (GLA)
> >>>>>>>>> will not be provided to hvm_emulate_one_mmio(), and instead is
> >>>>>>>>> unconditionally set of ~0.  As a consequence mmio_ro_emulated_write() will
> >>>>>>>>> always report an error, as the fault GLA generated by the emulation of the
> >>>>>>>>> access won't be ~0.
> >>>>>>>>
> >>>>>>>> Which means subpage_mmio_write_accept() is flawed, too, on AMD (or more
> >>>>>>>> generally whenever .gla_valid isn't set).
> >>>>>>>
> >>>>>>> Oh, yes, good catch.  I didn't notice that one.  We should move all
> >>>>>>> those checks to use a paddr rather than a gla.
> >>>>>>
> >>>>>> Really that function could just be passed the offset into the page.
> >>>>>>
> >>>>>>>>> Fix this by only checking for the fault GLA in mmio_ro_emulated_write()
> >>>>>>>>> when the guest is PV.
> >>>>>>>>
> >>>>>>>> This narrows checking too much, imo. For VT-x we could continue to do so,
> >>>>>>>> provided we pass e.g. npfec down into hvm_emulate_one_mmio(), i.e. make
> >>>>>>>> the gla_valid flag visible there.
> >>>>>>>
> >>>>>>> I don't think we should rely on the gla at all in
> >>>>>>> mmio_ro_emulated_write(), and instead just use the physical address.
> >>>>>>
> >>>>>> But you can't validate a physical address against a CR2 value. And I view
> >>>>>> this validation as meaningful, to guard (best effort, but still) against
> >>>>>> e.g. insn re-writing under our feet.
> >>>>>
> >>>>> But we have the mfn in mmio_ro_ctxt, and could possibly use that to
> >>>>> validate?  I could expand the context to include the offset also, so
> >>>>> that we could fully validate it.
> >>>>
> >>>> How would you use the MFN to validate against the VA in CR2?
> >>>
> >>> I would use hvmemul_virtual_to_linear()
> >>
> >> If you mean to use the CR2 as input, you wouldn't need this. I said VA in
> >> my earlier reply, yes, but strictly speaking that's a linear address.
> > 
> > I was thinking about using the segment and offset parameters of the
> > mmio_ro_emulated_write() call.
> > 
> >>> and hvm_translate_get_page()
> >>> to get the underlying mfn of the linear address.  But maybe there's a
> >>> part of this that I'm missing, I've certainly haven't tried to
> >>> implement any of it.
> >>
> >> Hmm, I see. I didn't think of doing it this way round. There's certainly
> >> at least one caveat with this approach: Multiple linear addresses can all
> >> map to the same GFN and hence MFN. Checking against the original linear
> >> address (when available) doesn't have such an issue.
> > 
> > I see... Yet for AMD that address is not uniformly available as part
> > of the vmexit information?
> 
> Even stronger, I thought: It's uniformly not available.

Oh yes, that's what I meant to say but got the words the other way
around.

> >  As I understand the checks done in
> > mmio_ro_emulated_write() are to ensure correctness, but carrying the
> > access even when the %cr2 check fail wouldn't cause issues to Xen
> > itself?
> 
> Well, "wouldn't" is too strong for my taste, "shouldn't" would fit. The
> checking is there to avoid guests playing games. Whether that prevents
> merely in-guest just-bugs or actual XSAs we can't know until we find a
> case where the game playing might make us do something wrong. I expect
> it's unlikely for Xen itself to be affected. But an in-guest privilege
> escalation would already be bad enough.

I see.  That was kind of my understanding of the checks.  At least on
HVM it feels a bit weird to handle r/o regions this way.  It would IMO
be more natural to use an hvm_io_handler, but that's maybe because I'm
more familiar with those.

And in that regard, hvm_io_handler don't seem to do any of the extra
checking that mmio_ro_emulated_write() does with the %cr2, but maybe
that's done by some higher layer?  AFAICT that would ultimately get
called by hvmemul_read(), and there are no checks there at all.

> So why don't we do the linear address check as we do today (provided a
> linear address is available), and only use the alternative approach when
> no linear address is available?

I can try to do that, albeit as said above, at least for HVM guests
that checking of %cr2 seems to be quite inconsistent, as
hvmemul_{read,write}() won't do any of it.

Thanks, Roger.