[PATCH] consider stack access while checking for alternate signal stack

[PATCH] consider stack access while checking for alternate signal stack

[Sebastian Andrzej Siewior]

The stack pointer can be either first incremented/decremented and then
used (lets call it PRE) or incremented/decremented after its use (lets
call it POST). The difference is whether the stack pointer points to the
last variable on the stack or to the first free slot. This tiny little
detail caused Debian bug #544905 on AMD64. gcc-4.3 with -O2 optimized
the code in a way where the signal stack pointer had the same value as
the real stack pointer. The stack pointer is PRE_DEC and therefore we
are not on the alternative stack yet.
This patch should handle all corned cases now. AVR is the only
architecture which has a POST operation defined and a Linux port
avaiable. All other architectures are POST_DEC except PA-RISC which is
POST_INC

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
I have also a reduced testcase at [0] which is AMD64 only. Most other
architectures I've looked at store some register(s) between the stack
pointer and the first/last variable so I could only trigger it on AMD64.

Haavard: The AVR32 assumption is pure on gcc's STACK_PUSH_CODE which is
POST_DEC. Could you please ack/nak it?

[0] http://download.breakpoint.cc/tc-sig-stack.c

 arch/avr32/Kconfig    |    3 +++
 include/linux/sched.h |   24 ++++++++++++++++++++++--
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/arch/avr32/Kconfig b/arch/avr32/Kconfig
index 35e3bd9..520c489 100644
--- a/arch/avr32/Kconfig
+++ b/arch/avr32/Kconfig
@@ -70,6 +70,9 @@ config GENERIC_BUG
 	def_bool y
 	depends on BUG
 
+config STACK_STORE_POST
+	def_bool y
+
 source "init/Kconfig"
 
 source "kernel/Kconfig.freezer"
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 75e6e60..23e6eb6 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2086,11 +2086,31 @@ static inline int is_si_special(const struct siginfo *info)
 	return info <= SEND_SIG_FORCED;
 }
 
-/* True if we are on the alternate signal stack.  */
-
+/*
+ * True if we are on the alternate signal stack, based on the follwoing
+ * example: The alternative stack handler starts at 0x10 and its size is 0x20
+ * bytes. The numbers behind PRE and POST are aimed as the result.
+ * PRE means the stack is first decremented and than the content of the variale
+ * is stored. This means, the stack pointer points in the PRE case to the last
+ * variable on the stack. In the POST case it points to the first free slot.
+ *
+ *         0   5       10    15   30     40   80
+ *         +-----------+-----------+----------+
+ *         |           | SIG STACK |          |
+ *         +-----------+-----------+----------+
+ * POST        0       1     1     0     0
+ * PRE         0       0     1     1     0
+ */
 static inline int on_sig_stack(unsigned long sp)
 {
+#if defined(CONFIG_STACK_STORE_POST)
 	return (sp - current->sas_ss_sp < current->sas_ss_size);
+
+#else
+	if (!(sp - current->sas_ss_sp))
+		return 0;
+	return (sp - current->sas_ss_sp <= current->sas_ss_size);
+#endif
 }
 
 static inline int sas_ss_flags(unsigned long sp)
-- 
1.6.4.GIT

Re: [PATCH] consider stack access while checking for alternate signal stack

[Haavard Skinnemoen]

Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:
> Haavard: The AVR32 assumption is pure on gcc's STACK_PUSH_CODE which is
> POST_DEC. Could you please ack/nak it?

Hmm, no, that is not correct. AVR32 is definitely pre-decrement.

Note that 8-bit AVR and AVR32 are two completely different
architectures.

Haavard

Re: [PATCH] consider stack access while checking for alternate signal stack

[Sebastian Andrzej Siewior]

* Haavard Skinnemoen | 2009-10-19 13:09:31 [+0900]:

>Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:
>> Haavard: The AVR32 assumption is pure on gcc's STACK_PUSH_CODE which is
>> POST_DEC. Could you please ack/nak it?
>
>Hmm, no, that is not correct. AVR32 is definitely pre-decrement.
>
>Note that 8-bit AVR and AVR32 are two completely different
>architectures.
Ah okay. So the AVR32 bits are not in upstream GCC yet?

>
>Haavard

Sebastian

Re: [PATCH] consider stack access while checking for alternate signal stack

[Haavard Skinnemoen]

Sebastian Andrzej Siewior <bigeasy@breakpoint.cc> wrote:
> >Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:  
> >> Haavard: The AVR32 assumption is pure on gcc's STACK_PUSH_CODE which is
> >> POST_DEC. Could you please ack/nak it?  
> >
> >Hmm, no, that is not correct. AVR32 is definitely pre-decrement.
> >
> >Note that 8-bit AVR and AVR32 are two completely different
> >architectures.  
> Ah okay. So the AVR32 bits are not in upstream GCC yet?

