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

[Dmitry V. Levin]

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