On Thu, Aug 15, 2024 at 01:27:47PM GMT, Peter Maydell wrote: > From: Johannes Stoelp <johannes.sto...@googlemail.com> > > Change the data type of the ioctl _request_ argument from 'int' to > 'unsigned long' for the various accel/kvm functions which are > essentially wrappers around the ioctl() syscall. > > The correct type for ioctl()'s 'request' argument is confused: > * POSIX defines the request argument as 'int'
A bit of history: POSIX defined ioctl() because of the old Solaris STREAMS interface, that ended up deprecated in Issue 7 (2004) and removed in Issue 8 (just released this year). So POSIX no longer specified a signature for ioctl(). Admittedly, it DID add a new interface posix_devctl() designed to be a "portable" replacement for what ioctl() did (by being a new interface, there is no longer a question on what type it has), with the intent that libraries will eventually implement it (perhaps as a wrapper around the real ioctl), and that one uses 'int' for the command, and replaces the varargs of ioctl with a more direct specification of pointer and length. https://pubs.opengroup.org/onlinepubs/9799919799/functions/posix_devctl.html > * glibc uses 'unsigned long' in the prototype in sys/ioctl.h > * the glibc info documentation uses 'int' Arguably, that's a bug in glibc for being self-inconsistent. > * the Linux manpage uses 'unsigned long' > * the Linux implementation of the syscall uses 'unsigned int' And there's more of the history, which didn't become apparent until 64-bit architectures became common, but where we now have fallout like this thread. > > If we wrap ioctl() with another function which uses 'int' as the > type for the request argument, then requests with the 0x8000_0000 > bit set will be sign-extended when the 'int' is cast to > 'unsigned long' for the call to ioctl(). > > On x86_64 one such example is the KVM_IRQ_LINE_STATUS request. > Bit requests with the _IOC_READ direction bit set, will have the high > bit set. > > Fortunately the Linux Kernel truncates the upper 32bit of the request > on 64bit machines (because it uses 'unsigned int', and see also Linus > Torvalds' comments in > https://sourceware.org/bugzilla/show_bug.cgi?id=14362 ) > so this doesn't cause active problems for us. However it is more > consistent to follow the glibc ioctl() prototype when we define > functions that are essentially wrappers around ioctl(). > > This resolves a Coverity issue where it points out that in > kvm_get_xsave() we assign a value (KVM_GET_XSAVE or KVM_GET_XSAVE2) > to an 'int' variable which can't hold it without overflow. For the record, despite POSIX having picked a signed type, I am totally in favor of an unsigned type in any of our uses. > > Resolves: Coverity CID 1547759 > Signed-off-by: Johannes Stoelp <johannes.sto...@gmail.com> > [PMM: Rebased patch, adjusted commit message, included note about > Coverity fix, updated the type of the local var in kvm_get_xsave, > updated the comment in the KVMState struct definition] > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org> > --- > This is a patch that was posted back in 2021, and I reviewed it > at the time > https://lore.kernel.org/qemu-devel/20210901213426.360748-1-johannes.sto...@gmail.com/ > but we never actually took it into the tree. I was reminded of it > by the Coverity issue, where a change to Coverity means it now > complains about the potential integer overflow when we put one > of these high-bit-set ioctls into an "int". So I thought it would > be worth dusting this off and getting it upstream. > > For more discussion of the ioctl request datatype see also the > review thread on the previous version of the patch: > https://lore.kernel.org/qemu-devel/CAFEAcA8TRQdj33Ycm=xzmuuunapaxvgedexfs+3ycg6klnp...@mail.gmail.com/ > > Since this doesn't actually cause any incorrect behaviour this > is obviously for-9.2 material. > --- > include/sysemu/kvm.h | 8 ++++---- > include/sysemu/kvm_int.h | 16 ++++++++++++---- > accel/kvm/kvm-all.c | 8 ++++---- > target/i386/kvm/kvm.c | 3 ++- > accel/kvm/trace-events | 8 ++++---- > 5 files changed, 26 insertions(+), 17 deletions(-) Reviewed-by: Eric Blake <ebl...@redhat.com> > +++ b/include/sysemu/kvm_int.h > @@ -122,10 +122,18 @@ struct KVMState > bool sync_mmu; > bool guest_state_protected; > uint64_t manual_dirty_log_protect; > - /* The man page (and posix) say ioctl numbers are signed int, but > - * they're not. Linux, glibc and *BSD all treat ioctl numbers as > - * unsigned, and treating them as signed here can break things */ > - unsigned irq_set_ioctl; > + /* > + * POSIX says that ioctl numbers are signed int, but in practice Maybe s/POSIX/older POSIX/ (given that newer POSIX does not specify ioctl at all) > + * they are not. Linux, glibc and *BSD all treat ioctl numbers as > + * unsigned, and real-world ioctl values like KVM_GET_XSAVE have > + * bit 31 set, which means that passing them via an 'int' will > + * result in sign-extension when they get converted back to the > + * 'unsigned long' which the ioctl() prototype uses. Luckily Linux > + * always treats the argument as an unsigned 32-bit int, so any > + * possible sign-extension is deliberately ignored, but for > + * consistency we keep to the same type that glibc is using. > + */ > + unsigned long irq_set_ioctl; > unsigned int sigmask_len; > GHashTable *gsimap; -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
