sigaltstack: remove EPERM check

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.

sigaltstack needs to be disabled before the signal handler can safely use swapcontext(). Unfortunately linux implementation of sigaltstack() returns EPERM in that case.

---
 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

index c8edff6..d5b2005 100644
@@ -17,6 +17,7 @@ TARGETS += powerpc
 TARGETS += pstore
 TARGETS += ptrace
 TARGETS += seccomp
+TARGETS += sigaltstack
 TARGETS += size
 TARGETS += static_keys
 TARGETS += sysctl

new file mode 100644
index 0000000..56af56e
@@ -0,0 +1,8 @@
+CFLAGS = -Wall
+BINARIES = sas
+all: $(BINARIES)
+
+include ../lib.mk
+
+clean:
+    rm -rf $(BINARIES)

2.4.3

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.

---
 kernel/signal.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

index f3f1f7a..0a6af54 100644
@@ -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

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."

09.01.2016 05:03, Andy Lutomirski пишет:

"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 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).

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.

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?

09.01.2016 05:49, Stas Sergeev пишет:

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?

index cb6282c..06e2591 100644
@@ -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 &&

index edad7a4..f7e7026 100644
@@ -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)

index 92557bb..844b113 100644
@@ -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);
 

index fce002e..e5edc5c 100644
@@ -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

index f3f1f7a..ef537d8 100644
@@ -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-sending with better formatting]

09.01.2016 05:49, Stas Sergeev пишет:

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?

index cb6282c..06e2591 100644
@@ -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 &&

index edad7a4..f7e7026 100644
@@ -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)

index 92557bb..844b113 100644
@@ -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);
 

index fce002e..e5edc5c 100644
@@ -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

index f3f1f7a..ef537d8 100644
@@ -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);

}