[PATCH 0/4] SS_AUTODISARM fixes and an ABI change

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/4] SS_AUTODISARM fixes and an ABI change

Andy Lutomirski-3
The first three are fixes IMO.  The fourth changes the SS_AUTODISARM
bit.  I'm assuming that's okay, as the bit has existed in -tip for
less than a day.

Andy Lutomirski (4):
  signals/sigaltstack: If SS_AUTODISARM, bypass on_sig_stack
  selftests/sigaltstack: Fix the sas test on old kernels
  signals/sigaltstack: Report current flag bits in sigaltstack()
  signals/sigaltstack: Change SS_AUTODISARM to (1U << 31)

 include/linux/sched.h                     | 12 +++++++++
 include/uapi/linux/signal.h               |  2 +-
 kernel/signal.c                           |  3 ++-
 tools/testing/selftests/sigaltstack/sas.c | 42 +++++++++++++++++++++++--------
 4 files changed, 46 insertions(+), 13 deletions(-)

--
2.5.5

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/4] signals/sigaltstack: If SS_AUTODISARM, bypass on_sig_stack

Andy Lutomirski-3
If a signal stack is set up with SS_AUTODISARM, then the kernel
inherently avoids incorrectly resetting the signal stack if signals
recurse: the signal stack will be reset on the first signal
delivery.  This means that we don't need check the stack pointer
when delivering signals if SS_AUTODISARM is set.

This will make segmented x86 programs more robust: currently there's
a hole that could be triggered if ESP/RSP appears to point to the
signal stack but actually doesn't due to a nonzero SS base.

Signed-off-by: Stas Sergeev <[hidden email]>
Cc: Al Viro <[hidden email]>
Cc: Aleksa Sarai <[hidden email]>
Cc: Amanieu d'Antras <[hidden email]>
Cc: Andrea Arcangeli <[hidden email]>
Cc: Andrew Morton <[hidden email]>
Cc: Andy Lutomirski <[hidden email]>
Cc: Borislav Petkov <[hidden email]>
Cc: Brian Gerst <[hidden email]>
Cc: Denys Vlasenko <[hidden email]>
Cc: Eric W. Biederman <[hidden email]>
Cc: Frederic Weisbecker <[hidden email]>
Cc: H. Peter Anvin <[hidden email]>
Cc: Heinrich Schuchardt <[hidden email]>
Cc: Jason Low <[hidden email]>
Cc: Josh Triplett <[hidden email]>
Cc: Konstantin Khlebnikov <[hidden email]>
Cc: Linus Torvalds <[hidden email]>
Cc: Oleg Nesterov <[hidden email]>
Cc: Palmer Dabbelt <[hidden email]>
Cc: Paul Moore <[hidden email]>
Cc: Pavel Emelyanov <[hidden email]>
Cc: Peter Zijlstra <[hidden email]>
Cc: Richard Weinberger <[hidden email]>
Cc: Sasha Levin <[hidden email]>
Cc: Shuah Khan <[hidden email]>
Cc: Tejun Heo <[hidden email]>
Cc: Thomas Gleixner <[hidden email]>
Cc: Vladimir Davydov <[hidden email]>
Cc: [hidden email]
Cc: [hidden email]
Signed-off-by: Andy Lutomirski <[hidden email]>
---
 include/linux/sched.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2950c5cd3005..8f03a93348b9 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2576,6 +2576,18 @@ static inline int kill_cad_pid(int sig, int priv)
  */
 static inline int on_sig_stack(unsigned long sp)
 {
+ /*
+ * If the signal stack is AUTODISARM then, by construction, we
+ * can't be on the signal stack unless user code deliberately set
+ * SS_AUTODISARM when we were already on the it.
+ *
+ * This improve reliability: if user state gets corrupted such that
+ * the stack pointer points very close to the end of the signal stack,
+ * then this check will enable the signal to be handled anyway.
+ */
+ if (current->sas_ss_flags & SS_AUTODISARM)
+ return 0;
+
 #ifdef CONFIG_STACK_GROWSUP
  return sp >= current->sas_ss_sp &&
  sp - current->sas_ss_sp < current->sas_ss_size;
--
2.5.5

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/4] selftests/sigaltstack: Fix the sas test on old kernels

Andy Lutomirski-3
In reply to this post by Andy Lutomirski-3
The handling for old kernels was wrong.  Fix it.

Reported-by: Ingo Molnar <[hidden email]>
Cc: Stas Sergeev <[hidden email]>
Cc: Al Viro <[hidden email]>
Cc: Andrew Morton <[hidden email]>
Cc: Andy Lutomirski <[hidden email]>
Cc: Borislav Petkov <[hidden email]>
Cc: Brian Gerst <[hidden email]>
Cc: Denys Vlasenko <[hidden email]>
Cc: H. Peter Anvin <[hidden email]>
Cc: Linus Torvalds <[hidden email]>
Cc: Oleg Nesterov <[hidden email]>
Cc: Pavel Emelyanov <[hidden email]>
Cc: Peter Zijlstra <[hidden email]>
Cc: Shuah Khan <[hidden email]>
Cc: Thomas Gleixner <[hidden email]>
Cc: [hidden email]
Cc: [hidden email]
Signed-off-by: Andy Lutomirski <[hidden email]>
---
 tools/testing/selftests/sigaltstack/sas.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c
index 57da8bfde60b..a98c3ef8141f 100644
--- a/tools/testing/selftests/sigaltstack/sas.c
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -15,6 +15,7 @@
 #include <alloca.h>
 #include <string.h>
 #include <assert.h>
+#include <errno.h>
 
 #ifndef SS_AUTODISARM
 #define SS_AUTODISARM  (1 << 4)
@@ -117,13 +118,19 @@ int main(void)
  stk.ss_flags = SS_ONSTACK | SS_AUTODISARM;
  err = sigaltstack(&stk, NULL);
  if (err) {
- perror("[FAIL]\tsigaltstack(SS_ONSTACK | SS_AUTODISARM)");
- stk.ss_flags = SS_ONSTACK;
- }
- err = sigaltstack(&stk, NULL);
- if (err) {
- perror("[FAIL]\tsigaltstack(SS_ONSTACK)");
- return EXIT_FAILURE;
+ if (errno == EINVAL) {
+ printf("[NOTE]\tThe running kernel doesn't support SS_AUTODISARM\n");
+ /*
+ * If test cases for the !SS_AUTODISARM variant were
+ * added, we could still run them.  We don't have any
+ * test cases like that yet, so just exit and report
+ * success.
+ */
+ return 0;
+ } else {
+ perror("[FAIL]\tsigaltstack(SS_ONSTACK | SS_AUTODISARM)");
+ return EXIT_FAILURE;
+ }
  }
 
  ustack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
--
2.5.5

Reply | Threaded
Open this post in threaded view
|

[PATCH 3/4] signals/sigaltstack: Report current flag bits in sigaltstack()

Andy Lutomirski-3
In reply to this post by Andy Lutomirski-3
sigaltstack()'s reported previous state uses a somewhat odd
convention, but the concept of flag bits is new, and we can do the
flag bits sensibly.  Specifically, let's just report them directly.

This will allow saving and restoring the sigaltstack state using
sigaltstack() to work correctly.

