| Message ID | 20170805200050.GA24804@altlinux.org |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Aug 06, 2017 at 07:22:03PM +0100, Al Viro wrote: > 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. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Al. On Mon, Aug 28, 2017 at 05:41:49AM +0100, Al Viro wrote: > On Sun, Aug 06, 2017 at 07:22:03PM +0100, Al Viro wrote: > > > 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 -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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(-)