Re: [PATCH 06/11] compat_ioctl: remove /dev/random commands

[Arnd Bergmann]

On Thu, Sep 13, 2018 at 8:42 AM Martin Schwidefsky
<schwidefsky@xxxxxxxxxx> wrote:
>
> On Wed, 12 Sep 2018 16:02:40 +0200
> Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> > On Wed, Sep 12, 2018 at 7:29 AM Martin Schwidefsky
> > <schwidefsky@xxxxxxxxxx> wrote:
> > > On Tue, 11 Sep 2018 22:26:54 +0200 Arnd Bergmann <arnd@xxxxxxxx> wrote:

> >
> > This should probably be separate from the change to using compat_ptr()
> > in all other drivers, and I could easily drop this change if you prefer,
> > it is meant only as a cosmetic change.
>
> So generic_compat_ioctl_ptrarg will to the compat_ptr thing on the
> "unsigned int cmd" argument? Should work just fine.

It will do it on the "unsigned long arg" argument, I assume that's
what you meant. The "cmd" argument is correctly zero-extended
by the COMPAT_SYSCALL_DEFINE() wrapper on architectures
that need that (IIRC s390 is in that category).

> > I don't think we hit that problem anywhere: in the ioctl
> > argument we pass an 'unsigned long' that has already
> > been zero-extended by the compat_sys_ioctl() wrapper,
> > while any other usage would get extended by the compiler
> > when casting from compat_uptr_t to a 64-bit type.
> > This would be different if you had a function call with the
> > wrong prototype, i.e. calling a function declared as taking
> > an compat_uptr_t, but defining it as taking a void __user*.
> > (I suppose that is undefined behavior).
>
> That is true. For the ioctls we have a compat "unsigned int"
> or "unsigned long" and the system call wrapper must have cleared
> the upper half already. There are a few places where we copy
> a data structure from user space, then read a 32-bit pointer
> from the structure. These get the compat_ptr treatment as well.
> All of those structure definitions should use compat_uptr_t
> though, the compiler has to do the zero extension at the time
> the 32-bit value is cast to a pointer.

There is actually one more case: A number of the newer
interfaces that have ioctl structures with indirect pointers
encoded as __u64, so the layout becomes
and we don't normally need a conversion handler.

An example of this would be the sys_rseq() system call
that passes a relatively complex structure in place
of a pointer:

struct rseq {
        ...
        union {
                __u64 ptr64;
#ifdef __LP64__
                __u64 ptr;
#else
                struct {
#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) ||
defined(__BIG_ENDIAN)
                        __u32 padding;          /* Initialized to zero. */
                        __u32 ptr32;
#else /* LITTLE */
                        __u32 ptr32;
                        __u32 padding;          /* Initialized to zero. */
#endif /* ENDIAN */
                } ptr;
#endif
        } rseq_cs;
       __u32 flags;
};

We require user space to initialize the __padding field to
zero and then use the ptr64 field in the kernel as a pointer:

        u64 ptr;
        u32 __user *usig;
        copy_from_user(&ptr, &t->rseq->rseq_cs.ptr64, sizeof(ptr));
        urseq_cs = (struct rseq_cs __user *)(unsigned long)ptr;

but we don't ever clear bit 31 here. A similar pattern is used
in many device drivers (I could not find any that would apply to
s390 though). In theory, 32 bit user space might pass a pointer
with the high bit set in the ptr32 field, and that gets misinterpreted
by the kernel (resulting in -EFAULT).

It would be interesting to know whether there could be user space
that gets compiled from portable source code with a normal
C compiler but produces that high bit set, as opposed to someone
intentially settting the bit just to trigger the bug.

       Arnd