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

[Sam Ravnborg]

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

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

[Sam Ravnborg]

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

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

[Al Viro]

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.

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

[Al Viro]

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.

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(, sizeof(old_sigset_t)); 
> > -   if (err == 0)
> > -   if (copy_to_user(set32, , sizeof(old_sigset_t)))
> > -   err = -EFAULT;
> > +   int err = do_sigpending(, 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(, );
> /* we can get here only if sigsetsize <= sizeof(set) */
> if (copy_to_user(uset, , 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, , 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(, sizeof(set));
> if (!err)
>   err = put_compat_old_sigset(uset, );
> 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

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(, sizeof(old_sigset_t)); 
> > -   if (err == 0)
> > -   if (copy_to_user(set32, , sizeof(old_sigset_t)))
> > -   err = -EFAULT;
> > +   int err = do_sigpending(, 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(, );
> /* we can get here only if sigsetsize <= sizeof(set) */
> if (copy_to_user(uset, , 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, , 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(, sizeof(set));
> if (!err)
>   err = put_compat_old_sigset(uset, );
> 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

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

[Linus Torvalds]

On Sun, Aug 6, 2017 at 11:22 AM, Al Viro  wrote:
> Anyway, all of that can be done later; for now let's go with your patch.
> ACKed-by: 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).  Could somebody
> else pick that one?  Linus?

I'll take it directly,

   Linus

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

[Linus Torvalds]

On Sun, Aug 6, 2017 at 11:22 AM, Al Viro  wrote:
> Anyway, all of that can be done later; for now let's go with your patch.
> ACKed-by: 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).  Could somebody
> else pick that one?  Linus?

I'll take it directly,

   Linus

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

[Al Viro]

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(, sizeof(old_sigset_t)); 
> - if (err == 0)
> - if (copy_to_user(set32, , sizeof(old_sigset_t)))
> - err = -EFAULT;
> + int err = do_sigpending(, 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(, );
/* we can get here only if sigsetsize <= sizeof(set) */
if (copy_to_user(uset, , 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, , 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(, sigsetsize);
if (!err)
err = put_compat_sigset(uset, , sigsetsize);
return err;
}

etc., including
COMPAT_SYSCALL_DEFINE2(sigpending, compat_old_sigset_t __user *, uset)
{
sigset_t set;
int err = do_sigpending(, sizeof(set));
if (!err)
err = put_compat_old_sigset(uset, );
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 

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?

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

[Al Viro]

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(, sizeof(old_sigset_t)); 
> - if (err == 0)
> - if (copy_to_user(set32, , sizeof(old_sigset_t)))
> - err = -EFAULT;
> + int err = do_sigpending(, 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(, );
/* we can get here only if sigsetsize <= sizeof(set) */
if (copy_to_user(uset, , 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, , 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(, sigsetsize);
if (!err)
err = put_compat_sigset(uset, , sigsetsize);
return err;
}

etc., including
COMPAT_SYSCALL_DEFINE2(sigpending, compat_old_sigset_t __user *, uset)
{
sigset_t set;
int err = do_sigpending(, sizeof(set));
if (!err)
err = put_compat_old_sigset(uset, );
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 

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?

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

[Dmitry V. Levin]

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 
Inspired-by: Eugene Syromyatnikov 
Fixes: 8f13621abced ("sigpending(): move compat to native")
Signed-off-by: Dmitry V. Levin 
---
 kernel/signal.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

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(, sizeof(old_sigset_t)); 
-   if (err == 0)
-   if (copy_to_user(set32, , sizeof(old_sigset_t)))
-   err = -EFAULT;
+   int err = do_sigpending(, 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
 
-- 
ldv

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

[Dmitry V. Levin]

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 
Inspired-by: Eugene Syromyatnikov 
Fixes: 8f13621abced ("sigpending(): move compat to native")
Signed-off-by: Dmitry V. Levin 
---
 kernel/signal.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

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(, sizeof(old_sigset_t)); 
-   if (err == 0)
-   if (copy_to_user(set32, , sizeof(old_sigset_t)))
-   err = -EFAULT;
+   int err = do_sigpending(, 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
 
-- 
ldv