Re: [PATCH v3] kernel/signal.c: fix BUG_ON with SIG128 (MIPS)

Re: [PATCH v3] kernel/signal.c: fix BUG_ON with SIG128 (MIPS)

[James Hogan]

On 25/06/13 23:13, James Hogan wrote:
> On 25 June 2013 22:40, Andrew Morton <akpm@linux-foundation.org> wrote:
>> Meanwhile, unprivileged users can make a MIPS kernel go BUG.
>>
>> How much of a problem is this?  Obviously less of a problem with MIPS
>> than it would be with some other CPU types, but I'd imagine it's still
>> awkward in some environments.
>>
>> If this _is_ considered a problem, can we think of some nasty little
>> hack which at least makes the effects less damaging, which we can also
>> put into -stable kernels?
> 
> The first rfc patch I sent sort of satisfies that by passing 127 if
> sig==128, or slightly better would be passing 126 if sig>=127 (so that
> SIFSIGNALED returns true). Effectively #ifdef'ing it on _NSIG>127 as
> this patch does may be preferable too.
> 
> That's probably the minimum change necessary to evade the BUG_ON
> without removing it. The wait status code will still be wrong, but it
> wasn't exactly right before so it's no worse.
> 
> IMO changing the ABI by reducing _NSIG to 127 or 126 isn't appropriate
> for stable.

How does this look for a nasty/stable fix?

>From 94d734526d61f5c74fd2df1c3ecb677495fc7a23 Mon Sep 17 00:00:00 2001
From: James Hogan <james.hogan@imgtec.com>
Date: Wed, 26 Jun 2013 11:48:11 +0100
Subject: [PATCH 1/1] kernel/signal.c: fix BUG_ON with SIG128 (MIPS)

MIPS has 128 signals, the highest of which has the number 128 (they
start from 1). The following command causes get_signal_to_deliver() to
pass this signal number straight through to do_group_exit() as the exit
code:

  strace sleep 10 & sleep 1 && kill -128 `pidof sleep`

However do_group_exit() checks for the core dump bit (0x80) in the exit
code which matches in this particular case and the kernel panics:

  BUG_ON(exit_code & 0x80); /* core dumps don't get here */

As a quick fix, mask out higher bits in the signal number. This
effectively matches the exit code from other code paths but avoids the
BUG_ON.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: David Daney <david.daney@cavium.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Dave Jones <davej@redhat.com>
Cc: linux-mips@linux-mips.org
Cc: stable@vger.kernel.org
---
 kernel/signal.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 113411b..9ea8f4f 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2366,8 +2366,14 @@ relock:
 
 		/*
 		 * Death signals, no core dump.
+		 *
+		 * Some architectures (MIPS) have 128 signals which doesn't play
+		 * nicely with the exit code since there are only 7 bits to
+		 * store the terminating signal number. Mask out higher bits to
+		 * avoid overflowing into the core dump bit and triggering
+		 * BUG_ON in do_group_exit.
 		 */
-		do_group_exit(info->si_signo);
+		do_group_exit(info->si_signo & 0x7f);
 		/* NOTREACHED */
 	}
 	spin_unlock_irq(&sighand->siglock);
-- 
1.8.1.2

Re: [PATCH v3] kernel/signal.c: fix BUG_ON with SIG128 (MIPS)

[Ralf Baechle]

On Wed, Jun 26, 2013 at 12:07:44PM +0100, James Hogan wrote:

> > IMO changing the ABI by reducing _NSIG to 127 or 126 isn't appropriate
> > for stable.
> 
> How does this look for a nasty/stable fix?

> diff --git a/kernel/signal.c b/kernel/signal.c
> index 113411b..9ea8f4f 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2366,8 +2366,14 @@ relock:
>  
>  		/*
>  		 * Death signals, no core dump.
> +		 *
> +		 * Some architectures (MIPS) have 128 signals which doesn't play
> +		 * nicely with the exit code since there are only 7 bits to
> +		 * store the terminating signal number. Mask out higher bits to
> +		 * avoid overflowing into the core dump bit and triggering
> +		 * BUG_ON in do_group_exit.
>  		 */
> -		do_group_exit(info->si_signo);
> +		do_group_exit(info->si_signo & 0x7f);
>  		/* NOTREACHED */
>  	}
>  	spin_unlock_irq(&sighand->siglock);