Signed-off-by: Stas Sergeev <[hidden email]>
Cc: Al Viro <[hidden email]>
Cc: Amanieu d'Antras <[hidden email]>
Cc: Andrew Morton <[hidden email]>
Cc: Andy Lutomirski <[hidden email]>
Cc: Borislav Petkov <[hidden email]>
Cc: Brian Gerst <[hidden email]>
Cc: Denys Vlasenko <[hidden email]>
Cc: H. Peter Anvin <[hidden email]>
Cc: Linus Torvalds <[hidden email]>
Cc: Michal Hocko <[hidden email]>
Cc: Oleg Nesterov <[hidden email]>
Cc: Pavel Emelyanov <[hidden email]>
Cc: Peter Zijlstra (Intel) <[hidden email]>
Cc: Peter Zijlstra <[hidden email]>
Cc: Richard Weinberger <[hidden email]>
Cc: Sasha Levin <[hidden email]>
Cc: Shuah Khan <[hidden email]>
Cc: Thomas Gleixner <[hidden email]>
Cc: Vladimir Davydov <[hidden email]>
Cc: [hidden email]
Cc: [hidden email]
Signed-off-by: Andy Lutomirski <[hidden email]>
---
 kernel/signal.c                           |  3 ++-
 tools/testing/selftests/sigaltstack/sas.c | 19 ++++++++++++++++---
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index bf97ea5775ae..ab122a2cee41 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3099,7 +3099,8 @@ do_sigaltstack (const stack_t __user *uss, stack_t __user *uoss, unsigned long s
 
  oss.ss_sp = (void __user *) current->sas_ss_sp;
  oss.ss_size = current->sas_ss_size;
- oss.ss_flags = sas_ss_flags(sp);
+ oss.ss_flags = sas_ss_flags(sp) |
+ (current->sas_ss_flags & SS_FLAG_BITS);
 
  if (uss) {
  void __user *ss_sp;
diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c
index a98c3ef8141f..4280d0699792 100644
--- a/tools/testing/selftests/sigaltstack/sas.c
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -113,6 +113,19 @@ int main(void)
  perror("mmap()");
  return EXIT_FAILURE;
  }
+
+ err = sigaltstack(NULL, &stk);
+ if (err) {
+ perror("[FAIL]\tsigaltstack()");
+ exit(EXIT_FAILURE);
+ }
+ if (stk.ss_flags == SS_DISABLE) {
+ printf("[OK]\tInitial sigaltstack state was SS_DISABLE\n");
+ } else {
+ printf("[FAIL]\tInitial sigaltstack state was %i; should have been SS_DISABLE\n", stk.ss_flags);
+ return EXIT_FAILURE;
+ }
+
  stk.ss_sp = sstack;
  stk.ss_size = SIGSTKSZ;
  stk.ss_flags = SS_ONSTACK | SS_AUTODISARM;
@@ -151,12 +164,12 @@ int main(void)
  perror("[FAIL]\tsigaltstack()");
  exit(EXIT_FAILURE);
  }
- if (stk.ss_flags != 0) {
- printf("[FAIL]\tss_flags=%i, should be 0\n",
+ if (stk.ss_flags != SS_AUTODISARM) {
+ printf("[FAIL]\tss_flags=%i, should be SS_AUTODISARM\n",
  stk.ss_flags);
  exit(EXIT_FAILURE);
  }
- printf("[OK]\tsigaltstack is enabled after signal\n");
+ printf("[OK]\tsigaltstack is still SS_AUTODISARM after signal\n");
 
  printf("[OK]\tTest passed\n");
  return 0;
--
2.5.5

Reply | Threaded
Open this post in threaded view
|

[PATCH 4/4] signals/sigaltstack: Change SS_AUTODISARM to (1U << 31)

Andy Lutomirski-3
In reply to this post by Andy Lutomirski-3
Using bit 4 divides the space of available bits strangely.  Use bit
31 instead so that we have a better chance of keeping flag and mode
bits separate in the long run.

Cc: Stas Sergeev <[hidden email]>
Cc: Al Viro <[hidden email]>
Cc: Aleksa Sarai <[hidden email]>
Cc: Amanieu d'Antras <[hidden email]>
Cc: Andrea Arcangeli <[hidden email]>
Cc: Andrew Morton <[hidden email]>
Cc: Andy Lutomirski <[hidden email]>
Cc: Borislav Petkov <[hidden email]>
Cc: Brian Gerst <[hidden email]>
Cc: Denys Vlasenko <[hidden email]>
Cc: Eric W. Biederman <[hidden email]>
Cc: Frederic Weisbecker <[hidden email]>
Cc: H. Peter Anvin <[hidden email]>
Cc: Heinrich Schuchardt <[hidden email]>
Cc: Jason Low <[hidden email]>
Cc: Josh Triplett <[hidden email]>
Cc: Konstantin Khlebnikov <[hidden email]>
Cc: Linus Torvalds <[hidden email]>
Cc: Oleg Nesterov <[hidden email]>
Cc: Palmer Dabbelt <[hidden email]>
Cc: Paul Moore <[hidden email]>
Cc: Pavel Emelyanov <[hidden email]>
Cc: Peter Zijlstra <[hidden email]>
Cc: Richard Weinberger <[hidden email]>
Cc: Sasha Levin <[hidden email]>
Cc: Shuah Khan <[hidden email]>
Cc: Tejun Heo <[hidden email]>
Cc: Thomas Gleixner <[hidden email]>
Cc: Vladimir Davydov <[hidden email]>
Cc: [hidden email]
Cc: [hidden email]
Signed-off-by: Andy Lutomirski <[hidden email]>
---
 include/uapi/linux/signal.h               | 2 +-
 tools/testing/selftests/sigaltstack/sas.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/signal.h b/include/uapi/linux/signal.h
index 738826048af2..cd0804b6bfa2 100644
--- a/include/uapi/linux/signal.h
+++ b/include/uapi/linux/signal.h
@@ -8,7 +8,7 @@
 #define SS_DISABLE 2
 
 /* bit-flags */
-#define SS_AUTODISARM (1 << 4) /* disable sas during sighandling */
+#define SS_AUTODISARM (1U << 31) /* disable sas during sighandling */
 /* mask for all SS_xxx flags */
 #define SS_FLAG_BITS SS_AUTODISARM
 
diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c
index 4280d0699792..1bb01258e559 100644
--- a/tools/testing/selftests/sigaltstack/sas.c
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -18,7 +18,7 @@
 #include <errno.h>
 
 #ifndef SS_AUTODISARM
-#define SS_AUTODISARM  (1 << 4)
+#define SS_AUTODISARM  (1U << 31)
 #endif
 
 static void *sstack, *ustack;
--
2.5.5

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/4] SS_AUTODISARM fixes and an ABI change

Ingo Molnar
In reply to this post by Andy Lutomirski-3

* Andy Lutomirski <[hidden email]> wrote:

> The first three are fixes IMO.  The fourth changes the SS_AUTODISARM
> bit.  I'm assuming that's okay, as the bit has existed in -tip for
> less than a day.

Yeah, it's absolutely OK - I made it a standalone tree so we could even rebase it,
but I think authorship and intent is clearer with your series on top of Stas's
original commits.

Thanks,

        Ingo
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] signals/sigaltstack: If SS_AUTODISARM, bypass on_sig_stack

Ingo Molnar
In reply to this post by Andy Lutomirski-3

* Andy Lutomirski <[hidden email]> wrote:

> If a signal stack is set up with SS_AUTODISARM, then the kernel
> inherently avoids incorrectly resetting the signal stack if signals
> recurse: the signal stack will be reset on the first signal
> delivery.  This means that we don't need check the stack pointer
> when delivering signals if SS_AUTODISARM is set.
>
> This will make segmented x86 programs more robust: currently there's
> a hole that could be triggered if ESP/RSP appears to point to the
> signal stack but actually doesn't due to a nonzero SS base.
>
> Signed-off-by: Stas Sergeev <[hidden email]>

