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 <jbeul...@suse.com> Thanks. ~Andrew