Looks like something which I think we could live with.

Clearly it also scores in the "nasty" category, so fits the bill ;)

  Ralf

Re: [PATCH v3] kernel/signal.c: fix BUG_ON with SIG128 (MIPS)

[Oleg Nesterov]

On 06/26, James Hogan wrote:
>
> On 25/06/13 23:13, James Hogan wrote:
>   BUG_ON(exit_code & 0x80); /* core dumps don't get here */
>
> As a quick fix, mask out higher bits in the signal number. This
> effectively matches the exit code from other code paths but avoids the
> BUG_ON.
> 
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: David Daney <david.daney@cavium.com>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Dave Jones <davej@redhat.com>
> Cc: linux-mips@linux-mips.org
> Cc: stable@vger.kernel.org
> ---
>  kernel/signal.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 113411b..9ea8f4f 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2366,8 +2366,14 @@ relock:
>  
>  		/*
>  		 * Death signals, no core dump.
> +		 *
> +		 * Some architectures (MIPS) have 128 signals which doesn't play
> +		 * nicely with the exit code since there are only 7 bits to
> +		 * store the terminating signal number. Mask out higher bits to
> +		 * avoid overflowing into the core dump bit and triggering
> +		 * BUG_ON in do_group_exit.
>  		 */
> -		do_group_exit(info->si_signo);
> +		do_group_exit(info->si_signo & 0x7f);

Or simply remove the BUG_ON(), this can equally confuse wait(status).
128 & 0x7f == 0.

Still I think it would be better to change _NSIG on mips.

Oleg.

Re: [PATCH v3] kernel/signal.c: fix BUG_ON with SIG128 (MIPS)

[Ralf Baechle]

On Wed, Jun 26, 2013 at 06:14:52PM +0200, Oleg Nesterov wrote:

> Or simply remove the BUG_ON(), this can equally confuse wait(status).
> 128 & 0x7f == 0.
> 
> Still I think it would be better to change _NSIG on mips.

If it was that easy.  That's going to outright break binary compatibility,
see kernel/signal.c:

SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, nset,
                sigset_t __user *, oset, size_t, sigsetsize)
{
        sigset_t old_set, new_set;
        int error;

        /* XXX: Don't preclude handling different sized sigset_t's.  */
        if (sigsetsize != sizeof(sigset_t))
                return -EINVAL;

There are several more more syscalls performing tests like the above.

So at least the kernel sigset_t will have to remain constant, maybe something
like below, totally untested patch which I'm sure is going to open a few
20 foot containers full of worms such as NSIG being defined by glibc to 128
and fixing the kernel won't magically change installed libc headers or
binaries incorporating NSIG.

  Ralf

 arch/mips/include/uapi/asm/signal.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/mips/include/uapi/asm/signal.h b/arch/mips/include/uapi/asm/signal.h
index addb9f5..8bba323 100644
--- a/arch/mips/include/uapi/asm/signal.h
+++ b/arch/mips/include/uapi/asm/signal.h
@@ -11,12 +11,13 @@
 
 #include <linux/types.h>
 
-#define _NSIG		128
+#define _NSIG		64
 #define _NSIG_BPW	(sizeof(unsigned long) * 8)
 #define _NSIG_WORDS	(_NSIG / _NSIG_BPW)
 
 typedef struct {
 	unsigned long sig[_NSIG_WORDS];
+	unsigned long __fill[_NSIG_WORDS];
 } sigset_t;
 
 typedef unsigned long old_sigset_t;		/* at least 32 bits */

Re: [PATCH v3] kernel/signal.c: fix BUG_ON with SIG128 (MIPS)

[Oleg Nesterov]

On 06/26, Ralf Baechle wrote:
>
> On Wed, Jun 26, 2013 at 06:14:52PM +0200, Oleg Nesterov wrote:
>
> > Or simply remove the BUG_ON(), this can equally confuse wait(status).
> > 128 & 0x7f == 0.
> >
> > Still I think it would be better to change _NSIG on mips.
>
> If it was that easy.  That's going to outright break binary compatibility,
> see kernel/signal.c:
>
> SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, nset,
>                 sigset_t __user *, oset, size_t, sigsetsize)
> {
>         sigset_t old_set, new_set;
>         int error;
>
>         /* XXX: Don't preclude handling different sized sigset_t's.  */
>         if (sigsetsize != sizeof(sigset_t))
>                 return -EINVAL;

I meant the minimal hack like

	--- x/arch/mips/include/uapi/asm/signal.h
	+++ x/arch/mips/include/uapi/asm/signal.h
	@@ -11,9 +11,9 @@
	 
	 #include <linux/types.h>
	 
	-#define _NSIG		128
	+#define _NSIG		127
	 #define _NSIG_BPW	(sizeof(unsigned long) * 8)
	-#define _NSIG_WORDS	(_NSIG / _NSIG_BPW)
	+#define _NSIG_WORDS	DIV_ROUND_UP(_NSIG / _NSIG_BPW)
	 
	 typedef struct {
		unsigned long sig[_NSIG_WORDS];

just to avoid BUG_ON().

I agree that _NSIG == 126 or 64 needs more discussion. Although personally
I think this is the only choice in the long term, or we should change ABI
and break user-space completely.

And, just in case, the hack above doesn't kill SIG_128 completely.
Say, the task can block/unblock it.

Oleg.

Re: [PATCH v3] kernel/signal.c: fix BUG_ON with SIG128 (MIPS)

[James Hogan]

On 26/06/13 18:15, Oleg Nesterov wrote:
> On 06/26, Ralf Baechle wrote:
>>
>> On Wed, Jun 26, 2013 at 06:14:52PM +0200, Oleg Nesterov wrote:
>>
>>> Or simply remove the BUG_ON(), this can equally confuse wait(status).
>>> 128 & 0x7f == 0.
>>>
>>> Still I think it would be better to change _NSIG on mips.
>>
>> If it was that easy.  That's going to outright break binary compatibility,
>> see kernel/signal.c:
>>
>> SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, nset,
>>                 sigset_t __user *, oset, size_t, sigsetsize)
>> {
>>         sigset_t old_set, new_set;
>>         int error;
>>
>>         /* XXX: Don't preclude handling different sized sigset_t's.  */
>>         if (sigsetsize != sizeof(sigset_t))
>>                 return -EINVAL;
> 
> I meant the minimal hack like
> 
> 	--- x/arch/mips/include/uapi/asm/signal.h
> 	+++ x/arch/mips/include/uapi/asm/signal.h
> 	@@ -11,9 +11,9 @@
> 	 
> 	 #include <linux/types.h>
> 	 
> 	-#define _NSIG		128
> 	+#define _NSIG		127
> 	 #define _NSIG_BPW	(sizeof(unsigned long) * 8)
> 	-#define _NSIG_WORDS	(_NSIG / _NSIG_BPW)
> 	+#define _NSIG_WORDS	DIV_ROUND_UP(_NSIG / _NSIG_BPW)
> 	 
> 	 typedef struct {
> 		unsigned long sig[_NSIG_WORDS];
> 
> just to avoid BUG_ON().
> 
> I agree that _NSIG == 126 or 64 needs more discussion. Although personally
> I think this is the only choice in the long term, or we should change ABI
> and break user-space completely.
> 
> And, just in case, the hack above doesn't kill SIG_128 completely.
> Say, the task can block/unblock it.

Well it prevents a handler being added or the signal being sent, so it
pretty much does kill it (patch v2 did this). Since glibc already has
SIGRTMAX=127, nothing running glibc should be affected unless it's being
really stupid (uClibc and bionic is a different matter).

I've booted debian with only 64 signals and an appropriate printk in
do_sigaction, and although various things try and set all the handlers,
I didn't found anything that breaks because of them being missing.

I've also audited all the binaries installed on my desktop (Fedora 17)
which contain libc_current_sigrtmax to see if I can find anything
problematic. Various things iterate all the signals (which doesn't do
any harm if a bunch are missing, especially when EINVAL is often
expected for SIGRTMIN..SIGRTMIN+2 because of libc using them). High
level language bindings open up some potential for breakage since
SIGRTMAX etc can be exposed to higher levels. The only real problem I've
found though is openjdk:

http://hg.openjdk.java.net/jdk7/jsn/jdk/file/tip/src/solaris/native/java/net/linux_close.c
57 /*
58  * Signal to unblock thread
59  */
60 static int sigWakeup = (__SIGRTMAX - 2);

It sets up a signal handler when the library is loaded (without any
error checking), and tries to send the signal to all threads blocked on
a file descriptor to wake them up (again without error checking), called
from NET_Dup2 and NET_SocketClose. Clearly this could easily break
backwards compatibility if SIGRTMAX is reduced any lower than 126.

Obviously this isn't exhaustive (I haven't tried android etc or actually
running many applications, and open source software probably isn't the
best example of badly written code), but it looks like it may be safe to
reduce _NSIG to 127 for a stable fix if nobody objects, and possibly to
126 in the future to avoid the wait exit status problem.

Cheers
James

Re: [PATCH v3] kernel/signal.c: fix BUG_ON with SIG128 (MIPS)

[Oleg Nesterov]

On 06/28, James Hogan wrote:
>
> On 26/06/13 18:15, Oleg Nesterov wrote:
> >
> > I meant the minimal hack like
> >
> > 	--- x/arch/mips/include/uapi/asm/signal.h
> > 	+++ x/arch/mips/include/uapi/asm/signal.h
> > 	@@ -11,9 +11,9 @@
> >
> > 	 #include <linux/types.h>
> >
> > 	-#define _NSIG		128
> > 	+#define _NSIG		127
> > 	 #define _NSIG_BPW	(sizeof(unsigned long) * 8)
> > 	-#define _NSIG_WORDS	(_NSIG / _NSIG_BPW)
> > 	+#define _NSIG_WORDS	DIV_ROUND_UP(_NSIG / _NSIG_BPW)
> >
> > 	 typedef struct {
> > 		unsigned long sig[_NSIG_WORDS];
> >
> > just to avoid BUG_ON().
> >
> > I agree that _NSIG == 126 or 64 needs more discussion. Although personally
> > I think this is the only choice in the long term, or we should change ABI
> > and break user-space completely.
> >
> > And, just in case, the hack above doesn't kill SIG_128 completely.
> > Say, the task can block/unblock it.
>
> Well it prevents a handler being added or the signal being sent, so it
> pretty much does kill it (patch v2 did this).

Yes, iirc you already sent something like the hack above.

> but it looks like it may be safe to
> reduce _NSIG to 127 for a stable fix

This was my point.

Sure, this change can break something anyway, we can't know if nobody
ever uses 128 anyway. But this is better than the ability to crash the
kernel. No need to use strace, just block(128) + kill(128) + unblock().

So perhaps you can resend your patch? Just I think it makes sense to
update the changelog to explain that this is not the "final" solution
but the minimal fix.

Oleg.

Re: [PATCH v3] kernel/signal.c: fix BUG_ON with SIG128 (MIPS)

[Denys Vlasenko]

On Wednesday 26 June 2013 18:59, Ralf Baechle wrote:
> On Wed, Jun 26, 2013 at 06:14:52PM +0200, Oleg Nesterov wrote:
> 
> > Or simply remove the BUG_ON(), this can equally confuse wait(status).
> > 128 & 0x7f == 0.
> > 
> > Still I think it would be better to change _NSIG on mips.
> 
> If it was that easy.  That's going to outright break binary compatibility,
> see kernel/signal.c:
> 
> SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, nset,
>                 sigset_t __user *, oset, size_t, sigsetsize)
> {
>         sigset_t old_set, new_set;
>         int error;
> 
>         /* XXX: Don't preclude handling different sized sigset_t's.  */
>         if (sigsetsize != sizeof(sigset_t))
>                 return -EINVAL;
> 
> There are several more more syscalls performing tests like the above.

Reducing _NSIG to 127 (or 126) doesn't change sigset_t, so should be fine wrt above.