Presuably that SOB from Stas is stray, as there's no matching From: line?
I've removed it.

Thanks,

        Ingo
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/4] signals/sigaltstack: Report current flag bits in sigaltstack()

Ingo Molnar
In reply to this post by Andy Lutomirski-3

* Andy Lutomirski <[hidden email]> wrote:

> sigaltstack()'s reported previous state uses a somewhat odd
> convention, but the concept of flag bits is new, and we can do the
> flag bits sensibly.  Specifically, let's just report them directly.
>
> This will allow saving and restoring the sigaltstack state using
> sigaltstack() to work correctly.
>
> Signed-off-by: Stas Sergeev <[hidden email]>

This SOB seems stray as well, so I've removed it too.

Thanks,

        Ingo
Reply | Threaded
Open this post in threaded view
|

[tip:core/signals] selftests/sigaltstack: Fix the sigaltstack test on old kernels

tip-bot for Peter Zijlstra
In reply to this post by Andy Lutomirski-3
Commit-ID:  158b67b5c5ccda9b909f18028a3cd17185ca1efd
Gitweb:     http://git.kernel.org/tip/158b67b5c5ccda9b909f18028a3cd17185ca1efd
Author:     Andy Lutomirski <[hidden email]>
AuthorDate: Tue, 3 May 2016 10:31:50 -0700
Committer:  Ingo Molnar <[hidden email]>
CommitDate: Wed, 4 May 2016 08:34:13 +0200

selftests/sigaltstack: Fix the sigaltstack test on old kernels

The handling for old kernels was wrong, resulting in a segfault.  Fix it.

Reported-by: Ingo Molnar <[hidden email]>
Signed-off-by: Andy Lutomirski <[hidden email]>
Cc: Al Viro <[hidden email]>
Cc: Andrew Morton <[hidden email]>
Cc: Andy Lutomirski <[hidden email]>
Cc: Borislav Petkov <[hidden email]>
Cc: Brian Gerst <[hidden email]>
Cc: Denys Vlasenko <[hidden email]>
Cc: H. Peter Anvin <[hidden email]>
Cc: Linus Torvalds <[hidden email]>
Cc: Oleg Nesterov <[hidden email]>
Cc: Pavel Emelyanov <[hidden email]>
Cc: Peter Zijlstra <[hidden email]>
Cc: Shuah Khan <[hidden email]>
Cc: Stas Sergeev <[hidden email]>
Cc: Thomas Gleixner <[hidden email]>
Cc: [hidden email]
Link: http://lkml.kernel.org/r/f3e739bf435beeaecbd5f038f1359d2eac6d1e63.1462296606.git.luto@...
Signed-off-by: Ingo Molnar <[hidden email]>
---
 tools/testing/selftests/sigaltstack/sas.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c
index 57da8bf..a98c3ef 100644
--- a/tools/testing/selftests/sigaltstack/sas.c
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -15,6 +15,7 @@
 #include <alloca.h>
 #include <string.h>
 #include <assert.h>
+#include <errno.h>
 
 #ifndef SS_AUTODISARM
 #define SS_AUTODISARM  (1 << 4)
@@ -117,13 +118,19 @@ int main(void)
  stk.ss_flags = SS_ONSTACK | SS_AUTODISARM;
  err = sigaltstack(&stk, NULL);
  if (err) {
- perror("[FAIL]\tsigaltstack(SS_ONSTACK | SS_AUTODISARM)");
- stk.ss_flags = SS_ONSTACK;
- }
- err = sigaltstack(&stk, NULL);
- if (err) {
- perror("[FAIL]\tsigaltstack(SS_ONSTACK)");
- return EXIT_FAILURE;
+ if (errno == EINVAL) {
+ printf("[NOTE]\tThe running kernel doesn't support SS_AUTODISARM\n");
+ /*
+ * If test cases for the !SS_AUTODISARM variant were
+ * added, we could still run them.  We don't have any
+ * test cases like that yet, so just exit and report
+ * success.
+ */
+ return 0;
+ } else {
+ perror("[FAIL]\tsigaltstack(SS_ONSTACK | SS_AUTODISARM)");
+ return EXIT_FAILURE;
+ }
  }
 
  ustack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
Reply | Threaded
Open this post in threaded view
|

[tip:core/signals] signals/sigaltstack: If SS_AUTODISARM, bypass on_sig_stack()

tip-bot for Peter Zijlstra
In reply to this post by Andy Lutomirski-3
Commit-ID:  c876eeab6432687846d4cd5fe1e43dbc348de134
Gitweb:     http://git.kernel.org/tip/c876eeab6432687846d4cd5fe1e43dbc348de134
Author:     Andy Lutomirski <[hidden email]>
AuthorDate: Tue, 3 May 2016 10:31:49 -0700
Committer:  Ingo Molnar <[hidden email]>
CommitDate: Wed, 4 May 2016 08:34:13 +0200

signals/sigaltstack: If SS_AUTODISARM, bypass on_sig_stack()

If a signal stack is set up with SS_AUTODISARM, then the kernel
inherently avoids incorrectly resetting the signal stack if signals
recurse: the signal stack will be reset on the first signal
delivery.  This means that we don't need check the stack pointer
when delivering signals if SS_AUTODISARM is set.

This will make segmented x86 programs more robust: currently there's
a hole that could be triggered if ESP/RSP appears to point to the
signal stack but actually doesn't due to a nonzero SS base.

