[PATCH 0/2] sigaltstack: remove EPERM check

[PATCH 0/2] sigaltstack: remove EPERM check

[Stas Sergeev]

The following patches add the self-test for
sigaltstack(SS_DISABLE) inside the signal handler,
and remove the EPERM check in a sigaltstack()
syscall.
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.

[PATCH 1/2] 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.

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    | 117 
+++++++++++++++++++++++++++
  3 files changed, 126 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 c8edff6..d5b2005 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -17,6 +17,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..1071718
--- /dev/null
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -0,0 +1,117 @@
+/*
+ * 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, NULL);
+    if (err) {
+        perror("[FAIL]\tsigaltstack()");
+        /* 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);
+    }
+}
+
+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.4.3

[PATCH 2/2] sigaltstack: remove EPERM check to make swapcontext() usable

[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 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. The oss->ss_flags
will then return SS_DISABLE, which doesn't seem to contradict the
(very ambiguous) man page wording, namely:
        SS_ONSTACK
               The process is currently executing on the alternate signal
               stack. (Note that it is not possible to change the alternate
               signal stack if the process is currently executing on it.)

        SS_DISABLE
               The alternate signal stack is currently disabled.

It seems both the above cases apply when executing on sigaltstack with
sigaltstack being currently disabled, so hope no one really cares.

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: 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>
---
  kernel/signal.c | 15 +++++----------
  1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index f3f1f7a..0a6af54 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3111,18 +3111,13 @@ 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.
+         * Note - this code used to test on_sig_stack(sp) and
+         * return -EPERM. But we need at least SS_DISABLE to
+         * work while on sigaltstack, so the check was removed.
           */
+
+        error = -EINVAL;
          if (ss_flags != SS_DISABLE && ss_flags != SS_ONSTACK && 
ss_flags != 0)
              goto out;

-- 
2.4.3

Re: [PATCH 2/2] sigaltstack: remove EPERM check to make swapcontext() usable

[Andy Lutomirski]

On Fri, Jan 8, 2016 at 5:18 PM, Stas Sergeev <stsp@list.ru> wrote:
>
> 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 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. The oss->ss_flags
> will then return SS_DISABLE, which doesn't seem to contradict the
> (very ambiguous) man page wording, namely:
>        SS_ONSTACK
>               The process is currently executing on the alternate signal
>               stack. (Note that it is not possible to change the alternate
>               signal stack if the process is currently executing on it.)

You're definitely contradicting the "Note" part, though.  POSIX is
quite clear, too:

"Attempts to modify the alternate signal stack while the process is
executing on it fail."

> diff --git a/kernel/signal.c b/kernel/signal.c
> index f3f1f7a..0a6af54 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3111,18 +3111,13 @@ 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.
> +         * Note - this code used to test on_sig_stack(sp) and
> +         * return -EPERM. But we need at least SS_DISABLE to
> +         * work while on sigaltstack, so the check was removed.


That old comment was simply incorrect.  POSIX says:

       The ss_flags member specifies the new stack state.
       If it is set to SS_DISABLE, the stack is disabled and ss_sp and ss_size
       are  ignored.  Otherwise, the stack shall be enabled, and the ss_sp and
       ss_size members specify the new address and size of the stack.

Zero is perfectly valid.  That being said, Linux has apparently
rejected non-zero non-SA_ONSTACK values for a long time, so we should
be fine.

I think it would be safer and more posixly correct to change the
behavior differently:

ss_flags == 0 or SS_DISABLE or SS_ONSTACK: preserve old Linux behavior.

ss_flags == SS_DISABLE | SS_FORCE: disable the altstack regardless of
whether we're executing on it.

ss_flags == SS_ONSTACK | SS_FORCE: change the altstack regardless of
whether we're executing on it.

ss_flags == anything else (including SS_FORCE by itself): -EINVAL

This has some benefits.  Mainly, with this change, we're more
POSIX-violating than we were -- any users who didn't get EINVAL before
aren't unaffected.

Users of SS_FORCE are required to be very careful with nested signals.
In particular, changing the altstack using SS_ONSTACK | SS_FORCE is
very dangerous: if a user does that, then gets a signal, and the
signal handler uses SS_ONSTACK | SS_FORCE to change back to the old
altstack, and then another signal is delivered, a crash will result.
The manpage patch should mention that caveat.

--Andy

Re: [PATCH 2/2] sigaltstack: remove EPERM check to make swapcontext() usable

[Stas Sergeev]

