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

Groups > linux.kernel > #1716930 > unrolled thread

Re: [PATCH] Fix compat_sys_sigpending breakage introduced by v4.13-rc1~6^2~12

Started by"Dmitry V. Levin" <ldv@altlinux.org>
First post2017-08-22 01:10 +0200
Last post2017-08-22 01:20 +0200
Articles 4 — 1 participant

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

  Re: [PATCH] Fix compat_sys_sigpending breakage introduced by  v4.13-rc1~6^2~12 "Dmitry V. Levin" <ldv@altlinux.org> - 2017-08-22 01:10 +0200
    [PATCH 2/3] signal: simplify compat_sigpending() "Dmitry V. Levin" <ldv@altlinux.org> - 2017-08-22 01:20 +0200
    [PATCH 3/3] signal: lift sigset size check out of do_sigpending() "Dmitry V. Levin" <ldv@altlinux.org> - 2017-08-22 01:20 +0200
    [PATCH 1/3] signal: replace sigset_to_compat() with  put_compat_sigset() "Dmitry V. Levin" <ldv@altlinux.org> - 2017-08-22 01:20 +0200

#1716930 — Re: [PATCH] Fix compat_sys_sigpending breakage introduced by v4.13-rc1~6^2~12

From"Dmitry V. Levin" <ldv@altlinux.org>
Date2017-08-22 01:10 +0200
SubjectRe: [PATCH] Fix compat_sys_sigpending breakage introduced by v4.13-rc1~6^2~12
Message-ID<uh3Zv-2fV-5@gated-at.bofh.it>
On Sun, Aug 06, 2017 at 07:22:03PM +0100, Al Viro wrote:
> On Sat, Aug 05, 2017 at 11:00:50PM +0300, Dmitry V. Levin wrote:
> > The latest change of compat_sys_sigpending has broken it in two ways.
> > 
> > First, it tries to write 4 bytes more than userspace expects:
> > sizeof(old_sigset_t) == sizeof(long) == 8 instead of
> > sizeof(compat_old_sigset_t) == sizeof(u32) == 4.
> > 
> > Second, on big endian architectures these bytes are being written
> > in the wrong order.
> 
> > @@ -3303,12 +3303,15 @@ SYSCALL_DEFINE1(sigpending, old_sigset_t __user *, set)
> >  #ifdef CONFIG_COMPAT
> >  COMPAT_SYSCALL_DEFINE1(sigpending, compat_old_sigset_t __user *, set32)
> >  {
> > +#ifdef __BIG_ENDIAN
> >  	sigset_t set;
> > -	int err = do_sigpending(&set, sizeof(old_sigset_t)); 
> > -	if (err == 0)
> > -		if (copy_to_user(set32, &set, sizeof(old_sigset_t)))
> > -			err = -EFAULT;
> > +	int err = do_sigpending(&set, sizeof(set.sig[0]));
> > +	if (!err)
> > +		err = put_user(set.sig[0], set32);
> >  	return err;
> > +#else
> > +	return sys_rt_sigpending((sigset_t __user *)set32, sizeof(*set32));
> > +#endif
> 
> Interesting...  Basically, your fix makes it parallel to compat rt_sigpending(2);
> I agree that the bug is real and gets fixed by that, but...  rt_sigpending()
> itself looks a bit fishy.  There we have
>                 compat_sigset_t set32;
>                 sigset_to_compat(&set32, &set);
>                 /* we can get here only if sigsetsize <= sizeof(set) */
>                 if (copy_to_user(uset, &set32, sigsetsize))
>                         err = -EFAULT;
> in big-endian case; now, there are 4 callers of sigset_to_compat() in the
> entire kernel.  One in sparc compat rt_sigaction(2), the rest in kernel/signal.c
> itself.  All are followed by copy_to_user(), and all but the sparc one are
> under that kind of "if it's big-endian..." ifdefs.
> 
> Looks like it might make sense to do this:
> put_compat_sigset(compat_sigset_t __user *compat, const sigset_t *set, int size)
> {
> #ifdef 
> 	compat_sigset_t v;
>         switch (_NSIG_WORDS) {
>         case 4: v.sig[7] = (set->sig[3] >> 32); v.sig[6] = set->sig[3];
>         case 3: v.sig[5] = (set->sig[2] >> 32); v.sig[4] = set->sig[2];
>         case 2: v.sig[3] = (set->sig[1] >> 32); v.sig[2] = set->sig[1];
>         case 1: v.sig[1] = (set->sig[0] >> 32); v.sig[0] = set->sig[0];
>         }
> 	return copy_to_user(compat, &v, size) ? -EFAULT : 0;
> #else
> 	return copy_to_user(compat, set, size) ? -EFAULT : 0;
> #endif
> }
> 
> int put_compat_old_sigset(compat_old_sigset_t __user *compat, const sigset_t *set)
> {
> 	/* we want bits 0--31 of the bitmap */
> 	return put_user(compat, set->sig[0]);
> }
[...]
> COMPAT_SYSCALL_DEFINE2(sigpending, compat_old_sigset_t __user *, uset)
> {
>         sigset_t set;
>         int err = do_sigpending(&set, sizeof(set));
>         if (!err)
> 		err = put_compat_old_sigset(uset, &set);
>         return err;
> }

I don't think a separate function for put_user(compat, set->sig[0])
is needed given that its only user is going to be compat_sigpending().

Introducing put_compat_sigset() and moving sigset size check out
of do_sigpending() definitely makes sense, patches will follow shortly.


-- 
ldv

[toc] | [next] | [standalone]

#1716934 — [PATCH 2/3] signal: simplify compat_sigpending()

From"Dmitry V. Levin" <ldv@altlinux.org>
Date2017-08-22 01:20 +0200
Subject[PATCH 2/3] signal: simplify compat_sigpending()
Message-ID<uh49b-2jb-3@gated-at.bofh.it>
In reply to#1716930
Remove "if it's big-endian..." ifdef in compat_sigpending(),
use the endian-agnostic variant.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 kernel/signal.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index a1d0426..7d9d82b 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3292,15 +3292,11 @@ SYSCALL_DEFINE1(sigpending, old_sigset_t __user *, set)
 #ifdef CONFIG_COMPAT
 COMPAT_SYSCALL_DEFINE1(sigpending, compat_old_sigset_t __user *, set32)
 {
-#ifdef __BIG_ENDIAN
 	sigset_t set;
 	int err = do_sigpending(&set, sizeof(set.sig[0]));
 	if (!err)
 		err = put_user(set.sig[0], set32);
 	return err;
-#else
-	return sys_rt_sigpending((sigset_t __user *)set32, sizeof(*set32));
-#endif
 }
 #endif
 
-- 
ldv

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

#1716935 — [PATCH 3/3] signal: lift sigset size check out of do_sigpending()

From"Dmitry V. Levin" <ldv@altlinux.org>
Date2017-08-22 01:20 +0200
Subject[PATCH 3/3] signal: lift sigset size check out of do_sigpending()
Message-ID<uh49b-2jb-5@gated-at.bofh.it>
In reply to#1716930
As sigsetsize argument of do_sigpending() is not used anywhere else in
that function after the check, remove this argument and move the check
out of do_sigpending() into rt_sigpending() and its compat analog.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 kernel/signal.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 7d9d82b..894418b 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2629,11 +2629,8 @@ COMPAT_SYSCALL_DEFINE4(rt_sigprocmask, int, how, compat_sigset_t __user *, nset,
 }
 #endif
 
-static int do_sigpending(void *set, unsigned long sigsetsize)
+static int do_sigpending(sigset_t *set)
 {
-	if (sigsetsize > sizeof(sigset_t))
-		return -EINVAL;
-
 	spin_lock_irq(&current->sighand->siglock);
 	sigorsets(set, &current->pending.signal,
 		  &current->signal->shared_pending.signal);
@@ -2653,7 +2650,12 @@ static int do_sigpending(void *set, unsigned long sigsetsize)
 SYSCALL_DEFINE2(rt_sigpending, sigset_t __user *, uset, size_t, sigsetsize)
 {
 	sigset_t set;
-	int err = do_sigpending(&set, sigsetsize);
+	int err;
+
+	if (sigsetsize > sizeof(*uset))
+		return -EINVAL;
+
+	err = do_sigpending(&set);
 	if (!err && copy_to_user(uset, &set, sigsetsize))
 		err = -EFAULT;
 	return err;
@@ -2664,7 +2666,12 @@ COMPAT_SYSCALL_DEFINE2(rt_sigpending, compat_sigset_t __user *, uset,
 		compat_size_t, sigsetsize)
 {
 	sigset_t set;
-	int err = do_sigpending(&set, sigsetsize);
+	int err;
+
+	if (sigsetsize > sizeof(*uset))
+		return -EINVAL;
+
+	err = do_sigpending(&set);
 	if (!err)
 		err = put_compat_sigset(uset, &set, sigsetsize);
 	return err;
@@ -3293,7 +3300,7 @@ SYSCALL_DEFINE1(sigpending, old_sigset_t __user *, set)
 COMPAT_SYSCALL_DEFINE1(sigpending, compat_old_sigset_t __user *, set32)
 {
 	sigset_t set;
-	int err = do_sigpending(&set, sizeof(set.sig[0]));
+	int err = do_sigpending(&set);
 	if (!err)
 		err = put_user(set.sig[0], set32);
 	return err;
-- 
ldv

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

#1716939 — [PATCH 1/3] signal: replace sigset_to_compat() with put_compat_sigset()

From"Dmitry V. Levin" <ldv@altlinux.org>
Date2017-08-22 01:20 +0200
Subject[PATCH 1/3] signal: replace sigset_to_compat() with put_compat_sigset()
Message-ID<uh49c-2jb-13@gated-at.bofh.it>
In reply to#1716930
There are 4 callers of sigset_to_compat() in the entire kernel.  One is
in sparc compat rt_sigaction(2), the rest are in kernel/signal.c itself.
All are followed by copy_to_user(), and all but the sparc one are under
"if it's big-endian..." ifdefs.

Let's transform sigset_to_compat() into put_compat_sigset() that also
calls copy_to_user().

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 arch/sparc/kernel/sys_sparc32.c |  6 +++---
 include/linux/compat.h          |  3 ++-
 kernel/compat.c                 | 20 ++++++++++++++------
 kernel/signal.c                 | 27 ++++++---------------------
 4 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/arch/sparc/kernel/sys_sparc32.c b/arch/sparc/kernel/sys_sparc32.c
index bca44f3..5e2bec9 100644
--- a/arch/sparc/kernel/sys_sparc32.c
+++ b/arch/sparc/kernel/sys_sparc32.c
@@ -159,7 +159,6 @@ COMPAT_SYSCALL_DEFINE5(rt_sigaction, int, sig,
 {
         struct k_sigaction new_ka, old_ka;
         int ret;
-	compat_sigset_t set32;
 
         /* XXX: Don't preclude handling different sized sigset_t's.  */
         if (sigsetsize != sizeof(compat_sigset_t))
@@ -167,6 +166,7 @@ COMPAT_SYSCALL_DEFINE5(rt_sigaction, int, sig,
 
         if (act) {
 		u32 u_handler, u_restorer;
+		compat_sigset_t set32;
 
 		new_ka.ka_restorer = restorer;
 		ret = get_user(u_handler, &act->sa_handler);
@@ -183,9 +183,9 @@ COMPAT_SYSCALL_DEFINE5(rt_sigaction, int, sig,
 	ret = do_sigaction(sig, act ? &new_ka : NULL, oact ? &old_ka : NULL);
 
 	if (!ret && oact) {
-		sigset_to_compat(&set32, &old_ka.sa.sa_mask);
 		ret = put_user(ptr_to_compat(old_ka.sa.sa_handler), &oact->sa_handler);
-		ret |= copy_to_user(&oact->sa_mask, &set32, sizeof(compat_sigset_t));
+		ret |= put_compat_sigset(&oact->sa_mask, &old_ka.sa.sa_mask,
+					 sizeof(oact->sa_mask));
 		ret |= put_user(old_ka.sa.sa_flags, &oact->sa_flags);
 		ret |= put_user(ptr_to_compat(old_ka.sa.sa_restorer), &oact->sa_restorer);
 		if (ret)
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 5a6a109..17017bb 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -453,7 +453,8 @@ asmlinkage long compat_sys_settimeofday(struct compat_timeval __user *tv,
 asmlinkage long compat_sys_adjtimex(struct compat_timex __user *utp);
 
 extern void sigset_from_compat(sigset_t *set, const compat_sigset_t *compat);
-extern void sigset_to_compat(compat_sigset_t *compat, const sigset_t *set);
+extern int put_compat_sigset(compat_sigset_t __user *compat,
+			     const sigset_t *set, unsigned int size);
 
 asmlinkage long compat_sys_migrate_pages(compat_pid_t pid,
 		compat_ulong_t maxnode, const compat_ulong_t __user *old_nodes,
diff --git a/kernel/compat.c b/kernel/compat.c
index 6f0a0e7..d2bd03b 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -520,15 +520,23 @@ sigset_from_compat(sigset_t *set, const compat_sigset_t *compat)
 }
 EXPORT_SYMBOL_GPL(sigset_from_compat);
 
-void
-sigset_to_compat(compat_sigset_t *compat, const sigset_t *set)
+int
+put_compat_sigset(compat_sigset_t __user *compat, const sigset_t *set,
+		  unsigned int size)
 {
+	/* size <= sizeof(compat_sigset_t) <= sizeof(sigset_t) */
+#ifdef __BIG_ENDIAN
+	compat_sigset_t v;
 	switch (_NSIG_WORDS) {
-	case 4: compat->sig[7] = (set->sig[3] >> 32); compat->sig[6] = set->sig[3];
-	case 3: compat->sig[5] = (set->sig[2] >> 32); compat->sig[4] = set->sig[2];
-	case 2: compat->sig[3] = (set->sig[1] >> 32); compat->sig[2] = set->sig[1];
-	case 1: compat->sig[1] = (set->sig[0] >> 32); compat->sig[0] = set->sig[0];
+	case 4: v.sig[7] = (set->sig[3] >> 32); v.sig[6] = set->sig[3];
+	case 3: v.sig[5] = (set->sig[2] >> 32); v.sig[4] = set->sig[2];
+	case 2: v.sig[3] = (set->sig[1] >> 32); v.sig[2] = set->sig[1];
+	case 1: v.sig[1] = (set->sig[0] >> 32); v.sig[0] = set->sig[0];
 	}
+	return copy_to_user(compat, &v, size) ? -EFAULT : 0;
+#else
+	return copy_to_user(compat, set, size) ? -EFAULT : 0;
+#endif
 }
 
 #ifdef CONFIG_NUMA
diff --git a/kernel/signal.c b/kernel/signal.c
index ed804a4..a1d0426 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2621,13 +2621,7 @@ COMPAT_SYSCALL_DEFINE4(rt_sigprocmask, int, how, compat_sigset_t __user *, nset,
 		if (error)
 			return error;
 	}
-	if (oset) {
-		compat_sigset_t old32;
-		sigset_to_compat(&old32, &old_set);
-		if (copy_to_user(oset, &old32, sizeof(compat_sigset_t)))
-			return -EFAULT;
-	}
-	return 0;
+	return oset ? put_compat_sigset(oset, &old_set, sizeof(*oset)) : 0;
 #else
 	return sys_rt_sigprocmask(how, (sigset_t __user *)nset,
 				  (sigset_t __user *)oset, sigsetsize);
@@ -2669,20 +2663,11 @@ SYSCALL_DEFINE2(rt_sigpending, sigset_t __user *, uset, size_t, sigsetsize)
 COMPAT_SYSCALL_DEFINE2(rt_sigpending, compat_sigset_t __user *, uset,
 		compat_size_t, sigsetsize)
 {
-#ifdef __BIG_ENDIAN
 	sigset_t set;
 	int err = do_sigpending(&set, sigsetsize);
-	if (!err) {
-		compat_sigset_t set32;
-		sigset_to_compat(&set32, &set);
-		/* we can get here only if sigsetsize <= sizeof(set) */
-		if (copy_to_user(uset, &set32, sigsetsize))
-			err = -EFAULT;
-	}
+	if (!err)
+		err = put_compat_sigset(uset, &set, sigsetsize);
 	return err;
-#else
-	return sys_rt_sigpending((sigset_t __user *)uset, sigsetsize);
-#endif
 }
 #endif
 
@@ -3413,7 +3398,6 @@ COMPAT_SYSCALL_DEFINE4(rt_sigaction, int, sig,
 		compat_size_t, sigsetsize)
 {
 	struct k_sigaction new_ka, old_ka;
-	compat_sigset_t mask;
 #ifdef __ARCH_HAS_SA_RESTORER
 	compat_uptr_t restorer;
 #endif
@@ -3425,6 +3409,7 @@ COMPAT_SYSCALL_DEFINE4(rt_sigaction, int, sig,
 
 	if (act) {
 		compat_uptr_t handler;
+		compat_sigset_t mask;
 		ret = get_user(handler, &act->sa_handler);
 		new_ka.sa.sa_handler = compat_ptr(handler);
 #ifdef __ARCH_HAS_SA_RESTORER
@@ -3440,10 +3425,10 @@ COMPAT_SYSCALL_DEFINE4(rt_sigaction, int, sig,
 
 	ret = do_sigaction(sig, act ? &new_ka : NULL, oact ? &old_ka : NULL);
 	if (!ret && oact) {
-		sigset_to_compat(&mask, &old_ka.sa.sa_mask);
 		ret = put_user(ptr_to_compat(old_ka.sa.sa_handler), 
 			       &oact->sa_handler);
-		ret |= copy_to_user(&oact->sa_mask, &mask, sizeof(mask));
+		ret |= put_compat_sigset(&oact->sa_mask, &old_ka.sa.sa_mask,
+					 sizeof(oact->sa_mask));
 		ret |= put_user(old_ka.sa.sa_flags, &oact->sa_flags);
 #ifdef __ARCH_HAS_SA_RESTORER
 		ret |= put_user(ptr_to_compat(old_ka.sa.sa_restorer),

-- 
ldv

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web