Re: [PATCH v0] kvm: unsigned datatype in ioctl wrapper

[Johannes S]

On Sun, Aug 29, 2021 at 11:10 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> It would be more helpful to readers to state the reason directly
> in the commit message, rather than requiring them to go and look
> up a comment in some other file.

Thanks for the tip, that makes sense. I will follow it next time.

> > Of the various KVM_* ioctls we use via these functions, do
> > any actually have values that would result in invalid sign
> > extension here ? That is, is this fixing an existing bug, or is
> > it merely avoiding a potential future bug?
>
> My question as well.  If there is such a bug, calling it out in the
> commit message is essential; if the bug is just theoretical,
> mentioning that is still useful.

I agree, more details would have been helpful here.

The background is, that I observed some sign-extension for certain kvm
ioctl requests. However that was in an artificial environment which
included an ioctl wrapper that was defined as
    int ioctl(int fd, unsigned long type, ...);

One example for such an ioctl request is the 'KVM_IRQ_LINE_STATUS'.

So this is not fixing a actual bug, but a theoretical one.

On Mon, Aug 30, 2021 at 5:47 PM Eric Blake <eblake@redhat.com> wrote:
> The fact that glibc uses unsigned long rather than int for the second
> argument is a strong argument in favor of using an unsigned type (on
> 64-bit platforms, the kernel really is looking at 64 bits, even though
> POSIX says we are only passing in 32, and sign-extension is wrong),
> but on the other hand, I don't know if any ioctl requests CAN be sign
> extended (ideally, no ioctl request has bit 0x80000000 set, so that it
> doesn't matter if the userspace code was calling via a signed or
> unsigned type, or via the 32-bit POSIX signature instead of the actual
> kernel 'unsigned long' signature).

On my machine (64bit) I found some ioctl requests with the 0x8000_0000 bit
set, eg
- KVM_IRQ_LINE_STATUS
- KVM_GET_MSR_INDEX_LIST
- KVM_SET_IRQCHIP

I was curious and looked into the macro definitions and it turns out on my
machine, all requests with the '_IOC_READ' direction seem to have the
0x8000_0000 bit set.
_IOC_READ = 2
_IOC_DIRSHIFT = 30
_IOC_READ << _IOC_DIRSHIFT = 0x80000000

#define KVM_SET_IRQCHIP     _IOR(KVMIO,  0x63, struct kvm_irqchip)
#define _IOR(type,nr,size)  _IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)))
#define _IOC(dir,type,nr,size) \
    (((dir)  << _IOC_DIRSHIFT) | \
     ((type) << _IOC_TYPESHIFT) | \
     ((nr)   << _IOC_NRSHIFT) | \
     ((size) << _IOC_SIZESHIFT))

On Mon, Aug 30, 2021 at 7:33 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> I found this glibc bug from 2012, filed by some random guy named
> Linus Torvalds, and still open:
> https://sourceware.org/bugzilla/show_bug.cgi?id=14362
> where among other things he claims
> # As noted, this does not actually cause problems on Linux, because
> # unlike FreeBSD, Linux knows what the f*ck it is doing, and just
> # ignores the upper bits exactly because of possible sign confusion.

Sounds like we are saved be the Linux Kernel here :)

In my opinion we should use 'unsigned' data types here for the ioctl
request in the ioctl wrappers or would you prefer to keep the ioctl
wrapper definition as is today? What is you opinion?

Thanks and best,
Johannes