Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Mikael Pettersson]

On Thu, 4 Oct 2007 21:47:30 +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 04 Oct 2007 21:33:12 +0900
> Shi Weihua <shiwh@cn.fujitsu.com> wrote:
> 
> > KAMEZAWA Hiroyuki wrote::
> > > On Thu, 04 Oct 2007 20:56:14 +0900
> > > Shi Weihua <shiwh@cn.fujitsu.com> wrote:
> > > 
> > >> 	stack.ss_sp = addr + pagesize;
> > >> 	stack.ss_flags = 0;
> > >> 	stack.ss_size = pagesize;
> > > Here is bad. 
> > > stack,ss_sp = addr;
> > > stack.ss_flags = 0;
> > > stack.ss_size = pagesize * 2;
> > [What the test code want to do]
> > addr+pagesize*2 - addr+pagesize	 -> sigaltstack
> > addr+pagesize	- addr		 -> protected region
> > The code want to catch overflow when esp enter the protected region.
> > 
> You have to protect the top of *registered* sigaltstack.
> The reason of wraparound is %esp will be set to the bottom of sigaltstack
> if it is not on sigaltstack area when signaled.
> What you have to do is protect the top of registerd sigaltstack.
> If %esp is in the range of registerd sigaltstack at SEGV, wraparound
> will stop.

Exactly right. You mprotect or munmap the end of the altstack,
not the area beyond it.

/Mikael

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Shi Weihua]

Mikael Pettersson wrote::
> On Thu, 4 Oct 2007 21:47:30 +0900, KAMEZAWA Hiroyuki wrote:
>> On Thu, 04 Oct 2007 21:33:12 +0900
>> Shi Weihua <shiwh@cn.fujitsu.com> wrote:
>>
>>> KAMEZAWA Hiroyuki wrote::
>>>> On Thu, 04 Oct 2007 20:56:14 +0900
>>>> Shi Weihua <shiwh@cn.fujitsu.com> wrote:
>>>>
>>>>> 	stack.ss_sp = addr + pagesize;
>>>>> 	stack.ss_flags = 0;
>>>>> 	stack.ss_size = pagesize;
>>>> Here is bad. 
>>>> stack,ss_sp = addr;
>>>> stack.ss_flags = 0;
>>>> stack.ss_size = pagesize * 2;
>>> [What the test code want to do]
>>> addr+pagesize*2 - addr+pagesize	 -> sigaltstack
>>> addr+pagesize	- addr		 -> protected region
>>> The code want to catch overflow when esp enter the protected region.
>>>
>> You have to protect the top of *registered* sigaltstack.
>> The reason of wraparound is %esp will be set to the bottom of sigaltstack
>> if it is not on sigaltstack area when signaled.
>> What you have to do is protect the top of registerd sigaltstack.
>> If %esp is in the range of registerd sigaltstack at SEGV, wraparound
>> will stop.
> 
> Exactly right. You mprotect or munmap the end of the altstack,
> not the area beyond it.
So we tell users "Even if you protectted half of mmap's space, but you must to register all space to 
kernel. " ?

The image about my test code's result:
               No patch        Patched
┌───────────┐
│           │← 1 ┌ ← 3          ← 1
│    A      │    │(wraparound)
│           │    │
│           │← 2 │              ← 2
│           │    │
├───────────┤    │
│▒▒▒▒▒▒▒▒▒▒▒│← 3 ┘              ← 3
│▒▒▒▒B▒▒▒▒▒▒│                  (caught)
│▒protected▒│
│▒▒▒▒▒▒▒▒▒▒▒│
│▒▒▒▒▒▒▒▒▒▒▒│
└───────────┘
A+B  mmap's space
A    sigaltstack
B    protectted

I agree that if register A+B to kernel, the wraparound will stop.
But if register A to kernel, why not kernel do something?

Thanks
Shi Weihua
> 
> /Mikael
> 
> 
> 

Re: Fw: Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Shi Weihua]

Roland McGrath wrote::
> cf http://lkml.org/lkml/2007/10/3/41
> 
> To summarize: on Linux, SA_ONSTACK decides whether you are already on the
> signal stack based on the value of the SP at the time of a signal.  If
> you are not already inside the range, you are not "on the signal stack"
> and so the new signal handler frame starts over at the base of the signal
> stack.
> 
> sigaltstack (and sigstack before it) was invented in BSD.  There, the
> SA_ONSTACK behavior has always been different.  It uses a kernel state
> flag to decide, rather than the SP value.  When you first take an
> SA_ONSTACK signal and switch to the alternate signal stack, it sets the
> SS_ONSTACK flag in the thread's sigaltstack state in the kernel.
> Thereafter you are "on the signal stack" and don't switch SP before
> pushing a handler frame no matter what the SP value is.  Only when you
> sigreturn from the original handler context do you clear the SS_ONSTACK
> flag so that a new handler frame will start over at the base of the
> alternate signal stack.
> 
> The undesireable effect of the Linux behavior is that an overflow of the
> alternate signal stack can not only go undetected, but lead to a ring
> buffer effect of clobbering the original handler frame at the base of the
> signal stack for each successive signal that comes just after the
> overflow.  This is what Shi Weihua's test case demonstrates.  Normally
> this does not come up because of the signal mask, but the test case uses
> SA_NODEFER for its SIGSEGV handler.
> 
> The other subtle part of the existing Linux semantics is that a simple
> longjmp out of a signal handler serves to take you off the signal stack
> in a safe and reliable fashion without having used sigreturn (nor having
> just returned from the handler normally, which means the same).  After
> the longjmp (or even informal stack switching not via any proper libc or
> kernel interface), the alternate signal stack stands ready to be used
> again.
> 
> A paranoid program would allocate a PROT_NONE red zone around its
> alternate signal stack.  Then a small overflow would trigger a SIGSEGV in
> handler setup, and be fatal (core dump) whether or not SIGSEGV is
> blocked.  As with thread stack red zones, that cannot catch all overflows
> (or underflows).  e.g., a local array as large as page size allocated in
> a function called from a handler, but not actually touched before more
> calls push more stack, could cause an overflow that silently pushes into
> some unrelated allocated pages.
> 
> The BSD behavior does not do anything in particular about overflow.  But
> it does at least avoid the wraparound or "ring buffer effect", so you'll
> just get a straightforward all-out overflow down your address space past
> the low end of the alternate signal stack.  I don't know what the BSD
> behavior is for longjmp out of an SA_ONSTACK handler.
> 
> The POSIX wording relating to sigaltstack is pretty minimal.  I don't
> think it speaks to this issue one way or another.  (The program that
> overflows its stack is clearly in undefined behavior territory of one
> sort or another anyhow.)
> 
> Given the longjmp issue and the potential for highly subtle complications
> in existing programs relying on this in arcane ways deep in their code, I
> am very dubious about changing the behavior to the BSD style persistent
> flag.  I think Shi Weihua's patches have a similar effect by tracking the
> SP used in the last handler setup.
> 
> I think it would be sensible for the signal handler setup code to detect
> when it would itself be causing a stack overflow.  Maybe something like
> the following patch (untested).  This issue exists in the same way on all
> machines, so ideally they would all do a similar check.  
> 
> When it's the handler function itself or its callees that cause the
> overflow, rather than the signal handler frame setup alone crossing the
> boundary, this still won't help.  But I don't see any way to distinguish
> that from the valid longjmp case.

