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

[Jan Beulich <jbeulich@xxxxxxxx>]

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