No, unfortunately not. We're working on it, and have been for some
time, but the progress is very slow.

Haavard

Re: [PATCH] consider stack access while checking for alternate signal stack

[Ingo Molnar]

(Cc:-ed more folks, quoted mail and patch can be found below.)

* Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:

> The stack pointer can be either first incremented/decremented and then
> used (lets call it PRE) or incremented/decremented after its use (lets
> call it POST). The difference is whether the stack pointer points to the
> last variable on the stack or to the first free slot. This tiny little
> detail caused Debian bug #544905 on AMD64. gcc-4.3 with -O2 optimized
> the code in a way where the signal stack pointer had the same value as
> the real stack pointer. The stack pointer is PRE_DEC and therefore we
> are not on the alternative stack yet.
> This patch should handle all corned cases now. AVR is the only
> architecture which has a POST operation defined and a Linux port
> avaiable. All other architectures are POST_DEC except PA-RISC which is
> POST_INC
> 
> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> ---
> I have also a reduced testcase at [0] which is AMD64 only. Most other
> architectures I've looked at store some register(s) between the stack
> pointer and the first/last variable so I could only trigger it on AMD64.
> 
> Haavard: The AVR32 assumption is pure on gcc's STACK_PUSH_CODE which is
> POST_DEC. Could you please ack/nak it?
> 
> [0] http://download.breakpoint.cc/tc-sig-stack.c
> 
>  arch/avr32/Kconfig    |    3 +++
>  include/linux/sched.h |   24 ++++++++++++++++++++++--
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/avr32/Kconfig b/arch/avr32/Kconfig
> index 35e3bd9..520c489 100644
> --- a/arch/avr32/Kconfig
> +++ b/arch/avr32/Kconfig
> @@ -70,6 +70,9 @@ config GENERIC_BUG
>  	def_bool y
>  	depends on BUG
>  
> +config STACK_STORE_POST
> +	def_bool y
> +
>  source "init/Kconfig"
>  
>  source "kernel/Kconfig.freezer"
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 75e6e60..23e6eb6 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2086,11 +2086,31 @@ static inline int is_si_special(const struct siginfo *info)
>  	return info <= SEND_SIG_FORCED;
>  }
>  
> -/* True if we are on the alternate signal stack.  */
> -
> +/*
> + * True if we are on the alternate signal stack, based on the follwoing
> + * example: The alternative stack handler starts at 0x10 and its size is 0x20
> + * bytes. The numbers behind PRE and POST are aimed as the result.
> + * PRE means the stack is first decremented and than the content of the variale
> + * is stored. This means, the stack pointer points in the PRE case to the last
> + * variable on the stack. In the POST case it points to the first free slot.
> + *
> + *         0   5       10    15   30     40   80
> + *         +-----------+-----------+----------+
> + *         |           | SIG STACK |          |
> + *         +-----------+-----------+----------+
> + * POST        0       1     1     0     0
> + * PRE         0       0     1     1     0
> + */
>  static inline int on_sig_stack(unsigned long sp)
>  {
> +#if defined(CONFIG_STACK_STORE_POST)
>  	return (sp - current->sas_ss_sp < current->sas_ss_size);
> +
> +#else
> +	if (!(sp - current->sas_ss_sp))
> +		return 0;
> +	return (sp - current->sas_ss_sp <= current->sas_ss_size);
> +#endif
>  }
>  
>  static inline int sas_ss_flags(unsigned long sp)
> -- 
> 1.6.4.GIT

Re: [PATCH] consider stack access while checking for alternate signal stack

[Sebastian Andrzej Siewior]

* Ingo Molnar | 2009-10-19 09:33:58 [+0200]:

>> index 35e3bd9..520c489 100644
>> --- a/arch/avr32/Kconfig
>> +++ b/arch/avr32/Kconfig
>> @@ -70,6 +70,9 @@ config GENERIC_BUG
>>  	def_bool y
>>  	depends on BUG
>>  
>> +config STACK_STORE_POST
>> +	def_bool y
>> +
>>  source "init/Kconfig"
>>  
>>  source "kernel/Kconfig.freezer"
Haavard just nacked that part as I mixed up the 8bit AVR with the 32bit
version which is POST_DEC as well as the architectures. In that case
there are no POST_* architectures available.
So the question is should I leave dead code for further reference or
just remove it?

Sebastian

Re: [PATCH] consider stack access while checking for alternate signal stack

[Roland McGrath]

AFAICT all you want is the following, and I'm not sure it requires all that
much explanation.  I've probably missed some subtlety.


Thanks,
Roland

