Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1642104 > unrolled thread

[PATCH 8/8] waitid(): switch copyout of siginfo to unsafe_put_user()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 8/8] waitid(): switch copyout of siginfo to unsafe_put_user() Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-16 00:40 +0200
    Re: [lkp-robot] [waitid()]  75f64d68f9:  Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode= Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-21 09:40 +0200
      Re: [lkp-robot] [waitid()] 75f64d68f9: Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode= Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-21 21:10 +0200
      Re: [lkp-robot] [waitid()]  75f64d68f9:  Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode= Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-21 21:40 +0200
        Re: [lkp-robot] [waitid()]  75f64d68f9:  Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode= Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-21 23:20 +0200
          Re: [lkp-robot] [waitid()] 75f64d68f9: Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode= Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-21 23:40 +0200
            Re: [lkp-robot] [waitid()] 75f64d68f9: Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode= Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-22 00:30 +0200
              Re: [lkp-robot] [waitid()] 75f64d68f9: Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode= Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-22 03:40 +0200

#1642104 — [PATCH 8/8] waitid(): switch copyout of siginfo to unsafe_put_user()

From: Al Viro <viro@zeniv.linux.org.uk>

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 kernel/exit.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index e93876b06b28..a13dd4d0dc34 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1625,15 +1625,18 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
 	if (!infop)
 		return err;
 
-	if (put_user(err ? 0 : SIGCHLD, &infop->si_signo) ||
-	    put_user(0, &infop->si_errno) ||
-	    put_user((short)info.why, &infop->si_code) ||
-	    put_user(info.pid, &infop->si_pid) ||
-	    put_user(info.uid, &infop->si_uid) ||
-	    put_user(info.status, &infop->si_status))
-		err = -EFAULT;
-
+	user_access_begin();
+	unsafe_put_user(err ? 0 : SIGCHLD, &infop->si_signo, Efault);
+	unsafe_put_user(0, &infop->si_errno, Efault);
+	unsafe_put_user((short)info.why, &infop->si_code, Efault);
+	unsafe_put_user(info.pid, &infop->si_pid, Efault);
+	unsafe_put_user(info.uid, &infop->si_uid, Efault);
+	unsafe_put_user(info.status, &infop->si_status, Efault);
+	user_access_end();
 	return err;
+Efault:
+	user_access_end();
+	return -EFAULT;
 }
 
 static long kernel_wait4(pid_t upid, int __user *stat_addr,
@@ -1736,13 +1739,20 @@ COMPAT_SYSCALL_DEFINE5(waitid,
 			return -EFAULT;
 	}
 
-	if (put_user(err ? 0 : SIGCHLD, &uinfo->si_signo) ||
-	    put_user(0, &uinfo->si_errno) ||
-	    put_user((short)info.why, &uinfo->si_code) ||
-	    put_user(info.pid, &uinfo->si_pid) ||
-	    put_user(info.uid, &uinfo->si_uid) ||
-	    put_user(info.status, &uinfo->si_status))
-		err = -EFAULT;
+	if (!uinfo)
+		return err;
+
+	user_access_begin();
+	unsafe_put_user(err ? 0 : SIGCHLD, &infop->si_signo, Efault);
+	unsafe_put_user(0, &infop->si_errno, Efault);
+	unsafe_put_user((short)info.why, &infop->si_code, Efault);
+	unsafe_put_user(info.pid, &infop->si_pid, Efault);
+	unsafe_put_user(info.uid, &infop->si_uid, Efault);
+	unsafe_put_user(info.status, &infop->si_status, Efault);
+	user_access_end();
 	return err;
+Efault:
+	user_access_end();
+	return -EFAULT;
 }
 #endif
-- 
2.11.0

[toc] | [next] | [standalone]

#1646275 — Re: [lkp-robot] [waitid()] 75f64d68f9: Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode=

On Fri, May 19, 2017 at 02:08:20PM +0800, kernel test robot wrote:
> 
> FYI, we noticed the following commit:
> 
> commit: 75f64d68f9816a1c244b8685f056389b24d97e98 ("waitid(): switch copyout of siginfo to unsafe_put_user()")
> url: https://github.com/0day-ci/linux/commits/Al-Viro/move-compat-wait4-and-waitid-next-to-native-variants/20170516-084127
> 
> 
> in testcase: boot