Signed-off-by: Andy Lutomirski <[hidden email]>
Cc: Al Viro <[hidden email]>
Cc: Aleksa Sarai <[hidden email]>
Cc: Amanieu d'Antras <[hidden email]>
Cc: Andrea Arcangeli <[hidden email]>
Cc: Andrew Morton <[hidden email]>
Cc: Andy Lutomirski <[hidden email]>
Cc: Borislav Petkov <[hidden email]>
Cc: Brian Gerst <[hidden email]>
Cc: Denys Vlasenko <[hidden email]>
Cc: Eric W. Biederman <[hidden email]>
Cc: Frederic Weisbecker <[hidden email]>
Cc: H. Peter Anvin <[hidden email]>
Cc: Heinrich Schuchardt <[hidden email]>
Cc: Jason Low <[hidden email]>
Cc: Josh Triplett <[hidden email]>
Cc: Konstantin Khlebnikov <[hidden email]>
Cc: Linus Torvalds <[hidden email]>
Cc: Oleg Nesterov <[hidden email]>
Cc: Palmer Dabbelt <[hidden email]>
Cc: Paul Moore <[hidden email]>
Cc: Pavel Emelyanov <[hidden email]>
Cc: Peter Zijlstra <[hidden email]>
Cc: Richard Weinberger <[hidden email]>
Cc: Sasha Levin <[hidden email]>
Cc: Shuah Khan <[hidden email]>
Cc: Stas Sergeev <[hidden email]>
Cc: Tejun Heo <[hidden email]>
Cc: Thomas Gleixner <[hidden email]>
Cc: Vladimir Davydov <[hidden email]>
Cc: [hidden email]
Link: http://lkml.kernel.org/r/c46bee4654ca9e68c498462fd11746e2bd0d98c8.1462296606.git.luto@...
Signed-off-by: Ingo Molnar <[hidden email]>
---
 include/linux/sched.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2950c5c..77fd49f 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2576,6 +2576,18 @@ static inline int kill_cad_pid(int sig, int priv)
  */
 static inline int on_sig_stack(unsigned long sp)
 {
+ /*
+ * If the signal stack is SS_AUTODISARM then, by construction, we
+ * can't be on the signal stack unless user code deliberately set
+ * SS_AUTODISARM when we were already on it.
+ *
+ * This improves reliability: if user state gets corrupted such that
+ * the stack pointer points very close to the end of the signal stack,
+ * then this check will enable the signal to be handled anyway.
+ */
+ if (current->sas_ss_flags & SS_AUTODISARM)
+ return 0;
+
 #ifdef CONFIG_STACK_GROWSUP
  return sp >= current->sas_ss_sp &&
  sp - current->sas_ss_sp < current->sas_ss_size;
Reply | Threaded
Open this post in threaded view
|

[tip:core/signals] signals/sigaltstack: Report current flag bits in sigaltstack()

tip-bot for Peter Zijlstra
In reply to this post by Andy Lutomirski-3
Commit-ID:  0318bc8a919ded355eaa5078689924a15c1bf52a
Gitweb:     http://git.kernel.org/tip/0318bc8a919ded355eaa5078689924a15c1bf52a
Author:     Andy Lutomirski <[hidden email]>
AuthorDate: Tue, 3 May 2016 10:31:51 -0700
Committer:  Ingo Molnar <[hidden email]>
CommitDate: Wed, 4 May 2016 08:34:14 +0200

signals/sigaltstack: Report current flag bits in sigaltstack()

sigaltstack()'s reported previous state uses a somewhat odd
convention, but the concept of flag bits is new, and we can do the
flag bits sensibly.  Specifically, let's just report them directly.

This will allow saving and restoring the sigaltstack state using
sigaltstack() to work correctly.

Signed-off-by: Andy Lutomirski <[hidden email]>
Cc: Al Viro <[hidden email]>
Cc: Amanieu d'Antras <[hidden email]>
Cc: Andrew Morton <[hidden email]>
Cc: Andy Lutomirski <[hidden email]>
Cc: Borislav Petkov <[hidden email]>
Cc: Brian Gerst <[hidden email]>
Cc: Denys Vlasenko <[hidden email]>
Cc: H. Peter Anvin <[hidden email]>
Cc: Linus Torvalds <[hidden email]>
Cc: Michal Hocko <[hidden email]>
Cc: Oleg Nesterov <[hidden email]>
Cc: Pavel Emelyanov <[hidden email]>
Cc: Peter Zijlstra (Intel) <[hidden email]>
Cc: Peter Zijlstra <[hidden email]>
Cc: Richard Weinberger <[hidden email]>
Cc: Sasha Levin <[hidden email]>
Cc: Shuah Khan <[hidden email]>
Cc: Stas Sergeev <[hidden email]>
Cc: Thomas Gleixner <[hidden email]>
Cc: Vladimir Davydov <[hidden email]>
Cc: [hidden email]
Link: http://lkml.kernel.org/r/94b291ec9fd47741a9264851e316e158ded0b00d.1462296606.git.luto@...
Signed-off-by: Ingo Molnar <[hidden email]>
---
 kernel/signal.c                           |  3 ++-
 tools/testing/selftests/sigaltstack/sas.c | 19 ++++++++++++++++---
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index bf97ea5..ab122a2 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3099,7 +3099,8 @@ do_sigaltstack (const stack_t __user *uss, stack_t __user *uoss, unsigned long s
 
  oss.ss_sp = (void __user *) current->sas_ss_sp;
  oss.ss_size = current->sas_ss_size;
- oss.ss_flags = sas_ss_flags(sp);
+ oss.ss_flags = sas_ss_flags(sp) |
+ (current->sas_ss_flags & SS_FLAG_BITS);
 
  if (uss) {
  void __user *ss_sp;
diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c
index a98c3ef..4280d06 100644
--- a/tools/testing/selftests/sigaltstack/sas.c
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -113,6 +113,19 @@ int main(void)
  perror("mmap()");
  return EXIT_FAILURE;
  }
+
+ err = sigaltstack(NULL, &stk);
+ if (err) {
+ perror("[FAIL]\tsigaltstack()");
+ exit(EXIT_FAILURE);
+ }
+ if (stk.ss_flags == SS_DISABLE) {
+ printf("[OK]\tInitial sigaltstack state was SS_DISABLE\n");
+ } else {
+ printf("[FAIL]\tInitial sigaltstack state was %i; should have been SS_DISABLE\n", stk.ss_flags);
+ return EXIT_FAILURE;
+ }
+
  stk.ss_sp = sstack;
  stk.ss_size = SIGSTKSZ;
  stk.ss_flags = SS_ONSTACK | SS_AUTODISARM;
@@ -151,12 +164,12 @@ int main(void)
  perror("[FAIL]\tsigaltstack()");
  exit(EXIT_FAILURE);
  }
- if (stk.ss_flags != 0) {
- printf("[FAIL]\tss_flags=%i, should be 0\n",
+ if (stk.ss_flags != SS_AUTODISARM) {
+ printf("[FAIL]\tss_flags=%i, should be SS_AUTODISARM\n",
  stk.ss_flags);
  exit(EXIT_FAILURE);
  }
- printf("[OK]\tsigaltstack is enabled after signal\n");
+ printf("[OK]\tsigaltstack is still SS_AUTODISARM after signal\n");
 
  printf("[OK]\tTest passed\n");
  return 0;
Reply | Threaded
Open this post in threaded view
|

[tip:core/signals] signals/sigaltstack: Change SS_AUTODISARM to (1U << 31)

tip-bot for Peter Zijlstra
In reply to this post by Andy Lutomirski-3
Commit-ID:  91c6180572e2fec71701d646ffc40ad30986275c
Gitweb:     http://git.kernel.org/tip/91c6180572e2fec71701d646ffc40ad30986275c
Author:     Andy Lutomirski <[hidden email]>
AuthorDate: Tue, 3 May 2016 10:31:52 -0700
Committer:  Ingo Molnar <[hidden email]>
CommitDate: Wed, 4 May 2016 08:34:14 +0200

signals/sigaltstack: Change SS_AUTODISARM to (1U << 31)

Using bit 4 divides the space of available bits strangely.  Use bit
31 instead so that we have a better chance of keeping flag and mode
bits separate in the long run.

Signed-off-by: Andy Lutomirski <[hidden email]>
Cc: Al Viro <[hidden email]>
Cc: Aleksa Sarai <[hidden email]>
Cc: Amanieu d'Antras <[hidden email]>
Cc: Andrea Arcangeli <[hidden email]>
Cc: Andrew Morton <[hidden email]>
Cc: Andy Lutomirski <[hidden email]>
Cc: Borislav Petkov <[hidden email]>
Cc: Brian Gerst <[hidden email]>
Cc: Denys Vlasenko <[hidden email]>
Cc: Eric W. Biederman <[hidden email]>
Cc: Frederic Weisbecker <[hidden email]>
Cc: H. Peter Anvin <[hidden email]>
Cc: Heinrich Schuchardt <[hidden email]>
Cc: Jason Low <[hidden email]>
Cc: Josh Triplett <[hidden email]>
Cc: Konstantin Khlebnikov <[hidden email]>
Cc: Linus Torvalds <[hidden email]>
Cc: Oleg Nesterov <[hidden email]>
Cc: Palmer Dabbelt <[hidden email]>
Cc: Paul Moore <[hidden email]>
Cc: Pavel Emelyanov <[hidden email]>
Cc: Peter Zijlstra <[hidden email]>
Cc: Richard Weinberger <[hidden email]>
Cc: Sasha Levin <[hidden email]>
Cc: Shuah Khan <[hidden email]>
Cc: Stas Sergeev <[hidden email]>
Cc: Tejun Heo <[hidden email]>
Cc: Thomas Gleixner <[hidden email]>
Cc: Vladimir Davydov <[hidden email]>
Cc: [hidden email]
Link: http://lkml.kernel.org/r/bb996508a600af14b406810c3d58fe0e0d0afe0d.1462296606.git.luto@...
Signed-off-by: Ingo Molnar <[hidden email]>
---
 include/uapi/linux/signal.h               | 2 +-
 tools/testing/selftests/sigaltstack/sas.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/signal.h b/include/uapi/linux/signal.h
index 7388260..cd0804b 100644
--- a/include/uapi/linux/signal.h
+++ b/include/uapi/linux/signal.h
@@ -8,7 +8,7 @@
 #define SS_DISABLE 2
 
 /* bit-flags */
-#define SS_AUTODISARM (1 << 4) /* disable sas during sighandling */
+#define SS_AUTODISARM (1U << 31) /* disable sas during sighandling */
 /* mask for all SS_xxx flags */
 #define SS_FLAG_BITS SS_AUTODISARM
 
diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c
index 4280d06..1bb0125 100644
--- a/tools/testing/selftests/sigaltstack/sas.c
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -18,7 +18,7 @@
 #include <errno.h>
 
 #ifndef SS_AUTODISARM
-#define SS_AUTODISARM  (1 << 4)
+#define SS_AUTODISARM  (1U << 31)
 #endif
 
 static void *sstack, *ustack;
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] signals/sigaltstack: If SS_AUTODISARM, bypass on_sig_stack

Andy Lutomirski
In reply to this post by Ingo Molnar
On May 3, 2016 11:32 PM, "Ingo Molnar" <[hidden email]> wrote:

>
>
> * Andy Lutomirski <[hidden email]> wrote:
>
> > If a signal stack is set up with SS_AUTODISARM, then the kernel
> > inherently avoids incorrectly resetting the signal stack if signals
> > recurse: the signal stack will be reset on the first signal
> > delivery.  This means that we don't need check the stack pointer
> > when delivering signals if SS_AUTODISARM is set.
> >
> > This will make segmented x86 programs more robust: currently there's
> > a hole that could be triggered if ESP/RSP appears to point to the
> > signal stack but actually doesn't due to a nonzero SS base.
> >
> > Signed-off-by: Stas Sergeev <[hidden email]>
>
> Presuably that SOB from Stas is stray, as there's no matching From: line?
> I've removed it.

Yes.  It was a cut-and-paste-o -- I meant to change it to cc.

>
> Thanks,
>
>         Ingo
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] signals/sigaltstack: If SS_AUTODISARM, bypass on_sig_stack

Stas Sergeev-2
In reply to this post by Andy Lutomirski-3
03.05.2016 20:31, Andy Lutomirski пишет:

> If a signal stack is set up with SS_AUTODISARM, then the kernel
> inherently avoids incorrectly resetting the signal stack if signals
> recurse: the signal stack will be reset on the first signal
> delivery.  This means that we don't need check the stack pointer
> when delivering signals if SS_AUTODISARM is set.
>
> This will make segmented x86 programs more robust: currently there's
> a hole that could be triggered if ESP/RSP appears to point to the
> signal stack but actually doesn't due to a nonzero SS base.
>
> Signed-off-by: Stas Sergeev <[hidden email]>
> Cc: Al Viro <[hidden email]>
> Cc: Aleksa Sarai <[hidden email]>
> Cc: Amanieu d'Antras <[hidden email]>
> Cc: Andrea Arcangeli <[hidden email]>
> Cc: Andrew Morton <[hidden email]>
> Cc: Andy Lutomirski <[hidden email]>
> Cc: Borislav Petkov <[hidden email]>
> Cc: Brian Gerst <[hidden email]>
> Cc: Denys Vlasenko <[hidden email]>
> Cc: Eric W. Biederman <[hidden email]>
> Cc: Frederic Weisbecker <[hidden email]>
> Cc: H. Peter Anvin <[hidden email]>
> Cc: Heinrich Schuchardt <[hidden email]>
> Cc: Jason Low <[hidden email]>
> Cc: Josh Triplett <[hidden email]>
> Cc: Konstantin Khlebnikov <[hidden email]>
> Cc: Linus Torvalds <[hidden email]>
> Cc: Oleg Nesterov <[hidden email]>
> Cc: Palmer Dabbelt <[hidden email]>
> Cc: Paul Moore <[hidden email]>
> Cc: Pavel Emelyanov <[hidden email]>
> Cc: Peter Zijlstra <[hidden email]>
> Cc: Richard Weinberger <[hidden email]>
> Cc: Sasha Levin <[hidden email]>
> Cc: Shuah Khan <[hidden email]>
> Cc: Tejun Heo <[hidden email]>
> Cc: Thomas Gleixner <[hidden email]>
> Cc: Vladimir Davydov <[hidden email]>
> Cc: [hidden email]
> Cc: [hidden email]
> Signed-off-by: Andy Lutomirski <[hidden email]>
> ---
>   include/linux/sched.h | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 2950c5cd3005..8f03a93348b9 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2576,6 +2576,18 @@ static inline int kill_cad_pid(int sig, int priv)
>    */
>   static inline int on_sig_stack(unsigned long sp)
>   {
> + /*
> + * If the signal stack is AUTODISARM then, by construction, we
> + * can't be on the signal stack unless user code deliberately set
> + * SS_AUTODISARM when we were already on the it.
"on the it" -> "on it".

Anyway, I am a bit puzzled with this patch.
You say "unless user code deliberately set
SS_AUTODISARM when we were already on the it"
so what happens in case it actually does?

Without your patch: if user sets up the same sas - no stack switch.
if user sets up different sas - stack switch on nested signal.

With your patch: stack switch in any case, so if user
set up same sas - stack corruption by nested signal.

Or am I missing the intention?
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/4] selftests/sigaltstack: Fix the sas test on old kernels

Stas Sergeev-2
In reply to this post by Andy Lutomirski-3
03.05.2016 20:31, Andy Lutomirski пишет:

> The handling for old kernels was wrong.  Fix it.
>
> Reported-by: Ingo Molnar <[hidden email]>
> Cc: Stas Sergeev <[hidden email]>
> Cc: Al Viro <[hidden email]>
> Cc: Andrew Morton <[hidden email]>
> Cc: Andy Lutomirski <[hidden email]>
> Cc: Borislav Petkov <[hidden email]>
> Cc: Brian Gerst <[hidden email]>
> Cc: Denys Vlasenko <[hidden email]>
> Cc: H. Peter Anvin <[hidden email]>
> Cc: Linus Torvalds <[hidden email]>
> Cc: Oleg Nesterov <[hidden email]>
> Cc: Pavel Emelyanov <[hidden email]>
> Cc: Peter Zijlstra <[hidden email]>
> Cc: Shuah Khan <[hidden email]>
> Cc: Thomas Gleixner <[hidden email]>
> Cc: [hidden email]
> Cc: [hidden email]
> Signed-off-by: Andy Lutomirski <[hidden email]>
> ---
>   tools/testing/selftests/sigaltstack/sas.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c
> index 57da8bfde60b..a98c3ef8141f 100644
> --- a/tools/testing/selftests/sigaltstack/sas.c
> +++ b/tools/testing/selftests/sigaltstack/sas.c
> @@ -15,6 +15,7 @@
>   #include <alloca.h>
>   #include <string.h>
>   #include <assert.h>
> +#include <errno.h>
>  
>   #ifndef SS_AUTODISARM
>   #define SS_AUTODISARM  (1 << 4)
> @@ -117,13 +118,19 @@ int main(void)
>   stk.ss_flags = SS_ONSTACK | SS_AUTODISARM;
>   err = sigaltstack(&stk, NULL);
>   if (err) {
> - perror("[FAIL]\tsigaltstack(SS_ONSTACK | SS_AUTODISARM)");
> - stk.ss_flags = SS_ONSTACK;
> - }
> - err = sigaltstack(&stk, NULL);
> - if (err) {
> - perror("[FAIL]\tsigaltstack(SS_ONSTACK)");
> - return EXIT_FAILURE;
> + if (errno == EINVAL) {
> + printf("[NOTE]\tThe running kernel doesn't support SS_AUTODISARM\n");
> + /*
> + * If test cases for the !SS_AUTODISARM variant were
> + * added, we could still run them.  We don't have any
> + * test cases like that yet, so just exit and report
> + * success.
> + */
But that was the point, please see how it handles the
old kernels:

$ ./sas
[FAIL]    sigaltstack(SS_ONSTACK | SS_AUTODISARM): Invalid argument
[RUN]    signal USR1
[FAIL]    ss_flags=1, should be SS_DISABLE
[RUN]    switched to user ctx
[RUN]    signal USR2
[FAIL]    sigaltstack re-used
[FAIL]    Stack corrupted
[RUN]    Aborting

Unfortunalely, for Ingo it crashed...
I am not sure why, I can't reproduce. :(
So if you disable all the "old" tests, you can as well
remove them, or... find the bug and re-enable. :)
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] signals/sigaltstack: Change SS_AUTODISARM to (1U << 31)

