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

Groups > linux.kernel > #1704700 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] Fix compat_sys_sigpending breakage introduced by  v4.13-rc1~6^2~12 "Dmitry V. Levin" <ldv@altlinux.org> - 2017-08-05 22:10 +0200
    Re: [PATCH] Fix compat_sys_sigpending breakage introduced by  v4.13-rc1~6^2~12 Al Viro <viro@ZenIV.linux.org.uk> - 2017-08-06 20:30 +0200
      Re: [PATCH] Fix compat_sys_sigpending breakage introduced by v4.13-rc1~6^2~12 Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-06 20:50 +0200

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

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.

This bug was found by strace test suite.

Reported-by: Anatoly Pugachev <matorola@gmail.com>
Inspired-by: Eugene Syromyatnikov <evgsyr@gmail.com>
Fixes: 8f13621abced ("sigpending(): move compat to native")
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 kernel/signal.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index caed913..7e33f8c 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -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
 }
 #endif
 
-- 
ldv

[toc] | [next] | [standalone]

#1704939

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]);
}

in kernel/signal.c and turn e.g. compat rt_sigpending() into
COMPAT_SYSCALL_DEFINE2(rt_sigpending, compat_sigset_t __user *, uset,
                compat_size_t, sigsetsize)
{
        sigset_t set;
        int err = do_sigpending(&set, sigsetsize);
        if (!err)
		err = put_compat_sigset(uset, &set, sigsetsize);
        return err;
}

etc., including
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;
}

Incidentally, this
        if (sigsetsize > sizeof(sigset_t))
                return -EINVAL;
should probably be lifted out of do_sigpending() into rt_sigpending() and its
compat analog - sigsetsize argument is not used anywhere else in do_sigpending(),
and rt_sigpending() (and *especially* its compat counterpart) would be a lot
more obviously correct with that check done where it's seen.

Anyway, all of that can be done later; for now let's go with your patch.
ACKed-by: Al Viro <viro@zeniv.linux.org.uk>

I would pick it through my tree, but the local network is half-disasembled
for move (containers arrive tomorrow, flight to Boston on 9th, stuff should
arrive there by the weekend, so I hope to be back to normal by the 14th
or so, assuming I'll have any sanity left by that time).  Could somebody
else pick that one?  Linus?

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

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

On Sun, Aug 6, 2017 at 11:22 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> Anyway, all of that can be done later; for now let's go with your patch.
> ACKed-by: Al Viro <viro@zeniv.linux.org.uk>
>
> I would pick it through my tree, but the local network is half-disasembled
> for move (containers arrive tomorrow, flight to Boston on 9th, stuff should
> arrive there by the weekend, so I hope to be back to normal by the 14th
> or so, assuming I'll have any sanity left by that time).  Could somebody
> else pick that one?  Linus?

I'll take it directly,

                   Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web