situation with signals

situation with signals

[Al Viro]

	There are several interesting issues in arch/*/*/*signal* (besides
shoggoths starting to show up when one reads that code) and I'd been crawling
through that area for the last few weeks.  Here are more or less common
issues; there are really arch-specific bugs (e.g. roothole on frv that
used to allow reading kernel memory by setting the right sa_handler), but
that's a separate story.

	1) a bunch of architectures happily block signals even when
setting sigframe up fails.  Obviously bad, since we lose the information
about original mask forever.  Some fixes are in mainline, some are
pending (mips one sent to Ralf, the rest is still in my local queue).

	2) avr32 and microblaze have retained the old bug fixed by sfr in
"[PATCH] convert signal handling of NODEFER to act like other Unix boxes"
back in 2005.  Fixes in local queue, will send shortly.

	3) sigreturn and its ilk should reset ->restart_block.fn to
do_no_restart_syscall(); alpha and parisc had been doing that in the
wrong place (missed race fixes ~5 years ago), frv, microblaze, mn10300,
score and xtensa don't do that at all.  alpha and frv fixes are in mainline,
the rest - in local queue.

	4) SIGSEGV on failure to create a sigframe should be delivered by
force_sigsegv(), not force_sig().  The difference between these two is that
the latter will lead to infinite spin if we have a handler for SIGSEGV.
force_sigsegv() will revert to default handler if the signal we'd failed
to deliver had been SIGSEGV itself.  Vulnerable: frv and mn10300; frv fix
in mainlined, mn10300 in local queue.

	5) minor QOI issue - if we are sending SIGSEGV after failure to set
sigframe up, it's better to avoid mangling registers, so that if we do
handler SIGSEGV (e.g. on altstack) we would have a sane userland state to
return to.  Minor, probably not worth bothering until the merge window;
most of the architectures are very easy to tweak so that the problem would
disappear.

	6) sigsuspend()/rt_sigsuspend() really ought to switch to use
of ->saved_sigmask; it's easy to do and fairly few architectures are
still not using that.  I'll probably leave that until the merge window;
it's not breakage per se.  Exception: m32r fix I'll send today; the damn
thing has switched to sigsuspend()/rt_sigsuspend() using ERESTARTNOHAND
(and setting ->saved_sigmask), but do_signal() doesn't even look at the
->saved_sigmask.  With obvious results.  Breakage has been there for
three years...

	7) sigreturn() should make sure we won't step into signal restart.
arm, frv and ppc fixed (fixes merged).  avr32, cris and score are b0rken the
same way arm is, fixes in local queue.  microblaze seems to be fscked the
same way, unless I'm missing something subtle in their sigreturn wrapper.

	8) no double syscall restarts if we get multiple signals, even if
we handle some or all of them in the kernel.  alpha, ppc, frv and sparc fixes
are in mainline, arm one sits in arm tree.  More is in the local queue and
I'm afraid that there's more to follow.

	9) there's a difference between architectures in the way we handle
multiple signals pending at the same time.  Some deal with all signals that
are there; some usually handle one, but may do more if the stars are right;
some handle only one and that's it.
	The last variant actually used to be common until 5-8 years ago.
There are several problems with it, starting with the obvious "WTF is that
SIGSEGV generated when I failed to set a sigframe up gets delivered only
on the next IRQ or syscall?".  I've looked through the history and apparently
there had been several independent reinventions of the fix (== handle all
pending signals, build all sigframes and let it go).
	I must apologize to davem - sparc is not the only remaining one with
that behaviour; several m68nommu derivatives are also that way.  IMO the right
thing to do is to switch all of them to "call do_signal until there's no
pending signal left" kind of behaviour; sparc patch doing that is pending
review, and I can do the rest (embedded ones) without too much PITA.
	There's one case where the problem has been papered over by kludges
inside do_signal() - itanic.  It's easier to do repeats in asm glue, actually,
(replace p6 = 0 with p6 = 1 in one place) and that'll kill a bunch of stuff
in the ia64/signal.c; similar junk is in place for parisc, with no
justification whatsoever - there we _do_ repeats in entry.S.

Re: situation with signals

[Arnd Bergmann]