Stas Sergeev-2
In reply to this post by Andy Lutomirski-3
03.05.2016 20:31, Andy Lutomirski пишет:

> Using bit 4 divides the space of available bits strangely.  Use bit
> 31 instead so that we have a better chance of keeping flag and mode
> bits separate in the long run.
>
> Cc: Stas Sergeev <[hidden email]>
> Cc: Al Viro <[hidden email]>
> Cc: Aleksa Sarai <[hidden email]>
> Cc: Amanieu d'Antras <[hidden email]>
> Cc: Andrea Arcangeli <[hidden email]>
> Cc: Andrew Morton <[hidden email]>
> Cc: Andy Lutomirski <[hidden email]>
> Cc: Borislav Petkov <[hidden email]>
> Cc: Brian Gerst <[hidden email]>
> Cc: Denys Vlasenko <[hidden email]>
> Cc: Eric W. Biederman <[hidden email]>
> Cc: Frederic Weisbecker <[hidden email]>
> Cc: H. Peter Anvin <[hidden email]>
> Cc: Heinrich Schuchardt <[hidden email]>
> Cc: Jason Low <[hidden email]>
> Cc: Josh Triplett <[hidden email]>
> Cc: Konstantin Khlebnikov <[hidden email]>
> Cc: Linus Torvalds <[hidden email]>
> Cc: Oleg Nesterov <[hidden email]>
> Cc: Palmer Dabbelt <[hidden email]>
> Cc: Paul Moore <[hidden email]>
> Cc: Pavel Emelyanov <[hidden email]>
> Cc: Peter Zijlstra <[hidden email]>
> Cc: Richard Weinberger <[hidden email]>
> Cc: Sasha Levin <[hidden email]>
> Cc: Shuah Khan <[hidden email]>
> Cc: Tejun Heo <[hidden email]>
> Cc: Thomas Gleixner <[hidden email]>
> Cc: Vladimir Davydov <[hidden email]>
> Cc: [hidden email]
> Cc: [hidden email]
> Signed-off-by: Andy Lutomirski <[hidden email]>
> ---
>   include/uapi/linux/signal.h               | 2 +-
>   tools/testing/selftests/sigaltstack/sas.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/signal.h b/include/uapi/linux/signal.h
> index 738826048af2..cd0804b6bfa2 100644
> --- a/include/uapi/linux/signal.h
> +++ b/include/uapi/linux/signal.h
> @@ -8,7 +8,7 @@
>   #define SS_DISABLE 2
>  
>   /* bit-flags */
> -#define SS_AUTODISARM (1 << 4) /* disable sas during sighandling */
> +#define SS_AUTODISARM (1U << 31) /* disable sas during sighandling */
And what if we are out of 32 bits for storing both mode and flags? :)
Well, yes, very unlikely, but I did it that way exactly so that
we can eventually promote to 64bit variable.
Doesn't matter at all, of course. Let it be any way you like.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/4] selftests/sigaltstack: Fix the sas test on old kernels