--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2090,7 +2090,13 @@ static inline int is_si_special(const struct siginfo *info)
 
 static inline int on_sig_stack(unsigned long sp)
 {
-	return (sp - current->sas_ss_sp < current->sas_ss_size);
+#ifdef CONFIG_STACK_GROWSUP
+	return sp >= current->sas_ss_sp &&
+		sp - current->sas_ss_sp < current->sas_ss_size;
+#else
+	return sp > current->sas_ss_sp &&
+		sp - current->sas_ss_sp <= current->sas_ss_size;
+#endif
 }
 

Re: [PATCH] consider stack access while checking for alternate signal stack

[Sebastian Andrzej Siewior]

* Roland McGrath | 2009-10-19 11:08:10 [-0700]:

>AFAICT all you want is the following, and I'm not sure it requires all that
>much explanation.  I've probably missed some subtlety.

>Thanks,
>Roland
>
>--- a/include/linux/sched.h
>+++ b/include/linux/sched.h
>@@ -2090,7 +2090,13 @@ static inline int is_si_special(const struct siginfo *info)
> 
> static inline int on_sig_stack(unsigned long sp)
> {
>-	return (sp - current->sas_ss_sp < current->sas_ss_size);
>+#ifdef CONFIG_STACK_GROWSUP
>+	return sp >= current->sas_ss_sp &&
>+		sp - current->sas_ss_sp < current->sas_ss_size;

CONFIG_STACK_GROWSUP is wrong: If your stack grows up and sp ==
sas_ss_sp + size than you are using the last entry in your sig stack
which will be not recognized correctly. The case where sp == sas_ss_sp
is also not detected correctly but this should not happen in real life.

>+#else
>+	return sp > current->sas_ss_sp &&
>+		sp - current->sas_ss_sp <= current->sas_ss_size;
>+#endif
That is the PRE case which is the only relevant since we don't have any
POST architectures. The check here produces the same results as my
variant so it is okay :)
So you prefer the smaller patch with comments around it?
> }
> 

Sebastian

Re: [PATCH] consider stack access while checking for alternate signal stack

[Roland McGrath]

> >+#ifdef CONFIG_STACK_GROWSUP
> >+	return sp >= current->sas_ss_sp &&
> >+		sp - current->sas_ss_sp < current->sas_ss_size;
> 
> CONFIG_STACK_GROWSUP is wrong: If your stack grows up and sp ==
> sas_ss_sp + size than you are using the last entry in your sig stack
> which will be not recognized correctly. 

+		sp - current->sas_ss_sp <= current->sas_ss_size;

then?

> The case where sp == sas_ss_sp
> is also not detected correctly but this should not happen in real life.

So you say that sp==sas_ss_sp should not be considered "on the sig stack"?

> That is the PRE case which is the only relevant since we don't have any
> POST architectures. The check here produces the same results as my
> variant so it is okay :)
> So you prefer the smaller patch with comments around it?

Yes, I think it is far clearer and easier to read than what you posted.


Thanks,
Roland

Re: [PATCH] consider stack access while checking for alternate signal stack

[Sebastian Andrzej Siewior]

* Roland McGrath | 2009-10-20 14:11:16 [-0700]:

>> >+#ifdef CONFIG_STACK_GROWSUP
>> >+	return sp >= current->sas_ss_sp &&
>> >+		sp - current->sas_ss_sp < current->sas_ss_size;
>> 
>> CONFIG_STACK_GROWSUP is wrong: If your stack grows up and sp ==
>> sas_ss_sp + size than you are using the last entry in your sig stack
>> which will be not recognized correctly. 
>
>+		sp - current->sas_ss_sp <= current->sas_ss_size;
>
>then?

+		sp - current->sas_ss_sp < current->sas_ss_size;
That (the old code) is correct on POST_* architectures. However we don't
have any.

>> The case where sp == sas_ss_sp
>> is also not detected correctly but this should not happen in real life.
>
>So you say that sp==sas_ss_sp should not be considered "on the sig stack"?
Exactly. Because if you have a PRE_* architecture than you first
increment/decrement the stack and than store the value.
So if sp == sas_ss_sp than your next store on the stack will be just
below the begin of your sig stack.

>> That is the PRE case which is the only relevant since we don't have any
>> POST architectures. The check here produces the same results as my
>> variant so it is okay :)
>> So you prefer the smaller patch with comments around it?
>
>Yes, I think it is far clearer and easier to read than what you posted.
Okay. This would bring us to:

-       return (sp - current->sas_ss_sp < current->sas_ss_size);
+       /* This considers PRE_DEC and PRE_INC architectures */
+       return sp > current->sas_ss_sp &&
+               sp - current->sas_ss_sp <= current->sas_ss_size;

And I throw my table away and put something else into patch's comment?

>
>Thanks,
>Roland

Sebastian

[PATCH v2] consider the kind of stack incrementation while checking for alternate signal stack