On Friday 24 September 2010, Al Viro wrote:
>         There are several interesting issues in arch/*/*/*signal* (besides
> shoggoths starting to show up when one reads that code) and I'd been crawling
> through that area for the last few weeks.  Here are more or less common
> issues; there are really arch-specific bugs (e.g. roothole on frv that
> used to allow reading kernel memory by setting the right sa_handler), but
> that's a separate story.

I still plan to make a counterpart to the asm-generic headers with an
example architecture that new architectures can copy from. Signal handling
is one of the areas that I have very limited understanding of. Did you
encounter any architecture that basically gets signal handling right and that
can serve as a positive example to others?

arch/tile/kernel/signal.c was the last one that got merged and I tried
to direct the maintainer in the right direction as much as I could, but
there are a lot of things I didn't know about.

	Arnd

Re: situation with signals

[Chris Metcalf]

On 9/24/2010 9:33 AM, Arnd Bergmann wrote:
> On Friday 24 September 2010, Al Viro wrote:
>>         There are several interesting issues in arch/*/*/*signal* (besides
>> shoggoths starting to show up when one reads that code) and I'd been crawling
>> through that area for the last few weeks.  Here are more or less common
>> issues; there are really arch-specific bugs (e.g. roothole on frv that
>> used to allow reading kernel memory by setting the right sa_handler), but
>> that's a separate story.
> 
> I still plan to make a counterpart to the asm-generic headers with an
> example architecture that new architectures can copy from. Signal handling
> is one of the areas that I have very limited understanding of. Did you
> encounter any architecture that basically gets signal handling right and that
> can serve as a positive example to others?
> 
> arch/tile/kernel/signal.c was the last one that got merged and I tried
> to direct the maintainer in the right direction as much as I could, but
> there are a lot of things I didn't know about.

(Sorry for the belated reply.)

I set aside this thread to look at when I had a minute, and I believe there
is just one of the signal issues present in the tile code.  The fix is to
reset regs->fault to something other than the "syscall" fault type when
exiting from do_signal(), so I'll submit that up for 2.6.37 shortly.

Otherwise I think tile is doing things right, though I admit, the signal
support is pretty deep magic generally.
-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: situation with signals

[Chris Metcalf]

On 9/24/2010 9:33 AM, Arnd Bergmann wrote:
> On Friday 24 September 2010, Al Viro wrote:
>>         There are several interesting issues in arch/*/*/*signal* (besides
>> shoggoths starting to show up when one reads that code) and I'd been crawling
>> through that area for the last few weeks.  Here are more or less common
>> issues; there are really arch-specific bugs (e.g. roothole on frv that
>> used to allow reading kernel memory by setting the right sa_handler), but
>> that's a separate story.
> 
> I still plan to make a counterpart to the asm-generic headers with an
> example architecture that new architectures can copy from. Signal handling
> is one of the areas that I have very limited understanding of. Did you
> encounter any architecture that basically gets signal handling right and that
> can serve as a positive example to others?
> 
> arch/tile/kernel/signal.c was the last one that got merged and I tried
> to direct the maintainer in the right direction as much as I could, but
> there are a lot of things I didn't know about.

(Sorry for the belated reply.)

I set aside this thread to look at when I had a minute, and I believe there
is just one of the signal issues present in the tile code.  The fix is to
reset regs->fault to something other than the "syscall" fault type when
exiting from do_signal(), so I'll submit that up for 2.6.37 shortly.

Otherwise I think tile is doing things right, though I admit, the signal
support is pretty deep magic generally.
-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: situation with signals

[Al Viro]

On Wed, Oct 27, 2010 at 05:02:10PM -0400, Chris Metcalf wrote:

> I set aside this thread to look at when I had a minute, and I believe there
> is just one of the signal issues present in the tile code.  The fix is to
> reset regs->fault to something other than the "syscall" fault type when
> exiting from do_signal(), so I'll submit that up for 2.6.37 shortly.
> 
> Otherwise I think tile is doing things right, though I admit, the signal
> support is pretty deep magic generally.

