linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Arnd Bergmann <arnd@arndb.de>
To: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
	"Theodore Ts'o" <tytso@mit.edu>,
	gregkh <gregkh@linuxfoundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux FS-devel Mailing List <linux-fsdevel@vger.kernel.org>,
	linux-s390 <linux-s390@vger.kernel.org>,
	Heiko Carstens <heiko.carstens@de.ibm.com>
Subject: Re: [PATCH 06/11] compat_ioctl: remove /dev/random commands
Date: Thu, 13 Sep 2018 10:13:31 +0200	[thread overview]
Message-ID: <CAK8P3a3jacWumTAw=tbj1LcPgih+s1ZNa7Hyk-rxVzUhP3_UTw@mail.gmail.com> (raw)
In-Reply-To: <20180913084242.217e6b77@mschwideX1>

On Thu, Sep 13, 2018 at 8:42 AM Martin Schwidefsky
<schwidefsky@de.ibm.com> wrote:
>
> On Wed, 12 Sep 2018 16:02:40 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
>
> > On Wed, Sep 12, 2018 at 7:29 AM Martin Schwidefsky
> > <schwidefsky@de.ibm.com> wrote:
> > > On Tue, 11 Sep 2018 22:26:54 +0200 Arnd Bergmann <arnd@arndb.de> 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

  reply	other threads:[~2018-09-13 13:22 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-08 14:28 [PATCH 01/11] compat_ioctl: remove keyboard ioctl translation Arnd Bergmann
2018-09-08 14:28 ` [PATCH 02/11] compat_ioctl: remove HIDIO translation Arnd Bergmann
2018-09-08 14:28 ` [PATCH 03/11] compat_ioctl: remove translation for sound ioctls Arnd Bergmann
2018-09-09  4:16   ` Al Viro
2018-09-11 19:35     ` Arnd Bergmann
2018-09-08 14:28 ` [PATCH 04/11] compat_ioctl: remove isdn ioctl translation Arnd Bergmann
2018-09-08 14:28 ` [PATCH 05/11] compat_ioctl: remove IGNORE_IOCTL() Arnd Bergmann
2018-09-08 14:28 ` [PATCH 06/11] compat_ioctl: remove /dev/random commands Arnd Bergmann
2018-09-09  4:11   ` Al Viro
2018-09-11 20:26     ` Arnd Bergmann
2018-09-12  5:28       ` Martin Schwidefsky
2018-09-12 14:02         ` Arnd Bergmann
2018-09-13  6:42           ` Martin Schwidefsky
2018-09-13  8:13             ` Arnd Bergmann [this message]
2018-09-08 14:28 ` [PATCH 07/11] compat_ioctl: remove joystick ioctl translation Arnd Bergmann
2018-09-08 14:28 ` [PATCH 08/11] compat_ioctl: remove PCI " Arnd Bergmann
2018-09-08 14:28 ` [PATCH 09/11] compat_ioctl: remove /dev/raw " Arnd Bergmann
2018-09-08 14:28 ` [PATCH 10/11] compat_ioctl: remove last RAID handling code Arnd Bergmann
2018-09-08 14:28 ` [PATCH 11/11] compat_ioctl: move tape handling into drivers Arnd Bergmann
2018-09-09  4:37   ` Al Viro
2018-09-11 15:36     ` Arnd Bergmann
2018-09-11 20:13       ` Arnd Bergmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAK8P3a3jacWumTAw=tbj1LcPgih+s1ZNa7Hyk-rxVzUhP3_UTw@mail.gmail.com' \
    --to=arnd@arndb.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=heiko.carstens@de.ibm.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=schwidefsky@de.ibm.com \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH 06/11] compat_ioctl: remove /dev/random commands' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).