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)
>  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 
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)
        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;
        return copy_to_user(compat, set, size) ? -EFAULT : 0;

int put_compat_old_sigset(compat_old_sigset_t __user *compat, const sigset_t 
        /* 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 
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 <v...@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?

Reply via email to