FWIW, I'd do that in handle_signal() when hitting a syscall restart.
BTW, is everything in your pt_regs safe to modify?  I.e. could bogus
values put there by sigreturn lead to something like kernel mode being
retained when you return from syscall or interesting flags being
set, etc.?

Re: situation with signals

[Chris Metcalf]

On 10/27/2010 5:37 PM, Al Viro wrote:
> On Wed, Oct 27, 2010 at 05:02:10PM -0400, Chris Metcalf wrote
>> I set aside this thread to look at when I had a minute, and I believe there
>> is just one of the signal issues present in the tile code.  The fix is to
>> reset regs->fault to something other than the "syscall" fault type when
>> exiting from do_signal(), so I'll submit that up for 2.6.37 shortly.
> FWIW, I'd do that in handle_signal() when hitting a syscall restart.

Right now I'm just doing it unconditionally in handle_signal()'s caller
whether or not I actually call handle_signal, to be paranoid:

@@ -353,11 +353,11 @@
                         * clear the TS_RESTORE_SIGMASK flag.
                         */
                        current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
                }

-               return;
+               goto done;
        }

        /* Did we come from a system call? */
        if (regs->faultnum == INT_SWINT_1) {
                /* Restart the system call - no handlers present */
@@ -381,10 +381,14 @@
        /* If there's no signal to deliver, just put the saved sigmask back. */
        if (current_thread_info()->status & TS_RESTORE_SIGMASK) {
                current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
                sigprocmask(SIG_SETMASK, &current->saved_sigmask, NULL);
        }
+
+done:
+       /* Avoid double syscall restart if there are nested signals. */
+       regs->faultnum = INT_SWINT_1_SIGRETURN;
 }
 

> BTW, is everything in your pt_regs safe to modify?

What an interesting observation. :-)  In fact, it would be possible to
overwrite the privilege level (the ex1 register) from within the signal
handler and then return to run arbitrary code at kernel PL.  I'll fix it.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: situation with signals

[Al Viro]

On Wed, Oct 27, 2010 at 06:51:12PM -0400, Chris Metcalf wrote:

> > BTW, is everything in your pt_regs safe to modify?
> 
> What an interesting observation. :-)  In fact, it would be possible to
> overwrite the privilege level (the ex1 register) from within the signal
> handler and then return to run arbitrary code at kernel PL.  I'll fix it.

Then you need to be careful with that in your ptrace as well (POKEUSR and
SETREGS).

[PATCH] arch/tile: correct double syscall restart for nested signals

[Chris Metcalf]

This change is modelled on similar fixes for other architectures.
The pt_regs "faultnum" member is set to the trap (fault) number that
caused us to enter the kernel, and is INT_SWINT_1 for the syscall software
interrupt.  We already supported a pseudo value, INT_SWINT_1_SIGRETURN,
that we used for the rt_sigreturn syscall; it avoided the case where
one signal was handled, then we "tail-called" to another handler.

This change avoids the similar case where we start to call one handler,
then are preempted into another handler when we start trying to run
the first handler.  We clear ->faultnum after calling handle_signal(),
and to be paranoid also in the case where there was no signal to deliver.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
 arch/tile/kernel/signal.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/tile/kernel/signal.c b/arch/tile/kernel/signal.c
index fb28e85..704ce0b 100644
--- a/arch/tile/kernel/signal.c
+++ b/arch/tile/kernel/signal.c
@@ -330,7 +330,7 @@ void do_signal(struct pt_regs *regs)
 			current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
 		}
 
-		return;
+		goto done;
 	}
 
 	/* Did we come from a system call? */
@@ -358,4 +358,8 @@ void do_signal(struct pt_regs *regs)
 		current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
 		sigprocmask(SIG_SETMASK, &current->saved_sigmask, NULL);
 	}
+
+done:
+	/* Avoid double syscall restart if there are nested signals. */
+	regs->faultnum = INT_SWINT_1_SIGRETURN;
 }
-- 
1.6.5.2

[PATCH] arch/tile: don't allow user code to set the PL via ptrace or signal return

[Chris Metcalf]