Cute...  That's unsafe_put_user() bug, actually.  There's no unsafe_put_user()
callers in mainline and it's fairly early in the cycle.  Linus, do you have
any problems with that one?  If not, I'll send a pull request with it + osf_wait4()
fix...

fix unsafe_put_user()

__put_user_size() relies upon its first argument having the same type as what
the second one points to; the only other user makes sure of that and
unsafe_put_user() should do the same.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 68766b276d9e..d9668c3beb5b 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -703,7 +703,7 @@ extern struct movsl_mask {
 #define unsafe_put_user(x, ptr, err_label)					\
 do {										\
 	int __pu_err;								\
-	__put_user_size((x), (ptr), sizeof(*(ptr)), __pu_err, -EFAULT);		\
+	__put_user_size((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), __pu_err, -EFAULT);		\
 	if (unlikely(__pu_err)) goto err_label;					\
 } while (0)

[toc] | [prev] | [next] | [standalone]

#1646397 — Re: [lkp-robot] [waitid()] 75f64d68f9: Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode=

On Sun, May 21, 2017 at 12:34 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> -       __put_user_size((x), (ptr), sizeof(*(ptr)), __pu_err, -EFAULT);         \
> +       __put_user_size((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), __pu_err, -EFAULT);             \

Hmm. Looking more at this, the "unsafe_get_user()" case is wrong too -
for types larger than "long".

But I see you have a pull request pending, and I'll take this fix as-is.

I *think* the right thing to do is to just do

   register __inttype(*(ptr)) __val_gu;

for unsafe_get_user.

I think the error crept in because I copied the "get_user_ex()" code,
which has the same type confusion (ie it doesn't handle values larger
then long, so "long long" on x86-32 wouldn't work).

That type limitation was ok'ish simply because get_user_ex() was
x86-only and of very limited use (and clearly never saw the 64-bit
value on a 32-bit arch case).

But for unsafe_get_user() we obviously want to make it generic enough
and just be able to replace existing get_user() calls.

                  Linus

[toc] | [prev] | [next] | [standalone]

#1646401 — Re: [lkp-robot] [waitid()] 75f64d68f9: Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode=

On Sun, 21 May 2017, Al Viro wrote:
> 
> fix unsafe_put_user()

So here's my proposed patch on top of yours to fix unsafe_get_user() with 
"long long" arguments, and to clean up the extra-long line you did.

Comments?

                  Linus

---
 arch/x86/include/asm/uaccess.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index d9668c3beb5b..661c497465ce 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -703,14 +703,15 @@ extern struct movsl_mask {
 #define unsafe_put_user(x, ptr, err_label)					\
 do {										\
 	int __pu_err;								\
-	__put_user_size((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), __pu_err, -EFAULT);		\
+	__typeof__(*(ptr)) __pu_val = (__typeof__(*(ptr)))(x);			\
+	__put_user_size(__pu_val, (ptr), sizeof(*(ptr)), __pu_err, -EFAULT);	\
 	if (unlikely(__pu_err)) goto err_label;					\
 } while (0)
 
 #define unsafe_get_user(x, ptr, err_label)					\
 do {										\
 	int __gu_err;								\
-	unsigned long __gu_val;							\
+	__inttype(*(ptr)) __gu_val;						\
 	__get_user_size(__gu_val, (ptr), sizeof(*(ptr)), __gu_err, -EFAULT);	\
 	(x) = (__force __typeof__(*(ptr)))__gu_val;				\
 	if (unlikely(__gu_err)) goto err_label;					\

[toc] | [prev] | [next] | [standalone]

#1646446 — Re: [lkp-robot] [waitid()] 75f64d68f9: Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode=

On Sun, May 21, 2017 at 12:35:28PM -0700, Linus Torvalds wrote:
> 
> 
> On Sun, 21 May 2017, Al Viro wrote:
> > 
> > fix unsafe_put_user()
> 
> So here's my proposed patch on top of yours to fix unsafe_get_user() with 
> "long long" arguments, and to clean up the extra-long line you did.
> 
> Comments?

>  #define unsafe_get_user(x, ptr, err_label)					\
>  do {										\
>  	int __gu_err;								\
> -	unsigned long __gu_val;							\
> +	__inttype(*(ptr)) __gu_val;						\

Umm...  get_user() for anything larger than long is simply not supported on
a lot of architectures[1].  Do we really want to do that for unsafe_get_user()?

[1] at the moment, blackfin, m32r, m68k/mmu, microblaze, mn10300, nios2, sh.
arm allows it for get_user() (and rmk was really unhappy about doing so), but
not for __get_user().

[toc] | [prev] | [next] | [standalone]

#1646450 — Re: [lkp-robot] [waitid()] 75f64d68f9: Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode=

On Sun, May 21, 2017 at 2:14 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Umm...  get_user() for anything larger than long is simply not supported on
> a lot of architectures[1].  Do we really want to do that for unsafe_get_user()?

I'm pretty sure there's a reason we added support for it on x86-32,
because there are structures that use __u64 and fill things one entry
at a time.

It's entirely possible that that code then fails (maybe it compiles,
but doesn't work) on various other architectures. There's a lot of
drivers that are disabled on non-x86.

                  Linus

[toc] | [prev] | [next] | [standalone]

#1646468 — Re: [lkp-robot] [waitid()] 75f64d68f9: Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode=

On Sun, May 21, 2017 at 2:37 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I'm pretty sure there's a reason we added support for it on x86-32,
> because there are structures that use __u64 and fill things one entry
> at a time.

Did an "allyesconfig" build on 32-bit x86, and looked at who uses the
8-byte get_user/put_user cases:

__get_user_8:
    i915_perf_open_ioctl

__put_user_8:
    snapshot_ioctl
    sys_sendfile64
    timerfd_read
    eventfd_read
    userfaultfd_ioctl
    kpagecgroup_read
    kpagecount_read
    kpageflags_read
    __ncp_ioctl
    blkdev_ioctl
    drm_mode_object_get_properties
    drm_mode_getproperty_ioctl
    efi_test_ioctl
    params_to_user
    __rds_rdma_map

so it's not common, but both do get used.

Would any of those be changed to the unsafe versions? Maybe not. But I
think we're better off being consistent.

We basically allow all kernel integer types to be used for
put/get_user(), and the fact that some architectures don't support
them is just a quirk of that architecture, not a sign that it
shouldn't be done.

                  Linus

[toc] | [prev] | [next] | [standalone]

#1646482 — Re: [lkp-robot] [waitid()] 75f64d68f9: Kernel_panic-not_syncing:Attempted_to_kill_init!exitcode=

On Sun, May 21, 2017 at 3:19 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Did an "allyesconfig" build on 32-bit x86, and looked at who uses the
> 8-byte get_user/put_user cases:

I've done more testing.

It turns out that quite independently of all these patches, our 32-bit
x86 code is entirely broken.

In particular, __get_user_asm_u64() has two independent bugs, one
fairly harmless, and one that can be entirely deadly.

The harmless one is that we have the ASM_STAC/ASM_CLAC markers around
the user access in there, even though they should have gotten removed.
That cone can be considered a "merge error" between commit

  b2f680380ddf ("x86/mm/32: Add support for 64-bit __get_user() on
32-bit kernels")

that added the 64-bit case, and commit

  11f1a4b9755f x86: reorganize SMAP handling in user space accesses

that moved the CLAC/STAC into the caller.

So it turns out that a 64-bit __get_user() case will have a double
pair of STAC/CLAC instructions, making it even slower than it should
otherwise be. But it all still *works* fine.

The much worse issue is that the asm is just buggered, and when it does

      "1: movl %2,%%eax\n" \
      "2: movl %3,%%edx\n" \

it can be that %eax is actually used for the address, so the second
move can do crazy bad things. It can (and does) generate code like
this:

 18c:   8b 00                   mov    (%eax),%eax
 18e:   8b 50 04                mov    0x4(%eax),%edx

(I'm not sure that actually happens anywhere in the kernel, but it did
happen in my test-case).

So the 64-bit output needs to be marked as being an early-clobber,
meaning that it can be written to early in the asm. So we need to use
"=&A", not "=A" for it.

Adding Ben LaHaise to the cc, since that "=A" bug goes back to the
original implementation of __get_user_asm_u64() (which is only a year
ago, but still).

I've committed a fix, and now the generated asm looks ok, but I don't
actually have any 32-bit x86 machines left. Hopefully somebody still
does and can test this..

                Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web