Thank you for your detailed explanation and patch.
I tested your patch, unfortunately it can not stop all kinds of overflow.

The esp is a user space pointer, so the user can move it. 
For example, the user defines "int i[1000];" in  the handler function.
Please run the following test program, and pay attention to "int i[1000];".
-------------------------------------------------------------------------
#include <stdio.h>
#include <signal.h>
#include <stdlib.h>
#include <string.h>

volatile int counter = 0;

#ifdef __i386__
void print_esp()
{
	unsigned long esp;
	__asm__ __volatile__("movl %%esp, %0":"=g"(esp));

	printf("esp = 0x%08lx\n", esp);
}
#endif

void segv_handler()
{
#ifdef __i386__
	print_esp();
#endif

	int *c = NULL;
	counter++;
	printf("%d\n", counter);

	int i[1000];	// <- Pay attention here.

	*c = 1;			// SEGV
}

int main()
{
	int *c = NULL;
	char *s = malloc(SIGSTKSZ);
	stack_t stack;
	struct sigaction action;

	memset(s, 0, SIGSTKSZ);
	stack.ss_sp = s;
	stack.ss_flags = 0;
	stack.ss_size = SIGSTKSZ;
	int error = sigaltstack(&stack, NULL);
	if (error) {
		printf("Failed to use sigaltstack!\n");
		return -1;
	}

	memset(&action, 0, sizeof(action));
	action.sa_handler = segv_handler;
	action.sa_flags = SA_ONSTACK | SA_NODEFER;

	sigemptyset(&action.sa_mask);

	sigaction(SIGSEGV, &action, NULL);

	*c = 0;			//SEGV

	if (!s)
		free(s);

	return 0;
}
-------------------------------------------------------------------------

So, the patch I posted is still needed 
Surely, adding a variable to sched.h is not a good idea. 
Could you tell me a better place to store the previous esp?

Here is a new patch rebased on 2.6.24-rc3-git2.

Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com> 

