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