Andy Lutomirski
In reply to this post by Stas Sergeev-2
On May 7, 2016 8:02 AM, "Stas Sergeev" <[hidden email]> wrote:

>
> 03.05.2016 20:31, Andy Lutomirski пишет:
>
>> The handling for old kernels was wrong.  Fix it.
>>
>> Reported-by: Ingo Molnar <[hidden email]>
>> Cc: Stas Sergeev <[hidden email]>
>> Cc: Al Viro <[hidden email]>
>> Cc: Andrew Morton <[hidden email]>
>> Cc: Andy Lutomirski <[hidden email]>
>> Cc: Borislav Petkov <[hidden email]>
>> Cc: Brian Gerst <[hidden email]>
>> Cc: Denys Vlasenko <[hidden email]>
>> Cc: H. Peter Anvin <[hidden email]>
>> Cc: Linus Torvalds <[hidden email]>
>> Cc: Oleg Nesterov <[hidden email]>
>> Cc: Pavel Emelyanov <[hidden email]>
>> Cc: Peter Zijlstra <[hidden email]>
>> Cc: Shuah Khan <[hidden email]>
>> Cc: Thomas Gleixner <[hidden email]>
>> Cc: [hidden email]
>> Cc: [hidden email]
>> Signed-off-by: Andy Lutomirski <[hidden email]>
>> ---
>>   tools/testing/selftests/sigaltstack/sas.c | 21 ++++++++++++++-------
>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c
>> index 57da8bfde60b..a98c3ef8141f 100644
>> --- a/tools/testing/selftests/sigaltstack/sas.c
>> +++ b/tools/testing/selftests/sigaltstack/sas.c
>> @@ -15,6 +15,7 @@
>>   #include <alloca.h>
>>   #include <string.h>
>>   #include <assert.h>
>> +#include <errno.h>
>>     #ifndef SS_AUTODISARM
>>   #define SS_AUTODISARM  (1 << 4)
>> @@ -117,13 +118,19 @@ int main(void)
>>         stk.ss_flags = SS_ONSTACK | SS_AUTODISARM;
>>         err = sigaltstack(&stk, NULL);
>>         if (err) {
>> -               perror("[FAIL]\tsigaltstack(SS_ONSTACK | SS_AUTODISARM)");
>> -               stk.ss_flags = SS_ONSTACK;
>> -       }
>> -       err = sigaltstack(&stk, NULL);
>> -       if (err) {
>> -               perror("[FAIL]\tsigaltstack(SS_ONSTACK)");
>> -               return EXIT_FAILURE;
>> +               if (errno == EINVAL) {
>> +                       printf("[NOTE]\tThe running kernel doesn't support SS_AUTODISARM\n");
>> +                       /*
>> +                        * If test cases for the !SS_AUTODISARM variant were
>> +                        * added, we could still run them.  We don't have any
>> +                        * test cases like that yet, so just exit and report
>> +                        * success.
>> +                        */
>
> But that was the point, please see how it handles the
> old kernels:
>
> $ ./sas
> [FAIL]    sigaltstack(SS_ONSTACK | SS_AUTODISARM): Invalid argument
> [RUN]    signal USR1
> [FAIL]    ss_flags=1, should be SS_DISABLE
> [RUN]    switched to user ctx
> [RUN]    signal USR2
> [FAIL]    sigaltstack re-used
> [FAIL]    Stack corrupted
> [RUN]    Aborting

This is useful as a demonstration of why the feature is useful, but it
doesn't indicate that anything is wrong with old kernels per she.
That's why I changed it to simply report that the feature is missing.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] signals/sigaltstack: If SS_AUTODISARM, bypass on_sig_stack

Andy Lutomirski
In reply to this post by Stas Sergeev-2
On May 7, 2016 7:38 AM, "Stas Sergeev" <[hidden email]> wrote:

>
> 03.05.2016 20:31, Andy Lutomirski пишет:
>
>> If a signal stack is set up with SS_AUTODISARM, then the kernel
>> inherently avoids incorrectly resetting the signal stack if signals
>> recurse: the signal stack will be reset on the first signal
>> delivery.  This means that we don't need check the stack pointer
>> when delivering signals if SS_AUTODISARM is set.
>>
>> This will make segmented x86 programs more robust: currently there's
>> a hole that could be triggered if ESP/RSP appears to point to the
>> signal stack but actually doesn't due to a nonzero SS base.
>>
>> Signed-off-by: Stas Sergeev <[hidden email]>
>> Cc: Al Viro <[hidden email]>
>> Cc: Aleksa Sarai <[hidden email]>
>> Cc: Amanieu d'Antras <[hidden email]>
>> Cc: Andrea Arcangeli <[hidden email]>
>> Cc: Andrew Morton <[hidden email]>
>> Cc: Andy Lutomirski <[hidden email]>
>> Cc: Borislav Petkov <[hidden email]>
>> Cc: Brian Gerst <[hidden email]>
>> Cc: Denys Vlasenko <[hidden email]>
>> Cc: Eric W. Biederman <[hidden email]>
>> Cc: Frederic Weisbecker <[hidden email]>
>> Cc: H. Peter Anvin <[hidden email]>
>> Cc: Heinrich Schuchardt <[hidden email]>
>> Cc: Jason Low <[hidden email]>
>> Cc: Josh Triplett <[hidden email]>
>> Cc: Konstantin Khlebnikov <[hidden email]>
>> Cc: Linus Torvalds <[hidden email]>
>> Cc: Oleg Nesterov <[hidden email]>
>> Cc: Palmer Dabbelt <[hidden email]>
>> Cc: Paul Moore <[hidden email]>
>> Cc: Pavel Emelyanov <[hidden email]>
>> Cc: Peter Zijlstra <[hidden email]>
>> Cc: Richard Weinberger <[hidden email]>
>> Cc: Sasha Levin <[hidden email]>
>> Cc: Shuah Khan <[hidden email]>
>> Cc: Tejun Heo <[hidden email]>
>> Cc: Thomas Gleixner <[hidden email]>
>> Cc: Vladimir Davydov <[hidden email]>
>> Cc: [hidden email]
>> Cc: [hidden email]
>> Signed-off-by: Andy Lutomirski <[hidden email]>
>> ---
>>   include/linux/sched.h | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 2950c5cd3005..8f03a93348b9 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -2576,6 +2576,18 @@ static inline int kill_cad_pid(int sig, int priv)
>>    */
>>   static inline int on_sig_stack(unsigned long sp)
>>   {
>> +       /*
>> +        * If the signal stack is AUTODISARM then, by construction, we
>> +        * can't be on the signal stack unless user code deliberately set
>> +        * SS_AUTODISARM when we were already on the it.
>
> "on the it" -> "on it".
>
> Anyway, I am a bit puzzled with this patch.
> You say "unless user code deliberately set
>
> SS_AUTODISARM when we were already on the it"
> so what happens in case it actually does?
>

Stack corruption.  Don't do that.

> Without your patch: if user sets up the same sas - no stack switch.
> if user sets up different sas - stack switch on nested signal.
>
> With your patch: stack switch in any case, so if user
> set up same sas - stack corruption by nested signal.
>
> Or am I missing the intention?

The intention is to make everything completely explicit.  With
SS_AUTODISARM, the kernel knows directly whether you're on the signal
stack, and there should be no need to look at sp.  If you set
SS_AUTODISARM and get a signal, the signal stack gets disarmed.  If
you take a nested signal, it's delivered normally.  When you return
all the way out, the signal stack is re-armed.

For DOSEMU, this means that no 16-bit register state can possibly
cause a signal to be delivered wrong, because the register state when
a signal is raised won't affect delivery, which seems like a good
thing to me.

If this behavior would be problematic for you, can you explain why?
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] signals/sigaltstack: If SS_AUTODISARM, bypass on_sig_stack

Stas Sergeev-2
09.05.2016 04:32, Andy Lutomirski пишет:

> On May 7, 2016 7:38 AM, "Stas Sergeev" <[hidden email]> wrote:
>> 03.05.2016 20:31, Andy Lutomirski пишет:
>>
>>> If a signal stack is set up with SS_AUTODISARM, then the kernel
>>> inherently avoids incorrectly resetting the signal stack if signals
>>> recurse: the signal stack will be reset on the first signal
>>> delivery.  This means that we don't need check the stack pointer
>>> when delivering signals if SS_AUTODISARM is set.
>>>
>>> This will make segmented x86 programs more robust: currently there's
>>> a hole that could be triggered if ESP/RSP appears to point to the
>>> signal stack but actually doesn't due to a nonzero SS base.
>>>
>>> Signed-off-by: Stas Sergeev <[hidden email]>
>>> Cc: Al Viro <[hidden email]>
>>> Cc: Aleksa Sarai <[hidden email]>
>>> Cc: Amanieu d'Antras <[hidden email]>
>>> Cc: Andrea Arcangeli <[hidden email]>
>>> Cc: Andrew Morton <[hidden email]>
>>> Cc: Andy Lutomirski <[hidden email]>
>>> Cc: Borislav Petkov <[hidden email]>
>>> Cc: Brian Gerst <[hidden email]>
>>> Cc: Denys Vlasenko <[hidden email]>
>>> Cc: Eric W. Biederman <[hidden email]>
>>> Cc: Frederic Weisbecker <[hidden email]>
>>> Cc: H. Peter Anvin <[hidden email]>
>>> Cc: Heinrich Schuchardt <[hidden email]>
>>> Cc: Jason Low <[hidden email]>
>>> Cc: Josh Triplett <[hidden email]>
>>> Cc: Konstantin Khlebnikov <[hidden email]>
>>> Cc: Linus Torvalds <[hidden email]>
>>> Cc: Oleg Nesterov <[hidden email]>
>>> Cc: Palmer Dabbelt <[hidden email]>
>>> Cc: Paul Moore <[hidden email]>
>>> Cc: Pavel Emelyanov <[hidden email]>
>>> Cc: Peter Zijlstra <[hidden email]>
>>> Cc: Richard Weinberger <[hidden email]>
>>> Cc: Sasha Levin <[hidden email]>
>>> Cc: Shuah Khan <[hidden email]>
>>> Cc: Tejun Heo <[hidden email]>
>>> Cc: Thomas Gleixner <[hidden email]>
>>> Cc: Vladimir Davydov <[hidden email]>
>>> Cc: [hidden email]
>>> Cc: [hidden email]
>>> Signed-off-by: Andy Lutomirski <[hidden email]>
>>> ---
>>>    include/linux/sched.h | 12 ++++++++++++
>>>    1 file changed, 12 insertions(+)
>>>
>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>> index 2950c5cd3005..8f03a93348b9 100644
>>> --- a/include/linux/sched.h
>>> +++ b/include/linux/sched.h
>>> @@ -2576,6 +2576,18 @@ static inline int kill_cad_pid(int sig, int priv)
>>>     */
>>>    static inline int on_sig_stack(unsigned long sp)
>>>    {
>>> +       /*
>>> +        * If the signal stack is AUTODISARM then, by construction, we
>>> +        * can't be on the signal stack unless user code deliberately set
>>> +        * SS_AUTODISARM when we were already on the it.
>> "on the it" -> "on it".
>>
>> Anyway, I am a bit puzzled with this patch.
>> You say "unless user code deliberately set
>>
>> SS_AUTODISARM when we were already on the it"
>> so what happens in case it actually does?
>>
> Stack corruption.  Don't do that.
Only after your change, I have to admit. :)

>> Without your patch: if user sets up the same sas - no stack switch.
>> if user sets up different sas - stack switch on nested signal.
>>
>> With your patch: stack switch in any case, so if user
>> set up same sas - stack corruption by nested signal.
>>
>> Or am I missing the intention?
> The intention is to make everything completely explicit.  With
> SS_AUTODISARM, the kernel knows directly whether you're on the signal
> stack, and there should be no need to look at sp.  If you set
> SS_AUTODISARM and get a signal, the signal stack gets disarmed.  If
> you take a nested signal, it's delivered normally.  When you return
> all the way out, the signal stack is re-armed.
>
> For DOSEMU, this means that no 16-bit register state can possibly
> cause a signal to be delivered wrong, because the register state when
> a signal is raised won't affect delivery, which seems like a good
> thing to me.
Yes, but doesn't affect dosemu1 which doesn't use SS_AUTODISARM.
So IMHO the SS check should still be added, even if not for dosemu2.

> If this behavior would be problematic for you, can you explain why?
Only theoretically: if someone sets SS_AUTODISARM inside a
sighandler. Since this doesn't give EPERM, I wouldn't deliberately
make it a broken scenario (esp if it wasn't before the particular change).
Ideally it would give EPERM, but we can't, so doesn't matter much.
I just wanted to warn about the possible regression.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] signals/sigaltstack: If SS_AUTODISARM, bypass on_sig_stack

Andy Lutomirski
On May 8, 2016 7:05 PM, "Stas Sergeev" <[hidden email]> wrote:

>
> 09.05.2016 04:32, Andy Lutomirski пишет:
>
>> On May 7, 2016 7:38 AM, "Stas Sergeev" <[hidden email]> wrote:
>>>
>>> 03.05.2016 20:31, Andy Lutomirski пишет:
>>>
>>>> If a signal stack is set up with SS_AUTODISARM, then the kernel
>>>> inherently avoids incorrectly resetting the signal stack if signals
>>>> recurse: the signal stack will be reset on the first signal
>>>> delivery.  This means that we don't need check the stack pointer
>>>> when delivering signals if SS_AUTODISARM is set.
>>>>
>>>> This will make segmented x86 programs more robust: currently there's
>>>> a hole that could be triggered if ESP/RSP appears to point to the
>>>> signal stack but actually doesn't due to a nonzero SS base.
>>>>
>>>> Signed-off-by: Stas Sergeev <[hidden email]>
>>>> Cc: Al Viro <[hidden email]>
>>>> Cc: Aleksa Sarai <[hidden email]>
>>>> Cc: Amanieu d'Antras <[hidden email]>
>>>> Cc: Andrea Arcangeli <[hidden email]>
>>>> Cc: Andrew Morton <[hidden email]>
>>>> Cc: Andy Lutomirski <[hidden email]>
>>>> Cc: Borislav Petkov <[hidden email]>
>>>> Cc: Brian Gerst <[hidden email]>
>>>> Cc: Denys Vlasenko <[hidden email]>
>>>> Cc: Eric W. Biederman <[hidden email]>
>>>> Cc: Frederic Weisbecker <[hidden email]>
>>>> Cc: H. Peter Anvin <[hidden email]>
>>>> Cc: Heinrich Schuchardt <[hidden email]>
>>>> Cc: Jason Low <[hidden email]>
>>>> Cc: Josh Triplett <[hidden email]>
>>>> Cc: Konstantin Khlebnikov <[hidden email]>
>>>> Cc: Linus Torvalds <[hidden email]>
>>>> Cc: Oleg Nesterov <[hidden email]>
>>>> Cc: Palmer Dabbelt <[hidden email]>
>>>> Cc: Paul Moore <[hidden email]>
>>>> Cc: Pavel Emelyanov <[hidden email]>
>>>> Cc: Peter Zijlstra <[hidden email]>
>>>> Cc: Richard Weinberger <[hidden email]>
>>>> Cc: Sasha Levin <[hidden email]>
>>>> Cc: Shuah Khan <[hidden email]>
>>>> Cc: Tejun Heo <[hidden email]>
>>>> Cc: Thomas Gleixner <[hidden email]>
>>>> Cc: Vladimir Davydov <[hidden email]>
>>>> Cc: [hidden email]
>>>> Cc: [hidden email]
>>>> Signed-off-by: Andy Lutomirski <[hidden email]>
>>>> ---
>>>>    include/linux/sched.h | 12 ++++++++++++
>>>>    1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>>> index 2950c5cd3005..8f03a93348b9 100644
>>>> --- a/include/linux/sched.h
>>>> +++ b/include/linux/sched.h
>>>> @@ -2576,6 +2576,18 @@ static inline int kill_cad_pid(int sig, int priv)
>>>>     */
>>>>    static inline int on_sig_stack(unsigned long sp)
>>>>    {
>>>> +       /*
>>>> +        * If the signal stack is AUTODISARM then, by construction, we
>>>> +        * can't be on the signal stack unless user code deliberately set
>>>> +        * SS_AUTODISARM when we were already on the it.
>>>
>>> "on the it" -> "on it".
>>>
>>> Anyway, I am a bit puzzled with this patch.
>>> You say "unless user code deliberately set
>>>
>>> SS_AUTODISARM when we were already on the it"
>>> so what happens in case it actually does?
>>>
>> Stack corruption.  Don't do that.
>
> Only after your change, I have to admit. :)
>
>
>>> Without your patch: if user sets up the same sas - no stack switch.
>>> if user sets up different sas - stack switch on nested signal.
>>>
>>> With your patch: stack switch in any case, so if user
>>> set up same sas - stack corruption by nested signal.
>>>
>>> Or am I missing the intention?
>>
>> The intention is to make everything completely explicit.  With
>> SS_AUTODISARM, the kernel knows directly whether you're on the signal
>> stack, and there should be no need to look at sp.  If you set
>> SS_AUTODISARM and get a signal, the signal stack gets disarmed.  If
>> you take a nested signal, it's delivered normally.  When you return
>> all the way out, the signal stack is re-armed.
>>
>> For DOSEMU, this means that no 16-bit register state can possibly
>> cause a signal to be delivered wrong, because the register state when
>> a signal is raised won't affect delivery, which seems like a good
>> thing to me.
>
> Yes, but doesn't affect dosemu1 which doesn't use SS_AUTODISARM.
> So IMHO the SS check should still be added, even if not for dosemu2.
>
>
>> If this behavior would be problematic for you, can you explain why?
>
> Only theoretically: if someone sets SS_AUTODISARM inside a
> sighandler. Since this doesn't give EPERM, I wouldn't deliberately
> make it a broken scenario (esp if it wasn't before the particular change).
> Ideally it would give EPERM, but we can't, so doesn't matter much.
> I just wanted to warn about the possible regression.

I suppose we could return an error if you are on the sigstack when
setting SS_AUTODISARM, although I was hoping to avoid yet more special
cases.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
12