--- /shiwh-tmp/linux-2.6.24-rc3-git2.orig/arch/x86/kernel/signal_32.c	2007-11-28 09:15:34.000000000 +0800
+++ /shiwh-tmp/linux-2.6.24-rc3-git2/arch/x86/kernel/signal_32.c	2007-11-28 13:19:13.000000000 +0800
@@ -297,7 +297,8 @@ get_sigframe(struct k_sigaction *ka, str
 
 	/* This is the X/Open sanctioned signal stack switching.  */
 	if (ka->sa.sa_flags & SA_ONSTACK) {
-		if (sas_ss_flags(esp) == 0)
+		if (sas_ss_flags(esp) == 0 &&
+		    !on_sig_stack(current->pre_ss_sp))
 			esp = current->sas_ss_sp + current->sas_ss_size;
 	}
 
@@ -330,9 +331,15 @@ static int setup_frame(int sig, struct k
 
 	frame = get_sigframe(ka, regs, sizeof(*frame));
 
+	if ((ka->sa.sa_flags & SA_ONSTACK) &&
+	    !sas_ss_flags((unsigned long)frame))
+		goto give_sigsegv;
+
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
 		goto give_sigsegv;
 
+	current->pre_ss_sp = (unsigned long)frame;
+
 	usig = current_thread_info()->exec_domain
 		&& current_thread_info()->exec_domain->signal_invmap
 		&& sig < 32
@@ -422,9 +429,15 @@ static int setup_rt_frame(int sig, struc
 
 	frame = get_sigframe(ka, regs, sizeof(*frame));
 
+	if ((ka->sa.sa_flags & SA_ONSTACK) &&
+	    !sas_ss_flags((unsigned long)frame))
+		goto give_sigsegv;
+
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
 		goto give_sigsegv;
 
+	current->pre_ss_sp = (unsigned long)frame;
+
 	usig = current_thread_info()->exec_domain
 		&& current_thread_info()->exec_domain->signal_invmap
 		&& sig < 32
--- /shiwh-tmp/linux-2.6.24-rc3-git2.orig/include/linux/sched.h	2007-11-28 09:15:52.000000000 +0800
+++ /shiwh-tmp/linux-2.6.24-rc3-git2/include/linux/sched.h	2007-11-28 13:20:04.000000000 +0800
@@ -1059,6 +1059,7 @@ struct task_struct {
 	struct sigpending pending;
 
 	unsigned long sas_ss_sp;
+	unsigned long pre_ss_sp;
 	size_t sas_ss_size;
 	int (*notifier)(void *priv);
 	void *notifier_data;
--- /shiwh-tmp/linux-2.6.24-rc3-git2.orig/kernel/signal.c	2007-11-28 09:15:52.000000000 +0800
+++ /shiwh-tmp/linux-2.6.24-rc3-git2/kernel/signal.c	2007-11-28 13:20:54.000000000 +0800
@@ -2403,6 +2403,9 @@ do_sigaltstack (const stack_t __user *us
 
 		current->sas_ss_sp = (unsigned long) ss_sp;
 		current->sas_ss_size = ss_size;
+
+		/* reset previous sp */
+		current->pre_ss_sp = 0;
 	}
 
 	if (uoss) {

Thanks,
Shi Weihua

> 
> 
> Thanks,
> Roland
> 
> ---
> 
> diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
> index d58d455..0000000 100644  
> --- a/arch/x86/kernel/signal_32.c
> +++ b/arch/x86/kernel/signal_32.c
> @@ -295,6 +295,13 @@ get_sigframe(struct k_sigaction *ka, str
>  	/* Default to using normal stack */
>  	esp = regs->esp;
>  
> +	/*
> +	 * If we are on the alternate signal stack and would overflow it, don't.
> +	 * Return an always-bogus address instead so we will die with SIGSEGV.
> +	 */
> +	if (on_sig_stack(esp) && !likely(on_sig_stack(esp - frame_size)))
> +		return (void __user *) -1L;
> +
>  	/* This is the X/Open sanctioned signal stack switching.  */
>  	if (ka->sa.sa_flags & SA_ONSTACK) {
>  		if (sas_ss_flags(esp) == 0)

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Ingo Molnar]

* Shi Weihua <shiwh@cn.fujitsu.com> wrote:

> > When it's the handler function itself or its callees that cause the 
> > overflow, rather than the signal handler frame setup alone crossing 
> > the boundary, this still won't help.  But I don't see any way to 
> > distinguish that from the valid longjmp case.
> 
> Thank you for your detailed explanation and patch. I tested your 
> patch, unfortunately it can not stop all kinds of overflow.
[...]
> So, the patch I posted is still needed 

thanks, i've picked up your fix for x86.git, for 2.6.25 merging.

> Surely, adding a variable to sched.h is not a good idea. 
> Could you tell me a better place to store the previous esp?

i think sched.h is ok - it has a sas_ss_sp field already. Alternatively, 
if we only want this in x86, we could put it into the thread_struct - 
but i think eventually other architectures would want to use this too, 
right?

	Ingo

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Roland McGrath]

> > Thank you for your detailed explanation and patch. I tested your 
> > patch, unfortunately it can not stop all kinds of overflow.
> [...]
> > So, the patch I posted is still needed 
> 
> thanks, i've picked up your fix for x86.git, for 2.6.25 merging.

I just explained that not all overflows would be caught and that doing so
would violate the semantics of e.g. longjmp.  I don't see how the patch
you've included now doesn't still have all those problems.  I think it's wrong.


Thanks,
Roland

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Ingo Molnar]

* Roland McGrath <roland@redhat.com> wrote:

> > > Thank you for your detailed explanation and patch. I tested your 
> > > patch, unfortunately it can not stop all kinds of overflow.
> > [...]
> > > So, the patch I posted is still needed 
> > 
> > thanks, i've picked up your fix for x86.git, for 2.6.25 merging.
> 
> I just explained that not all overflows would be caught and that doing 
> so would violate the semantics of e.g. longjmp.  I don't see how the 
> patch you've included now doesn't still have all those problems.  I 
> think it's wrong.

ok, i've dropped it and reinstated your original fix - i missed that 
detail.

	Ingo

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Shi Weihua]

Roland McGrath wrote::
>>> Thank you for your detailed explanation and patch. I tested your 
>>> patch, unfortunately it can not stop all kinds of overflow.
>> [...]
>>> So, the patch I posted is still needed 
>> thanks, i've picked up your fix for x86.git, for 2.6.25 merging.
> 
> I just explained that not all overflows would be caught and that doing so
> would violate the semantics of e.g. longjmp.  I don't see how the patch
> you've included now doesn't still have all those problems.  I think it's wrong.
> 

I am sorry, i don't understand how this is related to the semantics of e.g. longjmp.
But, i am sure my patch solves all overflows. Ingo's patch can't catch the overflow 
which is caught by "int i[1000];" in the handler function.

Do you have more idea for me? Thanks.

Regards
Shi Weihua

By the way, I think Ingo's patch can be improved. 

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com>

---
--- linux-2.6.24-rc4-git1.orig/arch/x86/kernel/signal_32.c	2007-12-04 12:26:10.000000000 +0800
+++ linux-2.6.24-rc4-git1/arch/x86/kernel/signal_32.c	2007-12-05 11:13:56.000000000 +0800
@@ -297,8 +297,17 @@ get_sigframe(struct k_sigaction *ka, str
 
 	/* This is the X/Open sanctioned signal stack switching.  */
 	if (ka->sa.sa_flags & SA_ONSTACK) {
-		if (sas_ss_flags(esp) == 0)
+		int onstack = sas_ss_flags(esp);
+		if (onstack == 0)
 			esp = current->sas_ss_sp + current->sas_ss_size;
+		else if(onstack == SS_ONSTACK){
+			/*
+			 * If we are on the alternate signal stack and would overflow it, don't.
+			 * Return an always-bogus address instead so we will die with SIGSEGV.
+			 */
+			if (!likely(on_sig_stack(esp - frame_size)))
+				return (void __user *) -1L;
+		}
 	}
 
 	/* This is the legacy signal stack switching. */

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Roland McGrath]

> I am sorry, i don't understand how this is related to the semantics of
> e.g. longjmp.  But, i am sure my patch solves all overflows. Ingo's patch
> can't catch the overflow which is caught by "int i[1000];" in the handler
> function.

Please read the long explanation I wrote, which Ingo forwarded.

Thanks,
Roland

Re: Fw: Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Roland McGrath]

cf http://lkml.org/lkml/2007/10/3/41

To summarize: on Linux, SA_ONSTACK decides whether you are already on the
signal stack based on the value of the SP at the time of a signal.  If
you are not already inside the range, you are not "on the signal stack"
and so the new signal handler frame starts over at the base of the signal
stack.

sigaltstack (and sigstack before it) was invented in BSD.  There, the
SA_ONSTACK behavior has always been different.  It uses a kernel state
flag to decide, rather than the SP value.  When you first take an
SA_ONSTACK signal and switch to the alternate signal stack, it sets the
SS_ONSTACK flag in the thread's sigaltstack state in the kernel.
Thereafter you are "on the signal stack" and don't switch SP before
pushing a handler frame no matter what the SP value is.  Only when you
sigreturn from the original handler context do you clear the SS_ONSTACK
flag so that a new handler frame will start over at the base of the
alternate signal stack.

The undesireable effect of the Linux behavior is that an overflow of the
alternate signal stack can not only go undetected, but lead to a ring
buffer effect of clobbering the original handler frame at the base of the
signal stack for each successive signal that comes just after the
overflow.  This is what Shi Weihua's test case demonstrates.  Normally
this does not come up because of the signal mask, but the test case uses
SA_NODEFER for its SIGSEGV handler.

The other subtle part of the existing Linux semantics is that a simple
longjmp out of a signal handler serves to take you off the signal stack
in a safe and reliable fashion without having used sigreturn (nor having
just returned from the handler normally, which means the same).  After
the longjmp (or even informal stack switching not via any proper libc or
kernel interface), the alternate signal stack stands ready to be used
again.

A paranoid program would allocate a PROT_NONE red zone around its
alternate signal stack.  Then a small overflow would trigger a SIGSEGV in
handler setup, and be fatal (core dump) whether or not SIGSEGV is
blocked.  As with thread stack red zones, that cannot catch all overflows
(or underflows).  e.g., a local array as large as page size allocated in
a function called from a handler, but not actually touched before more
calls push more stack, could cause an overflow that silently pushes into
some unrelated allocated pages.

The BSD behavior does not do anything in particular about overflow.  But
it does at least avoid the wraparound or "ring buffer effect", so you'll
just get a straightforward all-out overflow down your address space past
the low end of the alternate signal stack.  I don't know what the BSD
behavior is for longjmp out of an SA_ONSTACK handler.

The POSIX wording relating to sigaltstack is pretty minimal.  I don't
think it speaks to this issue one way or another.  (The program that
overflows its stack is clearly in undefined behavior territory of one
sort or another anyhow.)

Given the longjmp issue and the potential for highly subtle complications
in existing programs relying on this in arcane ways deep in their code, I
am very dubious about changing the behavior to the BSD style persistent
flag.  I think Shi Weihua's patches have a similar effect by tracking the
SP used in the last handler setup.

I think it would be sensible for the signal handler setup code to detect
when it would itself be causing a stack overflow.  Maybe something like
the following patch (untested).  This issue exists in the same way on all
machines, so ideally they would all do a similar check.  

When it's the handler function itself or its callees that cause the
overflow, rather than the signal handler frame setup alone crossing the
boundary, this still won't help.  But I don't see any way to distinguish
that from the valid longjmp case.


Thanks,
Roland

---

diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
index d58d455..0000000 100644  
--- a/arch/x86/kernel/signal_32.c
+++ b/arch/x86/kernel/signal_32.c
@@ -295,6 +295,13 @@ get_sigframe(struct k_sigaction *ka, str
 	/* Default to using normal stack */
 	esp = regs->esp;
 
+	/*
+	 * If we are on the alternate signal stack and would overflow it, don't.
+	 * Return an always-bogus address instead so we will die with SIGSEGV.
+	 */
+	if (on_sig_stack(esp) && !likely(on_sig_stack(esp - frame_size)))
+		return (void __user *) -1L;
+
 	/* This is the X/Open sanctioned signal stack switching.  */
 	if (ka->sa.sa_flags & SA_ONSTACK) {
 		if (sas_ss_flags(esp) == 0)

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Arjan van de Ven]

On Mon, 26 Nov 2007 19:02:22 -0800 (PST)
Roland McGrath <roland@redhat.com> wrote:

> cf http://lkml.org/lkml/2007/10/3/41
> 
> To summarize: on Linux, SA_ONSTACK decides whether you are already on
> the signal stack based on the value of the SP at the time of a
> signal.  If you are not already inside the range, you are not "on the
> signal stack" and so the new signal handler frame starts over at the
> base of the signal stack.
> 
> sigaltstack (and sigstack before it) was invented in BSD.  There, the
> SA_ONSTACK behavior has always been different.  It uses a kernel state
> flag to decide, rather than the SP value.  When you first take an
> SA_ONSTACK signal and switch to the alternate signal stack, it sets
> the SS_ONSTACK flag in the thread's sigaltstack state in the kernel.
> Thereafter you are "on the signal stack" and don't switch SP before
> pushing a handler frame no matter what the SP value is.  Only when you
> sigreturn from the original handler context do you clear the
> SS_ONSTACK flag so that a new handler frame will start over at the
> base of the alternate signal stack.
> 
> The undesireable effect of the Linux behavior is that an overflow of
> the alternate signal stack can not only go undetected, but lead to a
> ring buffer effect of clobbering the original handler frame at the
> base of the signal stack for each successive signal that comes just
> after the overflow.  This is what Shi Weihua's test case
> demonstrates.  Normally this does not come up because of the signal
> mask, but the test case uses SA_NODEFER for its SIGSEGV handler.
> 
> The other subtle part of the existing Linux semantics is that a simple
> longjmp out of a signal handler serves to take you off the signal
> stack in a safe and reliable fashion without having used sigreturn
> (nor having just returned from the handler normally, which means the
> same).  After the longjmp (or even informal stack switching not via
> any proper libc or kernel interface), the alternate signal stack
> stands ready to be used again.
> 
> A paranoid program would allocate a PROT_NONE red zone around its
> alternate signal stack.  Then a small overflow would trigger a
> SIGSEGV in handler setup, and be fatal (core dump) whether or not
> SIGSEGV is blocked.  As with thread stack red zones, that cannot
> catch all overflows (or underflows).  e.g., a local array as large as
> page size allocated in a function called from a handler, but not
> actually touched before more calls push more stack, could cause an
> overflow that silently pushes into some unrelated allocated pages.
> 
> The BSD behavior does not do anything in particular about overflow.
> But it does at least avoid the wraparound or "ring buffer effect", so
> you'll just get a straightforward all-out overflow down your address
> space past the low end of the alternate signal stack.  I don't know
> what the BSD behavior is for longjmp out of an SA_ONSTACK handler.
> 
> The POSIX wording relating to sigaltstack is pretty minimal.  I don't
> think it speaks to this issue one way or another.  (The program that
> overflows its stack is clearly in undefined behavior territory of one
> sort or another anyhow.)
> 
> Given the longjmp issue and the potential for highly subtle
> complications in existing programs relying on this in arcane ways
> deep in their code, I am very dubious about changing the behavior to
> the BSD style persistent flag.  I think Shi Weihua's patches have a
> similar effect by tracking the SP used in the last handler setup.
> 
> I think it would be sensible for the signal handler setup code to
> detect when it would itself be causing a stack overflow.  Maybe
> something like the following patch (untested).  This issue exists in
> the same way on all machines, so ideally they would all do a similar
> check.  
> 
> When it's the handler function itself or its callees that cause the
> overflow, rather than the signal handler frame setup alone crossing
> the boundary, this still won't help.  But I don't see any way to
> distinguish that from the valid longjmp case.
> 


we probably should also make sure userspace has at least a little bit
of stack space for itself, say 2Kb or 4Kb, not just "the kernel puts
you right at the edge"....

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Mikael Pettersson]

On Wed, 3 Oct 2007 22:20:46 +0900, KAMEZAWA Hiroyuki wrote:
> On Wed, 3 Oct 2007 21:40:29 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> 
> > On Wed, 3 Oct 2007 14:20:07 +0200 (MEST)
> > Mikael Pettersson <mikpe@it.uu.se> wrote:
> > 
> > > What I don't agree with is the logic itself:
> > > - You only catch altstack overflow caused by the kernel pushing
> > >   a sigframe. You don't catch overflow caused by the user-space
> > >   signal handler pushing its own stack frame after the sigframe.
> > > - SUSv3 specifies the effect of altstack overflow as "undefined".
> > > - The overflow problem can be solved in user-space: allocate the
> > >   altstack with mmap(), then mprotect() the lowest page to prevent
> > >   accesses to it. Any overflow into it, by the kernel's signal
> > >   delivery code or by the user-space signal handler, will be caught.
> > > 
> > > So this patch gets a NAK from me.
> > > 
> > 
> > I can understand what you say, but a program which meets this problem
> > cannot be debugged ;(
> > 
> > gdb just shows infinit loop of function frames and origignal signal frame
> > which includes the most important information is overwritten.
> > 
> there is a difference among user's stack overflow and kernel's.
>  - user's stack overflow just breaks memory next to stack frame.
>  - kernel's altstack overflow, which this patch tries to fix, breaks
>    the bottom of altstack  bacause %esp goes back to the bottom
>    of ths altstack when it exceeds altstack range.
>    This behavior overwrite orignail stack frame and shows  infinit loop
>    of function call to gdb and never stop with 100% cpu usage.

The proposed kernel signal delivery patch only handles the case
where the /sigframe/ ends up overlapping the end of the altstack.
If the sigframe remains within the altstack boundaries but the
user-space signal handler adds an /ordinary stack frame/ that
moves SP beyond the altstack limit, then the kernel patch solves
nothing and recursive signals will cause altstack wraparound.

On the other hand, the user-space technique of making the lowest
page(s) in the altstack inaccessible handles both cases of overflow.

/Mikael

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[KAMEZAWA Hiroyuki]

On Wed, 3 Oct 2007 15:46:32 +0200 (MEST)
Mikael Pettersson <mikpe@it.uu.se> wrote:
> The proposed kernel signal delivery patch only handles the case
> where the /sigframe/ ends up overlapping the end of the altstack.
> If the sigframe remains within the altstack boundaries but the
> user-space signal handler adds an /ordinary stack frame/ that
> moves SP beyond the altstack limit, then the kernel patch solves
> nothing and recursive signals will cause altstack wraparound.
> 
> On the other hand, the user-space technique of making the lowest
> page(s) in the altstack inaccessible handles both cases of overflow.
> 
Hmm, okay. Then, this fix is not enough. I see.
I'll consider how to eduacate users.

Thanks,
-Kame

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Shi Weihua]

KAMEZAWA Hiroyuki wrote::
> On Wed, 3 Oct 2007 15:46:32 +0200 (MEST)
> Mikael Pettersson <mikpe@it.uu.se> wrote:
>> The proposed kernel signal delivery patch only handles the case
>> where the /sigframe/ ends up overlapping the end of the altstack.
>> If the sigframe remains within the altstack boundaries but the
>> user-space signal handler adds an /ordinary stack frame/ that
>> moves SP beyond the altstack limit, then the kernel patch solves
>> nothing and recursive signals will cause altstack wraparound.
>>
>> On the other hand, the user-space technique of making the lowest
>> page(s) in the altstack inaccessible handles both cases of overflow.
>>
> Hmm, okay. Then, this fix is not enough. I see.
> I'll consider how to eduacate users.

Excuse me. What will Mr.Kamezawa educate users? How to use sigaltstack?

Following is about using mmap/mprotect. In the previous mail(just now), I have said the same 
thing.Now I say it again in detailed.

Mikael has told us user'd better to use mmap/mprotect. So I tried to use mmap/mprotect in my test code.

I want to mprotect() the place from mid to low, and hope it stop the overflow.
high
|
|	enable to access
|
mid
|
|	disable to access
|
low
I hope the kernel catch it when the esp beyond the boundaries(mid) in user-space.

But the altstack wraparound still occurs.
begin = 0xb7fec000
end   = 0xb7fee000
esp = 0xb7fedce0
1
esp = 0xb7fed9e0
2
esp = 0xb7fed6e0
3
esp = 0xb7fedce0  <- wraparound
4
...

Fortunately, when I reuse the patch, wraparound disappeared. Even if I activate the code *1(please 
refer to the following test code).
So I think we need the patch, in the same time,we advice the user it's better to use mmap/mprotect.

-----------------------------------------------------------
#include <stdio.h>
#include <signal.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
#include <unistd.h>

#define die(msg) do { perror(msg); exit(EXIT_FAILURE); } while (0)
volatile int counter = 0;

#ifdef __i386__
void print_esp()
{
	unsigned long esp;
	__asm__ __volatile__("movl %%esp, %0":"=g"(esp));

	printf("esp = 0x%08lx\n", esp);
}
#endif

static void segv_handler()
{
#ifdef __i386__
	print_esp();
#endif

//	int i[1000];	//*1
	
	int *c = NULL;
	counter++;
	printf("%d\n", counter);

	*c = 1;			// SEGV
}

int main()
{
	int *c = NULL;
	int pagesize;
	char *addr;
	stack_t stack;
	struct sigaction action;

	pagesize = sysconf(_SC_PAGE_SIZE);
	if (pagesize == -1)
		die("sysconf");

	addr = mmap(NULL, pagesize * 2, PROT_READ | PROT_WRITE | PROT_EXEC,
		    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	if (addr == MAP_FAILED)
		die("mmap");
	
	printf("begin = 0x%08lx\n", addr);
	printf("end   = 0x%08lx\n", addr + pagesize * 2);
	
	if (mprotect(addr, pagesize, PROT_NONE) == -1)
		die("mprotect");

	stack.ss_sp = addr + pagesize;
	stack.ss_flags = 0;
	stack.ss_size = pagesize;
	int error = sigaltstack(&stack, NULL);
	if (error) {
		printf("Failed to use sigaltstack!\n");
		return -1;
	}

	memset(&action, 0, sizeof(action));
	action.sa_handler = segv_handler;
	action.sa_flags = SA_ONSTACK | SA_NODEFER;

	sigemptyset(&action.sa_mask);

	sigaction(SIGSEGV, &action, NULL);

	*c = 0;			//SEGV

	return 0;
}
-----------------------------------------------------------

Any suggestion?

Thanks
Shi Weihua

> 
> Thanks,
> -Kame
> 
> 
> 

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[KAMEZAWA Hiroyuki]

On Thu, 04 Oct 2007 20:56:14 +0900
Shi Weihua <shiwh@cn.fujitsu.com> wrote:

> 	stack.ss_sp = addr + pagesize;
> 	stack.ss_flags = 0;
> 	stack.ss_size = pagesize;
Here is bad. 
stack,ss_sp = addr;
stack.ss_flags = 0;
stack.ss_size = pagesize * 2;

cheers.
-Kame

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Shi Weihua]

KAMEZAWA Hiroyuki wrote::
> On Thu, 04 Oct 2007 20:56:14 +0900
> Shi Weihua <shiwh@cn.fujitsu.com> wrote:
> 
>> 	stack.ss_sp = addr + pagesize;
>> 	stack.ss_flags = 0;
>> 	stack.ss_size = pagesize;
> Here is bad. 
> stack,ss_sp = addr;
> stack.ss_flags = 0;
> stack.ss_size = pagesize * 2;
[What the test code want to do]
addr+pagesize*2 - addr+pagesize	 -> sigaltstack
addr+pagesize	- addr		 -> protected region
The code want to catch overflow when esp enter the protected region.

But it failed ...
> 
> cheers.
> -Kame
> 
> 
> 

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[KAMEZAWA Hiroyuki]

On Thu, 04 Oct 2007 21:33:12 +0900
Shi Weihua <shiwh@cn.fujitsu.com> wrote:

> KAMEZAWA Hiroyuki wrote::
> > On Thu, 04 Oct 2007 20:56:14 +0900
> > Shi Weihua <shiwh@cn.fujitsu.com> wrote:
> > 
> >> 	stack.ss_sp = addr + pagesize;
> >> 	stack.ss_flags = 0;
> >> 	stack.ss_size = pagesize;
> > Here is bad. 
> > stack,ss_sp = addr;
> > stack.ss_flags = 0;
> > stack.ss_size = pagesize * 2;
> [What the test code want to do]
> addr+pagesize*2 - addr+pagesize	 -> sigaltstack
> addr+pagesize	- addr		 -> protected region
> The code want to catch overflow when esp enter the protected region.
> 
You have to protect the top of *registered* sigaltstack.
The reason of wraparound is %esp will be set to the bottom of sigaltstack
if it is not on sigaltstack area when signaled.
What you have to do is protect the top of registerd sigaltstack.
If %esp is in the range of registerd sigaltstack at SEGV, wraparound
will stop.

Thanks,
-Kame
 

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Mikael Pettersson]

On Wed, 03 Oct 2007 17:06:24 +0900, Shi Weihua wrote:
> Fixing alternative signal stack wraparound.
> 
> If a process uses alternative signal stack by using sigaltstack()
> and that stack overflow, stack wraparound occurs.
> This patch checks whether the signal frame is on the alternative
> stack. If the frame is not on there, kill a signal SIGSEGV to the process forcedly
> then the process will be terminated.
> 
> This patch is for i386,version is 2.6.23-rc8.
> 
> Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com>
> 
> diff -pur linux-2.6.23-rc8.orig/arch/i386/kernel/signal.c linux-2.6.23-rc8/arch/i386/kernel/signal.c
> --- linux-2.6.23-rc8.orig/arch/i386/kernel/signal.c	2007-09-26 09:44:08.000000000 +0900
> +++ linux-2.6.23-rc8/arch/i386/kernel/signal.c	2007-09-26 13:14:25.000000000 +0900
> @@ -332,6 +332,10 @@ static int setup_frame(int sig, struct k
> 
>   	frame = get_sigframe(ka, regs, sizeof(*frame));
> 
> +	if ((ka->sa.sa_flags & SA_ONSTACK) &&
> +		!sas_ss_flags((unsigned long)frame))
> +		goto give_sigsegv;
> +
>   	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
>   		goto give_sigsegv;
> 
> @@ -425,6 +429,10 @@ static int setup_rt_frame(int sig, struc
> 
>   	frame = get_sigframe(ka, regs, sizeof(*frame));
> 
> +	if ((ka->sa.sa_flags & SA_ONSTACK) &&
> +		!sas_ss_flags((unsigned long)frame))
> +		goto give_sigsegv;
> +
>   	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
>   		goto give_sigsegv;

Your patch description is a little terse. What you do is that
after the kernel has decided where to put the signal frame,
you add a check that the base of the frame still lies in the
altstack range if altstack delivery is requested for the signal,
and if it doesn't a hard error is forced.

The coding of that logic is fine.

What I don't agree with is the logic itself:
- You only catch altstack overflow caused by the kernel pushing
  a sigframe. You don't catch overflow caused by the user-space
  signal handler pushing its own stack frame after the sigframe.
- SUSv3 specifies the effect of altstack overflow as "undefined".
- The overflow problem can be solved in user-space: allocate the
  altstack with mmap(), then mprotect() the lowest page to prevent
  accesses to it. Any overflow into it, by the kernel's signal
  delivery code or by the user-space signal handler, will be caught.

So this patch gets a NAK from me.

/Mikael

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[KAMEZAWA Hiroyuki]

On Wed, 3 Oct 2007 14:20:07 +0200 (MEST)
Mikael Pettersson <mikpe@it.uu.se> wrote:

> What I don't agree with is the logic itself:
> - You only catch altstack overflow caused by the kernel pushing
>   a sigframe. You don't catch overflow caused by the user-space
>   signal handler pushing its own stack frame after the sigframe.
> - SUSv3 specifies the effect of altstack overflow as "undefined".
> - The overflow problem can be solved in user-space: allocate the
>   altstack with mmap(), then mprotect() the lowest page to prevent
>   accesses to it. Any overflow into it, by the kernel's signal
>   delivery code or by the user-space signal handler, will be caught.
> 
> So this patch gets a NAK from me.
> 

I can understand what you say, but a program which meets this problem
cannot be debugged ;(

gdb just shows infinit loop of function frames and origignal signal frame
which includes the most important information is overwritten.

Ah yes, user's sigaltstack setup is bad if this happens, but I can't ask
novice programmers to take care of "please verify the page next to
sigaltstack is not mapped or protected." such a thing is not written in man(2)
page of sigaltstack now.


Thanks,
-Kame

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[KAMEZAWA Hiroyuki]

On Wed, 3 Oct 2007 21:40:29 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Wed, 3 Oct 2007 14:20:07 +0200 (MEST)
> Mikael Pettersson <mikpe@it.uu.se> wrote:
> 
> > What I don't agree with is the logic itself:
> > - You only catch altstack overflow caused by the kernel pushing
> >   a sigframe. You don't catch overflow caused by the user-space
> >   signal handler pushing its own stack frame after the sigframe.
> > - SUSv3 specifies the effect of altstack overflow as "undefined".
> > - The overflow problem can be solved in user-space: allocate the
> >   altstack with mmap(), then mprotect() the lowest page to prevent
> >   accesses to it. Any overflow into it, by the kernel's signal
> >   delivery code or by the user-space signal handler, will be caught.
> > 
> > So this patch gets a NAK from me.
> > 
> 
> I can understand what you say, but a program which meets this problem
> cannot be debugged ;(
> 
> gdb just shows infinit loop of function frames and origignal signal frame
> which includes the most important information is overwritten.
> 
there is a difference among user's stack overflow and kernel's.
 - user's stack overflow just breaks memory next to stack frame.
 - kernel's altstack overflow, which this patch tries to fix, breaks
   the bottom of altstack  bacause %esp goes back to the bottom
   of ths altstack when it exceeds altstack range.
   This behavior overwrite orignail stack frame and shows  infinit loop
   of function call to gdb and never stop with 100% cpu usage.
   
Thanks,
-Kame

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Shi Weihua]

Mikael Pettersson wrote::
> On Wed, 03 Oct 2007 17:06:24 +0900, Shi Weihua wrote:
>> Fixing alternative signal stack wraparound.
>>
>> If a process uses alternative signal stack by using sigaltstack()
>> and that stack overflow, stack wraparound occurs.
>> This patch checks whether the signal frame is on the alternative
>> stack. If the frame is not on there, kill a signal SIGSEGV to the process forcedly
>> then the process will be terminated.
>>
>> This patch is for i386,version is 2.6.23-rc8.
>>
>> Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com>
>>
>> diff -pur linux-2.6.23-rc8.orig/arch/i386/kernel/signal.c linux-2.6.23-rc8/arch/i386/kernel/signal.c
>> --- linux-2.6.23-rc8.orig/arch/i386/kernel/signal.c	2007-09-26 09:44:08.000000000 +0900
>> +++ linux-2.6.23-rc8/arch/i386/kernel/signal.c	2007-09-26 13:14:25.000000000 +0900
>> @@ -332,6 +332,10 @@ static int setup_frame(int sig, struct k
>>
>>   	frame = get_sigframe(ka, regs, sizeof(*frame));
>>
>> +	if ((ka->sa.sa_flags & SA_ONSTACK) &&
>> +		!sas_ss_flags((unsigned long)frame))
>> +		goto give_sigsegv;
>> +
>>   	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
>>   		goto give_sigsegv;
>>
>> @@ -425,6 +429,10 @@ static int setup_rt_frame(int sig, struc
>>
>>   	frame = get_sigframe(ka, regs, sizeof(*frame));
>>
>> +	if ((ka->sa.sa_flags & SA_ONSTACK) &&
>> +		!sas_ss_flags((unsigned long)frame))
>> +		goto give_sigsegv;
>> +
>>   	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
>>   		goto give_sigsegv;
> 
> Your patch description is a little terse. What you do is that
> after the kernel has decided where to put the signal frame,
> you add a check that the base of the frame still lies in the
> altstack range if altstack delivery is requested for the signal,
> and if it doesn't a hard error is forced.
> 
> The coding of that logic is fine.
> 
> What I don't agree with is the logic itself:
> - You only catch altstack overflow caused by the kernel pushing
>   a sigframe. You don't catch overflow caused by the user-space
>   signal handler pushing its own stack frame after the sigframe.
> - SUSv3 specifies the effect of altstack overflow as "undefined".
> - The overflow problem can be solved in user-space: allocate the
>   altstack with mmap(), then mprotect() the lowest page to prevent
>   accesses to it. Any overflow into it, by the kernel's signal
>   delivery code or by the user-space signal handler, will be caught.

mmap/mprotect can not avoid this kind of wraparound.
Please compile and run the following test code on i386.
The code want to allow process access from high to mid,and not from mid to low.
high
|
|
mid
|
|
low

#include <stdio.h>
#include <signal.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
#include <unistd.h>

#define die(msg) do { perror(msg); exit(EXIT_FAILURE); } while (0)
volatile int counter = 0;

#ifdef __i386__
void print_esp()
{
	unsigned long esp;
	__asm__ __volatile__("movl %%esp, %0":"=g"(esp));

	printf("esp = 0x%08lx\n", esp);
}
#endif

static void segv_handler()
{
#ifdef __i386__
	print_esp();
#endif

	int *c = NULL;
	counter++;
	printf("%d\n", counter);

	*c = 1;			// SEGV
}

int main()
{
	int *c = NULL;
	int pagesize;
	char *addr;
	stack_t stack;
	struct sigaction action;

	pagesize = sysconf(_SC_PAGE_SIZE);
	if (pagesize == -1) {
		die("sysconf");
		exit(EXIT_FAILURE);
	}

	addr = mmap(NULL, pagesize * 2, PROT_READ | PROT_WRITE | PROT_EXEC,
		    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
	if (addr == MAP_FAILED) {
		die("mmap");
		exit(EXIT_FAILURE);
	}
	printf("begin = 0x%08lx\n", addr);
	printf("end   = 0x%08lx\n", addr + pagesize * 2);
	
	if (mprotect(addr, pagesize, PROT_NONE) == -1) {
		die("mprotect");
		exit(EXIT_FAILURE);
	}

	stack.ss_sp = addr + pagesize;
	stack.ss_flags = 0;
	stack.ss_size = pagesize;	//SIGSTKSZ;
	int error = sigaltstack(&stack, NULL);
	if (error) {
		printf("Failed to use sigaltstack!\n");
		return -1;
	}

	memset(&action, 0, sizeof(action));
	action.sa_handler = segv_handler;
	action.sa_flags = SA_ONSTACK | SA_NODEFER;

	sigemptyset(&action.sa_mask);

	sigaction(SIGSEGV, &action, NULL);

	*c = 0;			//SEGV

	return 0;
}

Any suggestion?

Thanks
Shi Weihua

> 
> So this patch gets a NAK from me.
> 
> /Mikael
> 
> 
> 

[PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Shi Weihua]

Fixing alternative signal stack wraparound.

If a process uses alternative signal stack by using sigaltstack()
and that stack overflow, stack wraparound occurs.
This patch checks whether the signal frame is on the alternative
stack. If the frame is not on there, kill a signal SIGSEGV to the process forcedly
then the process will be terminated.

This patch is for i386,version is 2.6.23-rc8.

Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com>

diff -pur linux-2.6.23-rc8.orig/arch/i386/kernel/signal.c linux-2.6.23-rc8/arch/i386/kernel/signal.c
--- linux-2.6.23-rc8.orig/arch/i386/kernel/signal.c	2007-09-26 09:44:08.000000000 +0900
+++ linux-2.6.23-rc8/arch/i386/kernel/signal.c	2007-09-26 13:14:25.000000000 +0900
@@ -332,6 +332,10 @@ static int setup_frame(int sig, struct k

  	frame = get_sigframe(ka, regs, sizeof(*frame));

+	if ((ka->sa.sa_flags & SA_ONSTACK) &&
+		!sas_ss_flags((unsigned long)frame))
+		goto give_sigsegv;
+
  	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
  		goto give_sigsegv;

@@ -425,6 +429,10 @@ static int setup_rt_frame(int sig, struc

  	frame = get_sigframe(ka, regs, sizeof(*frame));

+	if ((ka->sa.sa_flags & SA_ONSTACK) &&
+		!sas_ss_flags((unsigned long)frame))
+		goto give_sigsegv;
+
  	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
  		goto give_sigsegv;

Re: [PATCH 1/3] signal(i386): alternative signal stack wraparound occurs

[Shi Weihua]

Hi everyone,

If a process uses alternative signal stack by using sigaltstack(),
then that stack overflows and stack wraparound may occur.
Simple explanation:
The accurate esp order is A,B,C,D,...
But now the esp points to A,B,C and A,B,C... dropping into a recursion.

The upper bug and patch about "alternative signal stack wraparound occurs"
has been contributed here at 10/3.
(subject:[PATCH 0/3] signal: alternative signal stack wraparound occurs)
(Please refer to http://lkml.org/lkml/2007/10/3/41).

Now, I renewed the patch and it can stop wraparound.
Can you give me some advice about storing the previous esp?

Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com> 

---
diff -urpN linux-2.6.24-rc2.orig/arch/x86/kernel/signal_32.c linux-2.6.24-rc2/arch/x86/kernel/signal_32.c
--- linux-2.6.24-rc2.orig/arch/x86/kernel/signal_32.c	2007-11-13 14:30:45.000000000 +0800
+++ linux-2.6.24-rc2/arch/x86/kernel/signal_32.c	2007-11-13 14:38:03.000000000 +0800
@@ -297,7 +297,8 @@ get_sigframe(struct k_sigaction *ka, str
 
 	/* This is the X/Open sanctioned signal stack switching.  */
 	if (ka->sa.sa_flags & SA_ONSTACK) {
-		if (sas_ss_flags(esp) == 0)
+		if (sas_ss_flags(esp) == 0 &&
+			!on_sig_stack(current->pre_ss_sp))
 			esp = current->sas_ss_sp + current->sas_ss_size;
 	}
 
@@ -330,9 +331,15 @@ static int setup_frame(int sig, struct k
 
 	frame = get_sigframe(ka, regs, sizeof(*frame));
 
+	if ((ka->sa.sa_flags & SA_ONSTACK) &&
+		!sas_ss_flags((unsigned long)frame))
+		goto give_sigsegv;
+
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
 		goto give_sigsegv;
 
+	current->pre_ss_sp = (unsigned long)frame;
+
 	usig = current_thread_info()->exec_domain
 		&& current_thread_info()->exec_domain->signal_invmap
 		&& sig < 32
diff -urpN linux-2.6.24-rc2.orig/include/linux/sched.h linux-2.6.24-rc2/include/linux/sched.h
--- linux-2.6.24-rc2.orig/include/linux/sched.h	2007-11-13 14:29:17.000000000 +0800
+++ linux-2.6.24-rc2/include/linux/sched.h	2007-11-13 14:31:46.000000000 +0800
@@ -1059,6 +1059,7 @@ struct task_struct {
 	struct sigpending pending;
 
 	unsigned long sas_ss_sp;
+	unsigned long pre_ss_sp;
 	size_t sas_ss_size;
 	int (*notifier)(void *priv);
 	void *notifier_data;
diff -urpN linux-2.6.24-rc2.orig/kernel/signal.c linux-2.6.24-rc2/kernel/signal.c
--- linux-2.6.24-rc2.orig/kernel/signal.c	2007-11-13 14:29:16.000000000 +0800
+++ linux-2.6.24-rc2/kernel/signal.c	2007-11-13 14:33:00.000000000 +0800
@@ -2403,6 +2403,9 @@ do_sigaltstack (const stack_t __user *us
 
 		current->sas_ss_sp = (unsigned long) ss_sp;
 		current->sas_ss_size = ss_size;
+
+		/* reset previous sp */
+		current->pre_ss_sp = 0;
 	}
 
 	if (uoss) {


Shi Weihua wrote::
> Fixing alternative signal stack wraparound.
> 
> If a process uses alternative signal stack by using sigaltstack()
> and that stack overflow, stack wraparound occurs.
> This patch checks whether the signal frame is on the alternative
> stack. If the frame is not on there, kill a signal SIGSEGV to the
> process forcedly
> then the process will be terminated.
> 
> This patch is for i386,version is 2.6.23-rc8.
> 
> Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com>
> 
> diff -pur linux-2.6.23-rc8.orig/arch/i386/kernel/signal.c
> linux-2.6.23-rc8/arch/i386/kernel/signal.c
> --- linux-2.6.23-rc8.orig/arch/i386/kernel/signal.c    2007-09-26
> 09:44:08.000000000 +0900
> +++ linux-2.6.23-rc8/arch/i386/kernel/signal.c    2007-09-26
> 13:14:25.000000000 +0900
> @@ -332,6 +332,10 @@ static int setup_frame(int sig, struct k
> 
>      frame = get_sigframe(ka, regs, sizeof(*frame));
> 
> +    if ((ka->sa.sa_flags & SA_ONSTACK) &&
> +        !sas_ss_flags((unsigned long)frame))
> +        goto give_sigsegv;
> +
>      if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
>          goto give_sigsegv;
> 
> @@ -425,6 +429,10 @@ static int setup_rt_frame(int sig, struc
> 
>      frame = get_sigframe(ka, regs, sizeof(*frame));
> 
> +    if ((ka->sa.sa_flags & SA_ONSTACK) &&
> +        !sas_ss_flags((unsigned long)frame))
> +        goto give_sigsegv;
> +
>      if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
>          goto give_sigsegv;
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 
>