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