09.01.2016 05:03, Andy Lutomirski пишет:
> On Fri, Jan 8, 2016 at 5:18 PM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
>> 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 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. The oss->ss_flags
>> will then return SS_DISABLE, which doesn't seem to contradict the
>> (very ambiguous) man page wording, namely:
>>         SS_ONSTACK
>>                The process is currently executing on the alternate signal
>>                stack. (Note that it is not possible to change the alternate
>>                signal stack if the process is currently executing on it.)
> You're definitely contradicting the "Note" part, though.  POSIX is
> quite clear, too:
>
> "Attempts to modify the alternate signal stack while the process is
> executing on it fail."
"modify" may not include "disable".
You don't modify the stack's location or size, just temporary disable 
its use.
So I believe SS_DISABLE should be fine.
Of course this is just one of the possible interpretations.
But it is the one that looks simple and satisfies everyone.

>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index f3f1f7a..0a6af54 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -3111,18 +3111,13 @@ 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.
>> +         * Note - this code used to test on_sig_stack(sp) and
>> +         * return -EPERM. But we need at least SS_DISABLE to
>> +         * work while on sigaltstack, so the check was removed.
>
> That old comment was simply incorrect.  POSIX says:
>
>         The ss_flags member specifies the new stack state.
>         If it is set to SS_DISABLE, the stack is disabled and ss_sp and ss_size
>         are  ignored.  Otherwise, the stack shall be enabled, and the ss_sp and
>         ss_size members specify the new address and size of the stack.
>
> Zero is perfectly valid.  That being said, Linux has apparently
> rejected non-zero non-SA_ONSTACK values for a long time, so we should
> be fine.
>
> I think it would be safer and more posixly correct to change the
> behavior differently:
>
> ss_flags == 0 or SS_DISABLE or SS_ONSTACK: preserve old Linux behavior.
>
> ss_flags == SS_DISABLE | SS_FORCE: disable the altstack regardless of
> whether we're executing on it.
I really dislike the idea of adding SS_FORCE just for SS_DISABLE.
I think it is sane to use SS_DISABLE in a sighandler, and it doesn't
conflict with one of the possible interpretations of posix.
So I wonder what other people think (add Linus to CC).

> ss_flags == SS_ONSTACK | SS_FORCE: change the altstack regardless of
> whether we're executing on it.
This is something that no one will likely ever need.
For SS_DISABLE there is a clear use-case: swapcontext().
So we _need_ to address the SS_DISABLE case.
But why do we care about SA_ONSTACK in a sighandler's case?
Someone later, if need be, can add it together with the SA_FORCE.

> Users of SS_FORCE are required to be very careful with nested signals.
> In particular, changing the altstack using SS_ONSTACK | SS_FORCE is
> very dangerous:
So how about just not allowing such SS_ONSTACK until
someone really needs it... This is more difficult to implement,
but possible. Would you really add a new flag for the funtionality
that is both dangerous and useless?

Re: [PATCH 2/2] sigaltstack: remove EPERM check to make swapcontext() usable

[Stas Sergeev]

09.01.2016 05:49, Stas Sergeev пишет:
> 09.01.2016 05:03, Andy Lutomirski пишет:  >> On Fri, Jan 8, 2016 at 5:18 PM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote: 
 >>> 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 
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. The oss->ss_flags >>> will 
then return SS_DISABLE, which doesn't seem to contradict the >>> (very 
ambiguous) man page wording, namely: >>>         SS_ONSTACK 
 >>>                The process is currently executing on the alternate 
signal >>>                stack. (Note that it is not possible to change 
the alternate >>>                signal stack if the process is 
currently executing on it.) >> You're definitely contradicting the 
"Note" part, though.  POSIX is >> quite clear, too: >> >> "Attempts to 
modify the alternate signal stack while the process is >> executing on 
it fail." > "modify" may not include "disable". > You don't modify the 
stack's location or size, just temporary disable its use. > So I believe 
SS_DISABLE should be fine. > Of course this is just one of the possible 
interpretations. > But it is the one that looks simple and satisfies 
everyone.
I've finally made the patch to demonstrate that.
It has the nasty disadvantage of touching the arch-specific
code, but it has the advantages too:
- seems compatible with both posix and swapcontext()
   (before using swapcontext() in a sighandler, the sigaltstack
   have to be disabled - this is what this patch makes possible)
- doesn't require the new flag to paper around one of the
   possible posix interpretation
- consistently returns oss->ss_flags==SS_ONSTACK while in
   sighandler, even after setting flags to SS_DISABLE
- allows someone (like glibc), if need be, to implement the old
   behaviour: it can check for oss->ss_flags first, and if it is
   SS_ONSTACK, return EPERM without trying to set the new value.

So since this patch demonstrates the way of solving the
swapcontext() problem without adding the SS_FORCE flag
to sigaltstack(), I believe we shouldn't be adding it. That flag
will only allow us to have a smaller patch that doesn't touch
the arch code, but is that a good enough justification?

So if there are no objections, I'll probably have to add an
arch-specific ifdefs to this patch to make other arches
unaffected, and submit it.
Of course we can still just remove the EPERM check.
Thoughts?


diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index cb6282c..06e2591 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_enabled())
                  sp = current->sas_ss_sp + current->sas_ss_size;
          } else if (config_enabled(CONFIG_X86_32) &&
                 (regs->ss & 0xffff) != __USER_DS &&
diff --git a/include/linux/sched.h b/include/linux/sched.h
index edad7a4..f7e7026 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1572,6 +1572,7 @@ struct task_struct {

      unsigned long sas_ss_sp;
      size_t sas_ss_size;
+    int sas_ss_flags;

      struct callback_head *task_works;

@@ -2550,8 +2551,16 @@ static inline int sas_ss_flags(unsigned long sp)
  {
      if (!current->sas_ss_size)
          return SS_DISABLE;
+    if (on_sig_stack(sp))
+        return SS_ONSTACK;
+    if (current->sas_ss_flags == SS_DISABLE)
+        return SS_DISABLE;
+    return 0;
+}

-    return on_sig_stack(sp) ? SS_ONSTACK : 0;
+static inline int sas_ss_enabled(void)
+{
+    return (current->sas_ss_size && current->sas_ss_flags != SS_DISABLE);
  }

  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 fce002e..e5edc5c 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1483,8 +1483,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..ef537d8 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,48 @@ 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 +3185,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);
  }

Re: [PATCH 2/2] sigaltstack: remove EPERM check to make swapcontext() usable

[Stas Sergeev]

[re-sending with better formatting]

09.01.2016 05:49, Stas Sergeev пишет:
> 09.01.2016 05:03, Andy Lutomirski пишет:
>> On Fri, Jan 8, 2016 at 5:18 PM, Stas Sergeev <stsp-cmBhpYW9OiY@public.gmane.org> wrote:
>>> 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 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. The
 >>> oss->ss_flags
 >>> will then return SS_DISABLE, which doesn't seem to contradict the
 >>> (very ambiguous) man page wording, namely:
 >>>         SS_ONSTACK
 >>>                The process is currently executing on the alternate
 >>> signal
 >>>                stack. (Note that it is not possible to change the
 >>> alternate
 >>>                signal stack if the process is currently executing
 >>> on it.)
 >>>
 >> You're definitely contradicting the "Note" part, though.  POSIX is
 >> quite clear, too:
 >> "Attempts to modify the alternate signal stack while the process is
 >> executing on it fail."
 > "modify" may not include "disable".
 > You don't modify the stack's location or size, just temporary disable
 > its use.
 > So I believe SS_DISABLE should be fine.
 > Of course this is just one of the possible interpretations.
 > But it is the one that looks simple and satisfies everyone.

I've finally made the patch to demonstrate that.
It has the nasty disadvantage of touching the arch-specific
code, but it has the advantages too:
- seems compatible with both posix and swapcontext()
   (before using swapcontext() in a sighandler, the sigaltstack
   have to be disabled - this is what this patch makes possible)
- doesn't require the new flag to paper around one of the
   possible posix interpretation
- consistently returns oss->ss_flags==SS_ONSTACK while in
   sighandler, even after setting flags to SS_DISABLE
- allows someone (like glibc), if need be, to implement the old
   behaviour: it can check for oss->ss_flags first, and if it is
   SS_ONSTACK, return EPERM without trying to set the new value.

So since this patch demonstrates the way of solving the
swapcontext() problem without adding the SS_FORCE flag
to sigaltstack(), I believe we shouldn't be adding it. That flag
will only allow us to have a smaller patch that doesn't touch
the arch code, but is that a good enough justification?

So if there are no objections, I'll probably have to add an
arch-specific ifdefs to this patch to make other arches
unaffected, and submit it.
Of course we can still just remove the EPERM check.
Thoughts?


diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index cb6282c..06e2591 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_enabled())
                  sp = current->sas_ss_sp + current->sas_ss_size;
          } else if (config_enabled(CONFIG_X86_32) &&
                 (regs->ss & 0xffff) != __USER_DS &&
diff --git a/include/linux/sched.h b/include/linux/sched.h
index edad7a4..f7e7026 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1572,6 +1572,7 @@ struct task_struct {

      unsigned long sas_ss_sp;
      size_t sas_ss_size;
+    int sas_ss_flags;

      struct callback_head *task_works;

@@ -2550,8 +2551,16 @@ static inline int sas_ss_flags(unsigned long sp)
  {
      if (!current->sas_ss_size)
          return SS_DISABLE;
+    if (on_sig_stack(sp))
+        return SS_ONSTACK;
+    if (current->sas_ss_flags == SS_DISABLE)
+        return SS_DISABLE;
+    return 0;
+}

-    return on_sig_stack(sp) ? SS_ONSTACK : 0;
+static inline int sas_ss_enabled(void)
+{
+    return (current->sas_ss_size && current->sas_ss_flags != SS_DISABLE);
  }

  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 fce002e..e5edc5c 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1483,8 +1483,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..ef537d8 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,48 @@ 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 +3185,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);
  }