The kernel was allowing any component of the pt_regs to be updated either
by signal handlers writing to the stack, or by processes writing via
PTRACE_POKEUSR or PTRACE_SETREGS, which meant they could set their PL
up from 0 to 1 and get access to kernel code and data (or, in practice,
cause a kernel panic).  We now always reset the ex1 field, allowing the
user to set their ICS bit only.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
This was caught by Al Viro wondering out loud if tile properly handled
this case, and we didn't.  Thanks Al!

 arch/tile/kernel/ptrace.c |   37 +++++++++++++++++++++----------------
 arch/tile/kernel/signal.c |    3 +++
 2 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/arch/tile/kernel/ptrace.c b/arch/tile/kernel/ptrace.c
index 5b20c28..6d0cef8 100644
--- a/arch/tile/kernel/ptrace.c
+++ b/arch/tile/kernel/ptrace.c
@@ -49,10 +49,10 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 {
 	unsigned long __user *datap = (long __user __force *)data;
 	unsigned long tmp;
-	int i;
 	long ret = -EIO;
-	unsigned long *childregs;
 	char *childreg;
+	struct pt_regs copyregs;
+	int ex1_offset;
 
 	switch (request) {
 
@@ -79,6 +79,16 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 		if (addr < 0 || addr >= PTREGS_SIZE)
 			break;
 		childreg = (char *)task_pt_regs(child) + addr;
+
+		/* Guard against overwrites of the privilege level. */
+		ex1_offset = PTREGS_OFFSET_EX1;
+#if defined(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
+		if (is_compat_task())   /* point at low word */
+			ex1_offset += sizeof(compat_long_t);
+#endif
+		if (addr == ex1_offset)
+			data = PL_ICS_EX1(USER_PL, EX1_ICS(data));
+
 #ifdef CONFIG_COMPAT
 		if (is_compat_task()) {
 			if (addr & (sizeof(compat_long_t)-1))
@@ -95,24 +105,19 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 		break;
 
 	case PTRACE_GETREGS:  /* Get all registers from the child. */
-		if (!access_ok(VERIFY_WRITE, datap, PTREGS_SIZE))
-			break;
-		childregs = (long *)task_pt_regs(child);
-		for (i = 0; i < sizeof(struct pt_regs)/sizeof(long); ++i) {
-			ret = __put_user(childregs[i], &datap[i]);
-			if (ret != 0)
-				break;
+		if (copy_to_user(datap, task_pt_regs(child),
+				 sizeof(struct pt_regs)) == 0) {
+			ret = 0;
 		}
 		break;
 
 	case PTRACE_SETREGS:  /* Set all registers in the child. */
-		if (!access_ok(VERIFY_READ, datap, PTREGS_SIZE))
-			break;
-		childregs = (long *)task_pt_regs(child);
-		for (i = 0; i < sizeof(struct pt_regs)/sizeof(long); ++i) {
-			ret = __get_user(childregs[i], &datap[i]);
-			if (ret != 0)
-				break;
+		if (copy_from_user(&copyregs, datap,
+				   sizeof(struct pt_regs)) == 0) {
+			copyregs.ex1 =
+				PL_ICS_EX1(USER_PL, EX1_ICS(copyregs.ex1));
+			*task_pt_regs(child) = copyregs;
+			ret = 0;
 		}
 		break;
 
diff --git a/arch/tile/kernel/signal.c b/arch/tile/kernel/signal.c
index 704ce0b..687719d 100644
--- a/arch/tile/kernel/signal.c
+++ b/arch/tile/kernel/signal.c
@@ -71,6 +71,9 @@ int restore_sigcontext(struct pt_regs *regs,
 	for (i = 0; i < sizeof(struct pt_regs)/sizeof(long); ++i)
 		err |= __get_user(regs->regs[i], &sc->gregs[i]);
 
+	/* Ensure that the PL is always set to USER_PL. */
+	regs->ex1 = PL_ICS_EX1(USER_PL, EX1_ICS(regs->ex1));
+
 	regs->faultnum = INT_SWINT_1_SIGRETURN;
 
 	err |= __get_user(*pr0, &sc->gregs[0]);
-- 
1.6.5.2

[PATCH] arch/tile: don't allow user code to set the PL via ptrace or signal return

[Chris Metcalf]

The kernel was allowing any component of the pt_regs to be updated either
by signal handlers writing to the stack, or by processes writing via
PTRACE_POKEUSR or PTRACE_SETREGS, which meant they could set their PL
up from 0 to 1 and get access to kernel code and data (or, in practice,
cause a kernel panic).  We now always reset the ex1 field, allowing the
user to set their ICS bit only.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
This was caught by Al Viro wondering out loud if tile properly handled
this case, and we didn't.  Thanks Al!

 arch/tile/kernel/ptrace.c |   37 +++++++++++++++++++++----------------
 arch/tile/kernel/signal.c |    3 +++
 2 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/arch/tile/kernel/ptrace.c b/arch/tile/kernel/ptrace.c
index 5b20c28..6d0cef8 100644
--- a/arch/tile/kernel/ptrace.c
+++ b/arch/tile/kernel/ptrace.c
@@ -49,10 +49,10 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 {
 	unsigned long __user *datap = (long __user __force *)data;
 	unsigned long tmp;
-	int i;
 	long ret = -EIO;
-	unsigned long *childregs;
 	char *childreg;
+	struct pt_regs copyregs;
+	int ex1_offset;
 
 	switch (request) {
 
@@ -79,6 +79,16 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 		if (addr < 0 || addr >= PTREGS_SIZE)
 			break;
 		childreg = (char *)task_pt_regs(child) + addr;
+
+		/* Guard against overwrites of the privilege level. */
+		ex1_offset = PTREGS_OFFSET_EX1;
+#if defined(CONFIG_COMPAT) && defined(__BIG_ENDIAN)
+		if (is_compat_task())   /* point at low word */
+			ex1_offset += sizeof(compat_long_t);
+#endif
+		if (addr == ex1_offset)
+			data = PL_ICS_EX1(USER_PL, EX1_ICS(data));
+
 #ifdef CONFIG_COMPAT
 		if (is_compat_task()) {
 			if (addr & (sizeof(compat_long_t)-1))
@@ -95,24 +105,19 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 		break;
 
 	case PTRACE_GETREGS:  /* Get all registers from the child. */
-		if (!access_ok(VERIFY_WRITE, datap, PTREGS_SIZE))
-			break;
-		childregs = (long *)task_pt_regs(child);
-		for (i = 0; i < sizeof(struct pt_regs)/sizeof(long); ++i) {
-			ret = __put_user(childregs[i], &datap[i]);
-			if (ret != 0)
-				break;
+		if (copy_to_user(datap, task_pt_regs(child),
+				 sizeof(struct pt_regs)) == 0) {
+			ret = 0;
 		}
 		break;
 
 	case PTRACE_SETREGS:  /* Set all registers in the child. */
-		if (!access_ok(VERIFY_READ, datap, PTREGS_SIZE))
-			break;
-		childregs = (long *)task_pt_regs(child);
-		for (i = 0; i < sizeof(struct pt_regs)/sizeof(long); ++i) {
-			ret = __get_user(childregs[i], &datap[i]);
-			if (ret != 0)
-				break;
+		if (copy_from_user(&copyregs, datap,
+				   sizeof(struct pt_regs)) == 0) {
+			copyregs.ex1 =
+				PL_ICS_EX1(USER_PL, EX1_ICS(copyregs.ex1));
+			*task_pt_regs(child) = copyregs;
+			ret = 0;
 		}
 		break;
 
diff --git a/arch/tile/kernel/signal.c b/arch/tile/kernel/signal.c
index 704ce0b..687719d 100644
--- a/arch/tile/kernel/signal.c
+++ b/arch/tile/kernel/signal.c
@@ -71,6 +71,9 @@ int restore_sigcontext(struct pt_regs *regs,
 	for (i = 0; i < sizeof(struct pt_regs)/sizeof(long); ++i)
 		err |= __get_user(regs->regs[i], &sc->gregs[i]);
 
+	/* Ensure that the PL is always set to USER_PL. */
+	regs->ex1 = PL_ICS_EX1(USER_PL, EX1_ICS(regs->ex1));
+
 	regs->faultnum = INT_SWINT_1_SIGRETURN;
 
 	err |= __get_user(*pr0, &sc->gregs[0]);
-- 
1.6.5.2