Re: [PATCH 2/2] x86/pv: Handle #PF correctly when reading the IO permission bitmap

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/10/2024 8:36 am, Jan Beulich wrote:
> On 30.09.2024 18:18, Andrew Cooper wrote:
>> RFC: Should we make the boundary check be (port + bytes + 8)?  That would be
>> more correct, but liable to break unsuspecting VMs.  Maybe we should just
>> comment our way out of it.
> What would the "+ 8" be intended to express? (I take it you mean ...
>
>> --- a/xen/arch/x86/pv/emul-priv-op.c
>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>> @@ -169,29 +169,26 @@ static intguest_io_okay(unsigned int port, unsigned 
>> int bytes,
>>  
>>      if ( (port + bytes) <= v->arch.pv.iobmp_limit )
> ... this check, which looks correct to me as is. In particular with the
> "+ 8" there would appear to be no way to access ports at the very top of
> the 64k range anymore, as PHYSDEVOP_set_iobitmap handling caps nr_ports
> at 64k. IOW I think "commenting our way out of it" is the only possible
> approach.)

This is actually a horrid corner case, different to what I was thinking
yesterday.

In a real TSS, the CPU doesn't read past the TR limit.  Such accesses
are turned into a permission failure.

But in Xen, guest_io_okay() does read one byte past iobmp_limit.

Previously, we'd consume what was there if it was accessible, or declare
a permissions failure if it was inaccessible.  Now, we'll throw #PF back
to the kernel with %cr2 beyond the the limit.


It's not OK for Xen to be reading past the given limit; it's not how
real CPUs behave.  Part of the problem is that, even since it's
introduction in 013351bd7 (2006), the public interface was named
nr_ports while Xen's internal field was called iobmp_limit.

In terms of how it's used these days in PV guests:

Linux points the bitmap into it's real TSS, and sets nr_ports to either
0 or 65536 depending on whether the userspace task is permitted to use
IO ports or not.

NetBSD doesn't seem to use PHYSDEVOP_set_iobitmap at all, and only uses
PHYSDEVOP_set_iopl.

MiniOS uses neither.


I think Xen's internal variable wants renaming to not be a limit, and
the comment for PHYSDEVOP_set_iobitmap wants extending to note that,
like a real TSS, Xen might read one byte beyond the end of the bitmap.

I think this wants doing in a separate patch.  Thoughts?

> With or without such a comment added
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

~Andrew