Re: [PATCH 2/2] sigaltstack: remove EPERM check to make swapcontext() usable
flat view
From: Stas Sergeev <hidden>
Date: 2016-01-31 02:54:11
Also in:
lkml
Subsystem:
exec & binfmt api, elf, memory management - core, scheduler, the rest, x86 architecture (32-bit and 64-bit) · Maintainers:
Kees Cook, Andrew Morton, David Hildenbrand, Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot, Linus Torvalds, Thomas Gleixner, Borislav Petkov, Dave Hansen
[re-sending with better formatting] 09.01.2016 05:49, Stas Sergeev пишет:
09.01.2016 05:03, Andy Lutomirski пишет:quoted
On Fri, Jan 8, 2016 at 5:18 PM, Stas Sergeev [off-list ref] wrote:quoted
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); }