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

Thanks, Roger.