[Sebastian Andrzej Siewior]

On PRE_INC and PRE_DEC architectures the stack is first incremented /
decremented and than the value is saved. Therefore sas_ss_sp == sp is
not on the alternative signal stack while sas_ss_sp + sas_ss_size == sp
is on the alternative signal stack.
This was reported as Debian bug #544905 on AMD64 where gcc-4.3 with -O2
created such code.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
v1..v2: - remove support for POST_INC / POST_DEC archs. Every linux
          architecture is either PRE_INC or POST_INC
        - replaced  !(sp - current->sas_ss_sp)) with
          sp > current->sas_ss_sp
          with catches the equal case. Recommended by Roland.

AMD64 test case at [0]

[0] [0] http://download.breakpoint.cc/tc-sig-stack.c

 include/linux/sched.h |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 75e6e60..6ea5d12 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2086,11 +2086,14 @@ static inline int is_si_special(const struct siginfo *info)
 	return info <= SEND_SIG_FORCED;
 }
 
-/* True if we are on the alternate signal stack.  */
-
+/*
+ * True if we are on the alternate signal stack.
+ * The implementation considers PRE_DEC and PRE_INC architectures.
+ */
 static inline int on_sig_stack(unsigned long sp)
 {
-	return (sp - current->sas_ss_sp < current->sas_ss_size);
+	return sp > current->sas_ss_sp &&
+		sp - current->sas_ss_sp <= current->sas_ss_size;
 }
 
 static inline int sas_ss_flags(unsigned long sp)
-- 
1.6.4.GIT

[tip:core/signal] signal: Fix alternate signal stack check

[tip-bot for Sebastian Andrzej Siewior]

Commit-ID:  2a855dd01bc1539111adb7233f587c5c468732ac
Gitweb:     http://git.kernel.org/tip/2a855dd01bc1539111adb7233f587c5c468732ac
Author:     Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
AuthorDate: Sun, 25 Oct 2009 15:37:58 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 4 Nov 2009 18:19:12 +0100

signal: Fix alternate signal stack check

All architectures in the kernel increment/decrement the stack pointer
before storing values on the stack.

On architectures which have the stack grow down sas_ss_sp == sp is not
on the alternate signal stack while sas_ss_sp + sas_ss_size == sp is
on the alternate signal stack.

On architectures which have the stack grow up sas_ss_sp == sp is on
the alternate signal stack while sas_ss_sp + sas_ss_size == sp is not
on the alternate signal stack.

The current implementation fails for architectures which have the
stack grow down on the corner case where sas_ss_sp == sp.This was
reported as Debian bug #544905 on AMD64.
Simplified test case: http://download.breakpoint.cc/tc-sig-stack.c

The test case creates the following stack scenario:
   0xn0300	stack top
   0xn0200	alt stack pointer top (when switching to alt stack)
   0xn01ff	alt stack end
   0xn0100	alt stack start == stack pointer

If the signal is sent the stack pointer is pointing to the base
address of the alt stack and the kernel erroneously decides that it
has already switched to the alternate stack because of the current
check for "sp - sas_ss_sp < sas_ss_size"

On parisc (stack grows up) the scenario would be:
   0xn0200	stack pointer
   0xn01ff	alt stack end
   0xn0100	alt stack start = alt stack pointer base
   		    	  	  (when switching to alt stack)
   0xn0000	stack base

This is handled correctly by the current implementation.

[ tglx: Modified for archs which have the stack grow up (parisc) which
  	would fail with the correct implementation for stack grows
  	down. Added a check for sp >= current->sas_ss_sp which is
  	strictly not necessary but makes the code symetric for both
  	variants ]

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Roland McGrath <roland@redhat.com>
Cc: Kyle McMartin <kyle@mcmartin.ca>
Cc: stable@kernel.org
LKML-Reference: <20091025143758.GA6653@Chamillionaire.breakpoint.cc>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/sched.h |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 75e6e60..0f67914 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2086,11 +2086,18 @@ static inline int is_si_special(const struct siginfo *info)
 	return info <= SEND_SIG_FORCED;
 }
 
-/* True if we are on the alternate signal stack.  */
-
+/*
+ * True if we are on the alternate signal stack.
+ */
 static inline int on_sig_stack(unsigned long sp)
 {
-	return (sp - current->sas_ss_sp < current->sas_ss_size);
+#ifdef CONFIG_STACK_GROWSUP
+	return sp >= current->sas_ss_sp &&
+		sp - current->sas_ss_sp < current->sas_ss_size;
+#else
+	return sp > current->sas_ss_sp &&
+		sp - current->sas_ss_sp <= current->sas_ss_size;
+#endif
 }
 
 static inline int sas_ss_flags(unsigned long sp)