From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-f194.google.com ([209.85.216.194]:46708 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726985AbeIMNWL (ORCPT ); Thu, 13 Sep 2018 09:22:11 -0400 MIME-Version: 1.0 References: <20180908142837.2819693-1-arnd@arndb.de> <20180908142837.2819693-6-arnd@arndb.de> <20180909041114.GD19965@ZenIV.linux.org.uk> <20180912072854.13b4c3b8@mschwideX1> <20180913084242.217e6b77@mschwideX1> In-Reply-To: <20180913084242.217e6b77@mschwideX1> From: Arnd Bergmann Date: Thu, 13 Sep 2018 10:13:31 +0200 Message-ID: Subject: Re: [PATCH 06/11] compat_ioctl: remove /dev/random commands To: Martin Schwidefsky Cc: Al Viro , "Theodore Ts'o" , gregkh , Linux Kernel Mailing List , Linux FS-devel Mailing List , linux-s390 , Heiko Carstens Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Sep 13, 2018 at 8:42 AM Martin Schwidefsky wrote: > > On Wed, 12 Sep 2018 16:02:40 +0200 > Arnd Bergmann wrote: > > > On Wed, Sep 12, 2018 at 7:29 AM Martin Schwidefsky > > wrote: > > > On Tue, 11 Sep 2018 22:26:54 +0200 Arnd Bergmann 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