Re: [PATCH for-9.2] kvm: Use 'unsigned long' for request argument in functions wrapping ioctl()

[Eric Blake]

On Thu, Aug 15, 2024 at 01:27:47PM GMT, Peter Maydell wrote:
> From: Johannes Stoelp <johannes.stoelp@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.stoelp@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.maydell@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.stoelp@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+3Ycg6kLnpmyg@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 <eblake@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