2007-10-03 08:07:00

by Shi Weihua

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

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 <[email protected]>

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;



2007-10-03 12:20:32

by Mikael Pettersson

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

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 <[email protected]>
>
> 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

2007-10-03 12:40:51

by Kamezawa Hiroyuki

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

On Wed, 3 Oct 2007 14:20:07 +0200 (MEST)
Mikael Pettersson <[email protected]> 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

2007-10-03 13:20:59

by Kamezawa Hiroyuki

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

On Wed, 3 Oct 2007 21:40:29 +0900
KAMEZAWA Hiroyuki <[email protected]> wrote:

> On Wed, 3 Oct 2007 14:20:07 +0200 (MEST)
> Mikael Pettersson <[email protected]> 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

2007-10-03 13:47:01

by Mikael Pettersson

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

On Wed, 3 Oct 2007 22:20:46 +0900, KAMEZAWA Hiroyuki wrote:
> On Wed, 3 Oct 2007 21:40:29 +0900
> KAMEZAWA Hiroyuki <[email protected]> wrote:
>
> > On Wed, 3 Oct 2007 14:20:07 +0200 (MEST)
> > Mikael Pettersson <[email protected]> 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

2007-10-03 14:25:26

by Kamezawa Hiroyuki

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

On Wed, 3 Oct 2007 15:46:32 +0200 (MEST)
Mikael Pettersson <[email protected]> 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

2007-10-04 11:02:51

by Shi Weihua

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

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 <[email protected]>
>>
>> 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
>
>
>

2007-10-04 11:56:32

by Shi Weihua

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

KAMEZAWA Hiroyuki wrote::
> On Wed, 3 Oct 2007 15:46:32 +0200 (MEST)
> Mikael Pettersson <[email protected]> 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
>
>
>

2007-10-04 12:17:59

by Kamezawa Hiroyuki

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

On Thu, 04 Oct 2007 20:56:14 +0900
Shi Weihua <[email protected]> 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

2007-10-04 12:33:30

by Shi Weihua

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

KAMEZAWA Hiroyuki wrote::
> On Thu, 04 Oct 2007 20:56:14 +0900
> Shi Weihua <[email protected]> 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
>
>
>

2007-10-04 12:47:39

by Kamezawa Hiroyuki

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

On Thu, 04 Oct 2007 21:33:12 +0900
Shi Weihua <[email protected]> wrote:

> KAMEZAWA Hiroyuki wrote::
> > On Thu, 04 Oct 2007 20:56:14 +0900
> > Shi Weihua <[email protected]> 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

2007-10-04 13:09:16

by Mikael Pettersson

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

On Thu, 4 Oct 2007 21:47:30 +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 04 Oct 2007 21:33:12 +0900
> Shi Weihua <[email protected]> wrote:
>
> > KAMEZAWA Hiroyuki wrote::
> > > On Thu, 04 Oct 2007 20:56:14 +0900
> > > Shi Weihua <[email protected]> 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

2007-10-05 00:55:27

by Shi Weihua

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

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 <[email protected]> wrote:
>>
>>> KAMEZAWA Hiroyuki wrote::
>>>> On Thu, 04 Oct 2007 20:56:14 +0900
>>>> Shi Weihua <[email protected]> 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
>
>
>

2007-11-19 02:18:12

by Shi Weihua

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

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 <[email protected]>

---
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 <[email protected]>
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
>