[PATCH 3/4] x86: signal: unify the sigaltstack check with other arches

[PATCH 3/4] x86: signal: unify the sigaltstack check with other arches

[Stas Sergeev]

Currently x86's get_sigframe() checks for "current->sas_ss_size"
to determine whether there is a need to switch to sigaltstack.
The common practice used by all other arches is to check for
sas_ss_flags(sp) == 0

This patch makes the code consistent with other arches and also
allows for the further sigaltstack improvements within this patch serie.

CC: Andy Lutomirski <luto@amacapital.net>
CC: linux-kernel@vger.kernel.org
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
CC: Borislav Petkov <bp@suse.de>
CC: Brian Gerst <brgerst@gmail.com>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Richard Weinberger <richard@nod.at>

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
  arch/x86/kernel/signal.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index cb6282c..3955259 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -216,7 +216,7 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs 
*regs, size_t frame_size,
      if (!onsigstack) {
          /* This is the X/Open sanctioned signal stack switching. */
          if (ka->sa.sa_flags & SA_ONSTACK) {
-            if (current->sas_ss_size)
+            if (sas_ss_flags(sp) == 0)
                  sp = current->sas_ss_sp + current->sas_ss_size;
          } else if (config_enabled(CONFIG_X86_32) &&
                 (regs->ss & 0xffff) != __USER_DS &&
-- 
2.5.0

Re: [PATCH 3/4] x86: signal: unify the sigaltstack check with other arches

[Andy Lutomirski]

On Sun, Jan 31, 2016 at 8:24 AM, Stas Sergeev <stsp@list.ru> wrote:
>
> Currently x86's get_sigframe() checks for "current->sas_ss_size"
> to determine whether there is a need to switch to sigaltstack.
> The common practice used by all other arches is to check for
> sas_ss_flags(sp) == 0
>
> This patch makes the code consistent with other arches and also
> allows for the further sigaltstack improvements within this patch serie.

You've duplicate part of the check.  That whole block is already
checking !sigstack.  Can you remove that now?

--Andy

Re: [PATCH 3/4] x86: signal: unify the sigaltstack check with other arches

[Stas Sergeev]

31.01.2016 19:58, Andy Lutomirski пишет:
> On Sun, Jan 31, 2016 at 8:24 AM, Stas Sergeev <stsp@list.ru> wrote:
>> Currently x86's get_sigframe() checks for "current->sas_ss_size"
>> to determine whether there is a need to switch to sigaltstack.
>> The common practice used by all other arches is to check for
>> sas_ss_flags(sp) == 0
>>
>> This patch makes the code consistent with other arches and also
>> allows for the further sigaltstack improvements within this patch serie.
> You've duplicate part of the check.  That whole block is already
> checking !sigstack.  Can you remove that now?
OK.

[PATCH v2 0/4] make sigaltstack() compatible with swapcontext()

[Stas Sergeev]

The following patches add the self-test for
sigaltstack(SS_DISABLE) inside the signal handler,
and allow an app to temporarily disable and re-enable
the sigaltstack within a sighandler.
This is needed to make sigaltstack() compatible with
swapcontext(): before using swapcontext() inside
the sighandler, the sigaltstack should be disabled, or
the stack will be corrupted by the subsequent signals.


v2: avoid duplicating the "!sigstack" check in get_sigframe()
as requested by Andy Lutomirski.



  arch/score/kernel/signal.c                   |    2
  arch/x86/kernel/signal.c                     |    2
  include/linux/sched.h                        |    8 +
  include/linux/signal.h                       |    2
  kernel/fork.c                                |    4
  kernel/signal.c                              |   54 +++++++----
  tools/testing/selftests/Makefile             |    1
  tools/testing/selftests/sigaltstack/Makefile |    8 +
  tools/testing/selftests/sigaltstack/sas.c    |  132 
+++++++++++++++++++++++++++
  9 files changed, 189 insertions(+), 24 deletions(-)

[PATCH 1/4] selftests: Add test for sigaltstack(SS_DISABLE) inside sighandler

[Stas Sergeev]

sigaltstack needs to be disabled before the signal handler can
safely use swapcontext(). Unfortunately linux implementation of
sigaltstack() returns EPERM in that case.
Re-enabling is also needed and tested.

CC: Shuah Khan <shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
CC: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>

Signed-off-by: Stas Sergeev <stsp-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
  tools/testing/selftests/Makefile             |   1 +
  tools/testing/selftests/sigaltstack/Makefile |   8 ++
  tools/testing/selftests/sigaltstack/sas.c    | 132 
+++++++++++++++++++++++++++
  3 files changed, 141 insertions(+)
  create mode 100644 tools/testing/selftests/sigaltstack/Makefile
  create mode 100644 tools/testing/selftests/sigaltstack/sas.c

diff --git a/tools/testing/selftests/Makefile 
b/tools/testing/selftests/Makefile
index b04afc3..ff9e5f2 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -19,6 +19,7 @@ TARGETS += powerpc
  TARGETS += pstore
  TARGETS += ptrace
  TARGETS += seccomp
+TARGETS += sigaltstack
  TARGETS += size
  TARGETS += static_keys
  TARGETS += sysctl
diff --git a/tools/testing/selftests/sigaltstack/Makefile 
b/tools/testing/selftests/sigaltstack/Makefile
new file mode 100644
index 0000000..56af56e
--- /dev/null
+++ b/tools/testing/selftests/sigaltstack/Makefile
@@ -0,0 +1,8 @@
+CFLAGS = -Wall
+BINARIES = sas
+all: $(BINARIES)
+
+include ../lib.mk
+
+clean:
+    rm -rf $(BINARIES)
diff --git a/tools/testing/selftests/sigaltstack/sas.c 
b/tools/testing/selftests/sigaltstack/sas.c
new file mode 100644
index 0000000..5d9aabd
--- /dev/null
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -0,0 +1,132 @@
+/*
+ * Stas Sergeev <stsp-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
+ *
+ * test sigcontext(SS_DISABLE) inside signal handler
+ * If that succeeds, then swapcontext() can be used safely.
+ *
+ */
+
+#define _GNU_SOURCE
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <ucontext.h>
+#include <alloca.h>
+#include <string.h>
+#include <assert.h>
+
+static void *sstack, *ustack;
+static ucontext_t uc, sc;
+static const char *msg = "[OK]\tStack preserved";
+static const char *msg2 = "[FAIL]\tStack corrupted";
+
+void my_usr1(int sig)
+{
+    char *aa, *p;
+    int *i, err;
+    stack_t stk = { };
+
+    aa = alloca(1024);
+    assert(aa);
+    p = aa + 512;
+    strcpy(p, msg);
+    i = (int *) (p + 128);
+    *i = 1;
+    printf("[RUN]\tsignal USR1\n");
+    stk.ss_flags = SS_DISABLE;
+    err = sigaltstack(&stk, &stk);
+    if (err) {
+        perror("[FAIL]\tsigaltstack(SS_DISABLE)");
+        /* don't exit to demonstrate the breakage */
+        /* exit(EXIT_FAILURE); */
+    }
+    swapcontext(&sc, &uc);
+    printf("%s\n", p);
+    if (!*i) {
+        printf("[RUN]\tAborting\n");
+        exit(EXIT_FAILURE);
+    }
+
+    if (stk.ss_flags != SS_ONSTACK) {
+        printf("[FAIL]\tsigaltstack() returned wrong ss_flags %i\n",
+                stk.ss_flags);
+        stk.ss_flags = SS_ONSTACK;
+    }
+    err = sigaltstack(&stk, NULL);
+    if (err)
+        printf("[OK]\tsigaltstack(SS_ONSTACK) failed for non_zero "
+                "size\n");
+        /* but don't fail otherwise, as this is unspecified */
+    stk.ss_size = 0;
+    err = sigaltstack(&stk, NULL);
+    if (err)
+        perror("[FAIL]\tsigaltstack(SS_ONSTACK)");
+}
+
+void my_usr2(int sig)
+{
+    char *aa, *p;
+    int *i;
+
+    printf("[RUN]\tsignal USR2\n");
+    aa = alloca(1024);
+    /* dont run valgrind on this */
+    p = memmem(aa, 1024, msg, strlen(msg));
+    if (p) {
+        printf("[FAIL]\tsigaltstack re-used\n");
+        strcpy(p, msg2);
+        i = (int *) (p + 128);
+        *i = 0;
+    }
+}
+
+static void switch_fn(void)
+{
+    printf("[RUN]\tswitched to user ctx\n");
+    raise(SIGUSR2);
+    setcontext(&sc);
+}
+
+int main(void)
+{
+    struct sigaction act;
+    stack_t stk;
+    int err;
+
+    sigemptyset(&act.sa_mask);
+    act.sa_flags = SA_ONSTACK;
+    act.sa_handler = my_usr1;
+    sigaction(SIGUSR1, &act, NULL);
+    act.sa_handler = my_usr2;
+    sigaction(SIGUSR2, &act, NULL);
+    sstack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
+              MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
+    if (sstack == MAP_FAILED) {
+        perror("mmap()");
+        return EXIT_FAILURE;
+    }
+    stk.ss_sp = sstack;
+    stk.ss_size = SIGSTKSZ;
+    stk.ss_flags = SS_ONSTACK;
+    err = sigaltstack(&stk, NULL);
+    if (err) {
+        perror("sigaltstack()");
+        return EXIT_FAILURE;
+    }
+
+    ustack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
+              MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
+    if (ustack == MAP_FAILED) {
+        perror("mmap()");
+        return EXIT_FAILURE;
+    }
+    getcontext(&uc);
+    uc.uc_link = NULL;
+    uc.uc_stack.ss_sp = ustack;
+    uc.uc_stack.ss_size = SIGSTKSZ;
+    makecontext(&uc, switch_fn, 0);
+    raise(SIGUSR1);
+    printf("[OK]\tTest passed\n");
+    return 0;
+}
-- 
2.5.0

[PATCH 1/4] selftests: Add test for sigaltstack(SS_DISABLE) inside sighandler

[Stas Sergeev]

sigaltstack needs to be disabled before the signal handler can
safely use swapcontext(). Unfortunately linux implementation of
sigaltstack() returns EPERM in that case.
Re-enabling is also needed and tested.

CC: Shuah Khan <shuahkh@osg.samsung.com>
CC: linux-kernel@vger.kernel.org
CC: linux-api@vger.kernel.org
CC: Andy Lutomirski <luto@amacapital.net>

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
  tools/testing/selftests/Makefile             |   1 +
  tools/testing/selftests/sigaltstack/Makefile |   8 ++
  tools/testing/selftests/sigaltstack/sas.c    | 132 
+++++++++++++++++++++++++++
  3 files changed, 141 insertions(+)
  create mode 100644 tools/testing/selftests/sigaltstack/Makefile
  create mode 100644 tools/testing/selftests/sigaltstack/sas.c

diff --git a/tools/testing/selftests/Makefile 
b/tools/testing/selftests/Makefile
index b04afc3..ff9e5f2 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -19,6 +19,7 @@ TARGETS += powerpc
  TARGETS += pstore
  TARGETS += ptrace
  TARGETS += seccomp
+TARGETS += sigaltstack
  TARGETS += size
  TARGETS += static_keys
  TARGETS += sysctl
diff --git a/tools/testing/selftests/sigaltstack/Makefile 
b/tools/testing/selftests/sigaltstack/Makefile
new file mode 100644
index 0000000..56af56e
--- /dev/null
+++ b/tools/testing/selftests/sigaltstack/Makefile
@@ -0,0 +1,8 @@
+CFLAGS = -Wall
+BINARIES = sas
+all: $(BINARIES)
+
+include ../lib.mk
+
+clean:
+    rm -rf $(BINARIES)
diff --git a/tools/testing/selftests/sigaltstack/sas.c 
b/tools/testing/selftests/sigaltstack/sas.c
new file mode 100644
index 0000000..5d9aabd
--- /dev/null
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -0,0 +1,132 @@
+/*
+ * Stas Sergeev <stsp@users.sourceforge.net>
+ *
+ * test sigcontext(SS_DISABLE) inside signal handler
+ * If that succeeds, then swapcontext() can be used safely.
+ *
+ */
+
+#define _GNU_SOURCE
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <ucontext.h>
+#include <alloca.h>
+#include <string.h>
+#include <assert.h>
+
+static void *sstack, *ustack;
+static ucontext_t uc, sc;
+static const char *msg = "[OK]\tStack preserved";
+static const char *msg2 = "[FAIL]\tStack corrupted";
+
+void my_usr1(int sig)
+{
+    char *aa, *p;
+    int *i, err;
+    stack_t stk = { };
+
+    aa = alloca(1024);
+    assert(aa);
+    p = aa + 512;
+    strcpy(p, msg);
+    i = (int *) (p + 128);
+    *i = 1;
+    printf("[RUN]\tsignal USR1\n");
+    stk.ss_flags = SS_DISABLE;
+    err = sigaltstack(&stk, &stk);
+    if (err) {
+        perror("[FAIL]\tsigaltstack(SS_DISABLE)");
+        /* don't exit to demonstrate the breakage */
+        /* exit(EXIT_FAILURE); */
+    }
+    swapcontext(&sc, &uc);
+    printf("%s\n", p);
+    if (!*i) {
+        printf("[RUN]\tAborting\n");
+        exit(EXIT_FAILURE);
+    }
+
+    if (stk.ss_flags != SS_ONSTACK) {
+        printf("[FAIL]\tsigaltstack() returned wrong ss_flags %i\n",
+                stk.ss_flags);
+        stk.ss_flags = SS_ONSTACK;
+    }
+    err = sigaltstack(&stk, NULL);
+    if (err)
+        printf("[OK]\tsigaltstack(SS_ONSTACK) failed for non_zero "
+                "size\n");
+        /* but don't fail otherwise, as this is unspecified */
+    stk.ss_size = 0;
+    err = sigaltstack(&stk, NULL);
+    if (err)
+        perror("[FAIL]\tsigaltstack(SS_ONSTACK)");
+}
+
+void my_usr2(int sig)
+{
+    char *aa, *p;
+    int *i;
+
+    printf("[RUN]\tsignal USR2\n");
+    aa = alloca(1024);
+    /* dont run valgrind on this */
+    p = memmem(aa, 1024, msg, strlen(msg));
+    if (p) {
+        printf("[FAIL]\tsigaltstack re-used\n");
+        strcpy(p, msg2);
+        i = (int *) (p + 128);
+        *i = 0;
+    }
+}
+
+static void switch_fn(void)
+{
+    printf("[RUN]\tswitched to user ctx\n");
+    raise(SIGUSR2);
+    setcontext(&sc);
+}
+
+int main(void)
+{
+    struct sigaction act;
+    stack_t stk;
+    int err;
+
+    sigemptyset(&act.sa_mask);
+    act.sa_flags = SA_ONSTACK;
+    act.sa_handler = my_usr1;
+    sigaction(SIGUSR1, &act, NULL);
+    act.sa_handler = my_usr2;
+    sigaction(SIGUSR2, &act, NULL);
+    sstack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
+              MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
+    if (sstack == MAP_FAILED) {
+        perror("mmap()");
+        return EXIT_FAILURE;
+    }
+    stk.ss_sp = sstack;
+    stk.ss_size = SIGSTKSZ;
+    stk.ss_flags = SS_ONSTACK;
+    err = sigaltstack(&stk, NULL);
+    if (err) {
+        perror("sigaltstack()");
+        return EXIT_FAILURE;
+    }
+
+    ustack = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
+              MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
+    if (ustack == MAP_FAILED) {
+        perror("mmap()");
+        return EXIT_FAILURE;
+    }
+    getcontext(&uc);
+    uc.uc_link = NULL;
+    uc.uc_stack.ss_sp = ustack;
+    uc.uc_stack.ss_size = SIGSTKSZ;
+    makecontext(&uc, switch_fn, 0);
+    raise(SIGUSR1);
+    printf("[OK]\tTest passed\n");
+    return 0;
+}
-- 
2.5.0

[PATCH 2/4] score: signal: fix sigaltstack check

[Stas Sergeev]

Currently get_sigframe() checks only
(ka->sa.sa_flags & SA_ONSTACK) && (!on_sig_stack(sp))
to determine whether the switch to sigaltstack is needed.
It forgets to checks whether the sigaltstack was previously set.

This patch replaces the !on_sig_stack(sp) with the standard check
sas_ss_flags(sp) == 0
that takes into account both conditions: it succeeds only if the
sigaltstack is enabled but currently not active.

CC: Andy Lutomirski <luto@amacapital.net>
CC: linux-kernel@vger.kernel.org
CC: Chen Liqin <liqin.linux@gmail.com>
CC: Lennox Wu <lennox.wu@gmail.com>
CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: James Hogan <james.hogan@imgtec.com>

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
  arch/score/kernel/signal.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/score/kernel/signal.c b/arch/score/kernel/signal.c
index e381c8c..bd1c7c8 100644
--- a/arch/score/kernel/signal.c
+++ b/arch/score/kernel/signal.c
@@ -127,7 +127,7 @@ static void __user *get_sigframe(struct k_sigaction *ka,
      sp -= 32;

      /* This is the X/Open sanctioned signal stack switching.  */
-    if ((ka->sa.sa_flags & SA_ONSTACK) && (!on_sig_stack(sp)))
+    if ((ka->sa.sa_flags & SA_ONSTACK) && (sas_ss_flags(sp) == 0))
          sp = current->sas_ss_sp + current->sas_ss_size;

      return (void __user*)((sp - frame_size) & ~7);
-- 
2.5.0

[PATCH 3/4] x86: signal: unify the sigaltstack check with other arches

[Stas Sergeev]

Currently x86's get_sigframe() checks for "current->sas_ss_size"
to determine whether there is a need to switch to sigaltstack.
The common practice used by all other arches is to check for
sas_ss_flags(sp) == 0

This patch makes the code consistent with other arches and also
allows for the further sigaltstack improvements within this patch serie.
The slight complexity of the patch is added by the optimization on
!sigstack check that was requested by Andy Lutomirski: sas_ss_flags(sp)==0
already implies that we are not on a sigstack, so the code is shuffled
to avoid the duplicate checking.

CC: Andy Lutomirski <luto@amacapital.net>
CC: linux-kernel@vger.kernel.org
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
CC: Borislav Petkov <bp@suse.de>
CC: Brian Gerst <brgerst@gmail.com>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Richard Weinberger <richard@nod.at>

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
  arch/x86/kernel/signal.c | 23 +++++++++++------------
  1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index cb6282c..285183b 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -213,18 +213,17 @@ get_sigframe(struct k_sigaction *ka, struct 
pt_regs *regs, size_t frame_size,
      if (config_enabled(CONFIG_X86_64))
          sp -= 128;

-    if (!onsigstack) {
-        /* This is the X/Open sanctioned signal stack switching. */
-        if (ka->sa.sa_flags & SA_ONSTACK) {
-            if (current->sas_ss_size)
-                sp = current->sas_ss_sp + current->sas_ss_size;
-        } else if (config_enabled(CONFIG_X86_32) &&
-               (regs->ss & 0xffff) != __USER_DS &&
-               !(ka->sa.sa_flags & SA_RESTORER) &&
-               ka->sa.sa_restorer) {
-                /* This is the legacy signal stack switching. */
-                sp = (unsigned long) ka->sa.sa_restorer;
-        }
+    /* This is the X/Open sanctioned signal stack switching.  */
+    if (ka->sa.sa_flags & SA_ONSTACK) {
+        if (sas_ss_flags(sp) == 0)
+            sp = current->sas_ss_sp + current->sas_ss_size;
+    } else if (config_enabled(CONFIG_X86_32) &&
+           !onsigstack &&
+           (regs->ss & 0xffff) != __USER_DS &&
+           !(ka->sa.sa_flags & SA_RESTORER) &&
+           ka->sa.sa_restorer) {
+        /* This is the legacy signal stack switching. */
+        sp = (unsigned long) ka->sa.sa_restorer;
      }

      if (fpu->fpstate_active) {
-- 
2.5.0

[PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

linux implements the sigaltstack() in a way that makes it impossible to
use with swapcontext(). Per the man page, sigaltstack is allowed to return
EPERM if the process is altering its sigaltstack while running on
sigaltstack.
This is likely needed to consistently return oss->ss_flags, that indicates
whether the process is being on sigaltstack or not.
Unfortunately, linux takes that permission to return EPERM too literally:
it returns EPERM even if you don't want to change to another sigaltstack,
but only want to temporarily disable sigaltstack with SS_DISABLE.
You can't use swapcontext() without disabling sigaltstack first, or the
stack will be re-used and overwritten by a subsequent signal.

With this patch, disabling sigaltstack inside a signal handler became
possible, and the swapcontext() can then be used safely. After switching
back to the sighandler, the app can re-enable the sigatlstack.
The oss->ss_flags will correctly indicate the current use of sigaltstack,
even if it is temporarily disabled. Any attempt to modify the sigaltstack
(rather than to disable or re-enable it) within the sighandler, will still
be punished with EPERM as suggested by POSIX.

CC: Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
CC: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
CC: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: "Amanieu d'Antras" <amanieu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
CC: Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org>
CC: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
CC: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
CC: "Kirill A. Shutemov" <kirill.shutemov-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
CC: Jason Low <jason.low2-VXdhtT5mjnY@public.gmane.org>
CC: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
CC: Andrea Arcangeli <aarcange-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: Konstantin Khlebnikov <khlebnikov-XoJtRXgx1JseBXzfvpsJ4g@public.gmane.org>
CC: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org>
CC: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
CC: Aleksa Sarai <cyphar-gVpy/LI/lHzQT0dZR+AlfA@public.gmane.org>
CC: Paul Moore <pmoore-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
CC: Palmer Dabbelt <palmer-96lFi9zoCfxBDgjK7y7TUQ@public.gmane.org>
CC: Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
CC: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Signed-off-by: Stas Sergeev <stsp-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
  include/linux/sched.h  |  8 ++++++--
  include/linux/signal.h |  2 +-
  kernel/fork.c          |  4 +++-
  kernel/signal.c        | 54 
+++++++++++++++++++++++++++++++++-----------------
  4 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index a10494a..38f67dc 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1587,6 +1587,7 @@ struct task_struct {

      unsigned long sas_ss_sp;
      size_t sas_ss_size;
+    int sas_ss_flags;

      struct callback_head *task_works;

@@ -2569,8 +2570,11 @@ static inline int sas_ss_flags(unsigned long sp)
  {
      if (!current->sas_ss_size)
          return SS_DISABLE;
-
-    return on_sig_stack(sp) ? SS_ONSTACK : 0;
+    if (on_sig_stack(sp))
+        return SS_ONSTACK;
+    if (current->sas_ss_flags == SS_DISABLE)
+        return SS_DISABLE;
+    return 0;
  }

  static inline unsigned long sigsp(unsigned long sp, struct ksignal *ksig)
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 92557bb..844b113 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -432,7 +432,7 @@ int __save_altstack(stack_t __user *, unsigned long);
      stack_t __user *__uss = uss; \
      struct task_struct *t = current; \
      put_user_ex((void __user *)t->sas_ss_sp, &__uss->ss_sp); \
-    put_user_ex(sas_ss_flags(sp), &__uss->ss_flags); \
+    put_user_ex(t->sas_ss_flags, &__uss->ss_flags); \
      put_user_ex(t->sas_ss_size, &__uss->ss_size); \
  } while (0);

diff --git a/kernel/fork.c b/kernel/fork.c
index 2e391c7..ce840a9 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1482,8 +1482,10 @@ static struct task_struct *copy_process(unsigned 
long clone_flags,
      /*
       * sigaltstack should be cleared when sharing the same VM
       */
-    if ((clone_flags & (CLONE_VM|CLONE_VFORK)) == CLONE_VM)
+    if ((clone_flags & (CLONE_VM|CLONE_VFORK)) == CLONE_VM) {
          p->sas_ss_sp = p->sas_ss_size = 0;
+        p->sas_ss_flags = SS_DISABLE;
+    }

      /*
       * Syscall tracing and stepping should be turned off in the
diff --git a/kernel/signal.c b/kernel/signal.c
index f3f1f7a..d19180e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3101,6 +3101,7 @@ do_sigaltstack (const stack_t __user *uss, stack_t 
__user *uoss, unsigned long s
          void __user *ss_sp;
          size_t ss_size;
          int ss_flags;
+        int onsigstack;

          error = -EFAULT;
          if (!access_ok(VERIFY_READ, uss, sizeof(*uss)))
@@ -3111,32 +3112,49 @@ do_sigaltstack (const stack_t __user *uss, 
stack_t __user *uoss, unsigned long s
          if (error)
              goto out;

-        error = -EPERM;
-        if (on_sig_stack(sp))
-            goto out;
-
          error = -EINVAL;
-        /*
-         * Note - this code used to test ss_flags incorrectly:
-         *        old code may have been written using ss_flags==0
-         *      to mean ss_flags==SS_ONSTACK (as this was the only
-         *      way that worked) - this fix preserves that older
-         *      mechanism.
-         */
          if (ss_flags != SS_DISABLE && ss_flags != SS_ONSTACK && 
ss_flags != 0)
              goto out;

-        if (ss_flags == SS_DISABLE) {
-            ss_size = 0;
-            ss_sp = NULL;
-        } else {
+        onsigstack = on_sig_stack(sp);
+        if (ss_size == 0) {
+            switch (ss_flags) {
+            case 0:
+                error = -EPERM;
+                if (onsigstack)
+                    goto out;
+                current->sas_ss_sp = 0;
+                current->sas_ss_size = 0;
+                current->sas_ss_flags = SS_DISABLE;
+                break;
+            case SS_ONSTACK:
+                /* re-enable previously disabled sas */
+                error = -EINVAL;
+                if (current->sas_ss_size == 0)
+                    goto out;
+                break;
+            default:
+                break;
+            }
+        } else if (ss_flags != SS_DISABLE) {
+            error = -EPERM;
+            if (onsigstack)
+                goto out;
              error = -ENOMEM;
              if (ss_size < MINSIGSTKSZ)
                  goto out;
+            current->sas_ss_sp = (unsigned long) ss_sp;
+            current->sas_ss_size = ss_size;
+            /* unfortunately POSIX forces us to treat 0
+             * as SS_ONSTACK here, and some legacy apps
+             * perhaps used that...
+             */
+            if (ss_flags == 0)
+                ss_flags = SS_ONSTACK;
          }

-        current->sas_ss_sp = (unsigned long) ss_sp;
-        current->sas_ss_size = ss_size;
+        if (ss_flags != 0)
+            current->sas_ss_flags = ss_flags;
      }

      error = 0;
@@ -3168,7 +3186,7 @@ int __save_altstack(stack_t __user *uss, unsigned 
long sp)
  {
      struct task_struct *t = current;
      return  __put_user((void __user *)t->sas_ss_sp, &uss->ss_sp) |
-        __put_user(sas_ss_flags(sp), &uss->ss_flags) |
+        __put_user(t->sas_ss_flags, &uss->ss_flags) |
          __put_user(t->sas_ss_size, &uss->ss_size);
  }

-- 
2.5.0

[PATCH 4/4] sigaltstack: allow disabling and re-enabling sas within sighandler

[Stas Sergeev]

linux implements the sigaltstack() in a way that makes it impossible to
use with swapcontext(). Per the man page, sigaltstack is allowed to return
EPERM if the process is altering its sigaltstack while running on
sigaltstack.
This is likely needed to consistently return oss->ss_flags, that indicates
whether the process is being on sigaltstack or not.
Unfortunately, linux takes that permission to return EPERM too literally:
it returns EPERM even if you don't want to change to another sigaltstack,
but only want to temporarily disable sigaltstack with SS_DISABLE.
You can't use swapcontext() without disabling sigaltstack first, or the
stack will be re-used and overwritten by a subsequent signal.

With this patch, disabling sigaltstack inside a signal handler became
possible, and the swapcontext() can then be used safely. After switching
back to the sighandler, the app can re-enable the sigatlstack.
The oss->ss_flags will correctly indicate the current use of sigaltstack,
even if it is temporarily disabled. Any attempt to modify the sigaltstack
(rather than to disable or re-enable it) within the sighandler, will still
be punished with EPERM as suggested by POSIX.

CC: Ingo Molnar <mingo@redhat.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: "Amanieu d'Antras" <amanieu@gmail.com>
CC: Richard Weinberger <richard@nod.at>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Tejun Heo <tj@kernel.org>
CC: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
CC: Jason Low <jason.low2@hp.com>
CC: Heinrich Schuchardt <xypron.glpk@gmx.de>
CC: Andrea Arcangeli <aarcange@redhat.com>
CC: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
CC: Josh Triplett <josh@joshtriplett.org>
CC: "Eric W. Biederman" <ebiederm@xmission.com>
CC: Aleksa Sarai <cyphar@cyphar.com>
CC: Paul Moore <pmoore@redhat.com>
CC: Palmer Dabbelt <palmer@dabbelt.com>
CC: Vladimir Davydov <vdavydov@parallels.com>
CC: linux-kernel@vger.kernel.org
CC: linux-api@vger.kernel.org

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
---
  include/linux/sched.h  |  8 ++++++--
  include/linux/signal.h |  2 +-
  kernel/fork.c          |  4 +++-
  kernel/signal.c        | 54 
+++++++++++++++++++++++++++++++++-----------------
  4 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index a10494a..38f67dc 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1587,6 +1587,7 @@ struct task_struct {

      unsigned long sas_ss_sp;
      size_t sas_ss_size;
+    int sas_ss_flags;

      struct callback_head *task_works;

@@ -2569,8 +2570,11 @@ static inline int sas_ss_flags(unsigned long sp)
  {
      if (!current->sas_ss_size)
          return SS_DISABLE;
-
-    return on_sig_stack(sp) ? SS_ONSTACK : 0;
+    if (on_sig_stack(sp))
+        return SS_ONSTACK;
+    if (current->sas_ss_flags == SS_DISABLE)
+        return SS_DISABLE;
+    return 0;
  }

  static inline unsigned long sigsp(unsigned long sp, struct ksignal *ksig)
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 92557bb..844b113 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -432,7 +432,7 @@ int __save_altstack(stack_t __user *, unsigned long);
      stack_t __user *__uss = uss; \
      struct task_struct *t = current; \
      put_user_ex((void __user *)t->sas_ss_sp, &__uss->ss_sp); \
-    put_user_ex(sas_ss_flags(sp), &__uss->ss_flags); \
+    put_user_ex(t->sas_ss_flags, &__uss->ss_flags); \
      put_user_ex(t->sas_ss_size, &__uss->ss_size); \
  } while (0);

diff --git a/kernel/fork.c b/kernel/fork.c
index 2e391c7..ce840a9 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1482,8 +1482,10 @@ static struct task_struct *copy_process(unsigned 
long clone_flags,
      /*
       * sigaltstack should be cleared when sharing the same VM
       */
-    if ((clone_flags & (CLONE_VM|CLONE_VFORK)) == CLONE_VM)
+    if ((clone_flags & (CLONE_VM|CLONE_VFORK)) == CLONE_VM) {
          p->sas_ss_sp = p->sas_ss_size = 0;
+        p->sas_ss_flags = SS_DISABLE;
+    }

      /*
       * Syscall tracing and stepping should be turned off in the
diff --git a/kernel/signal.c b/kernel/signal.c
index f3f1f7a..d19180e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3101,6 +3101,7 @@ do_sigaltstack (const stack_t __user *uss, stack_t 
__user *uoss, unsigned long s
          void __user *ss_sp;
          size_t ss_size;
          int ss_flags;
+        int onsigstack;

          error = -EFAULT;
          if (!access_ok(VERIFY_READ, uss, sizeof(*uss)))
@@ -3111,32 +3112,49 @@ do_sigaltstack (const stack_t __user *uss, 
stack_t __user *uoss, unsigned long s
          if (error)
              goto out;

-        error = -EPERM;
-        if (on_sig_stack(sp))
-            goto out;
-
          error = -EINVAL;
-        /*
-         * Note - this code used to test ss_flags incorrectly:
-         *        old code may have been written using ss_flags==0
-         *      to mean ss_flags==SS_ONSTACK (as this was the only
-         *      way that worked) - this fix preserves that older
-         *      mechanism.
-         */
          if (ss_flags != SS_DISABLE && ss_flags != SS_ONSTACK && 
ss_flags != 0)
              goto out;

-        if (ss_flags == SS_DISABLE) {
-            ss_size = 0;
-            ss_sp = NULL;
-        } else {
+        onsigstack = on_sig_stack(sp);
+        if (ss_size == 0) {
+            switch (ss_flags) {
+            case 0:
+                error = -EPERM;
+                if (onsigstack)
+                    goto out;
+                current->sas_ss_sp = 0;
+                current->sas_ss_size = 0;
+                current->sas_ss_flags = SS_DISABLE;
+                break;
+            case SS_ONSTACK:
+                /* re-enable previously disabled sas */
+                error = -EINVAL;
+                if (current->sas_ss_size == 0)
+                    goto out;
+                break;
+            default:
+                break;
+            }
+        } else if (ss_flags != SS_DISABLE) {
+            error = -EPERM;
+            if (onsigstack)
+                goto out;
              error = -ENOMEM;
              if (ss_size < MINSIGSTKSZ)
                  goto out;
+            current->sas_ss_sp = (unsigned long) ss_sp;
+            current->sas_ss_size = ss_size;
+            /* unfortunately POSIX forces us to treat 0
+             * as SS_ONSTACK here, and some legacy apps
+             * perhaps used that...
+             */
+            if (ss_flags == 0)
+                ss_flags = SS_ONSTACK;
          }

-        current->sas_ss_sp = (unsigned long) ss_sp;
-        current->sas_ss_size = ss_size;
+        if (ss_flags != 0)
+            current->sas_ss_flags = ss_flags;
      }

      error = 0;
@@ -3168,7 +3186,7 @@ int __save_altstack(stack_t __user *uss, unsigned 
long sp)
  {
      struct task_struct *t = current;
      return  __put_user((void __user *)t->sas_ss_sp, &uss->ss_sp) |
-        __put_user(sas_ss_flags(sp), &uss->ss_flags) |
+        __put_user(t->sas_ss_flags, &uss->ss_flags) |
          __put_user(t->sas_ss_size, &uss->ss_size);
  }

-- 
2.5.0