Fix compat_sys_sigpending breakage introduced by v4.13-rc1~6^2~12
PATCH 1/1 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.
--- kernel/signal.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
kernel/signal.c +7 -4
@@ -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_ENDIANsigset_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
Signature · 1 line
ldv
Quoted 16 lines · Dmitry V. Levin
The latest change of
compat_sys_sigpendinghas broken it in two ways.First, it tries to write 4 bytes more than userspace expects:
sizeof(old_sigset_t)= sizeof(long) = 8 instead ofsizeof(compat_old_sigset_t)= sizeof(u32) = 4.Second, on big endian architectures these bytes are being written in the wrong order.
kernel/signal.c +1
-1@@ -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_ENDIANsigset_t set; -int err =do_sigpending (&set,sizeof (old_sigset_t));
kernel/signal.c SYSCALL_DEFINE1(sigpending, old_sigset_t __user *, set) · line 3314
Quoted 10 lines
kernel/signal.c +6
-3-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., includingCOMPAT_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.
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?
Quoted 8 lines · Al Viro
Anyway, all of that can be done later; for now let's go with your patch.
Acked-by Al Viro <[email protected]>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
Quoted 63 lines · Al Viro
Quoted 16 lines · Dmitry V. Levin
The latest change of
compat_sys_sigpendinghas broken it in two ways.First, it tries to write 4 bytes more than userspace expects:
sizeof(old_sigset_t)== sizeof(long) == 8 instead ofsizeof(compat_old_sigset_t)== sizeof(u32) == 4.Second, on big endian architectures these bytes are being written in the wrong order.
kernel/signal.c +1
-1@@ -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_ENDIANsigset_t set; -int err =do_sigpending (&set,sizeof (old_sigset_t));kernel/signal.c SYSCALL_DEFINE1(sigpending, old_sigset_t __user *, set) · line 3314
Quoted 10 lines
kernel/signal.c +6
-3-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 havecompat_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 compatrt_sigaction(2), the rest inkernel/signal.citself. All are followed bycopy_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]); }
[...]
Quoted 8 lines
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.
Signature · 1 line
ldv
PATCH 1/3 signal: replace sigset_to_compat() with put_compat_sigset()
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().
--- 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(-)
arch/sparc/kernel/sys_sparc32.c +3 -3
@@ -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)
include/linux/compat.h +2 -1
@@ -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,
kernel/compat.c +14 -6
@@ -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
kernel/signal.c +6 -21
@@ -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_ENDIANsigset_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_RESTORERcompat_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),
Signature · 1 line
ldv
PATCH 2/3 signal: simplify compat_sigpending()
In reply to Dmitry V. LevinRemove "if it's big-endian..." ifdef in compat_sigpending(), use the endian-agnostic variant.
--- kernel/signal.c | 4 ---- 1 file changed, 4 deletions(-)
kernel/signal.c +0 -4
@@ -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_ENDIANsigset_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
Signature · 1 line
ldv
PATCH 3/3 signal: lift sigset size check out of do_sigpending()
In reply to Dmitry V. LevinAs 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.
--- kernel/signal.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
kernel/signal.c +14 -7
@@ -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 (¤t->sighand->siglock);sigorsets (set, ¤t->pending.signal, ¤t->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 &©_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;
Signature · 1 line
ldv
Quoted 4 lines · Al Viro
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).
... and that hope had turned out to be far too optimistic. Getting the things back into working shape took two weeks longer than that; by now most of the damage has been dealt with. Dmitry's followups applied to for-next queue, with apologies for delay.
Hi Al.
Quoted 5 lines · Al Viro
Quoted 1 line · Al Viro
assuming I'll have any sanity left by that time).
... and that hope had turned out to be far too optimistic.
Ohh, looking forward to what impact this will have on patch descriptions and reply's to mails :-)
Hope that you have setteled well after moving. It can take sevaral months before one really starts to feel at home at a new place.
Sam