Messages in this thread Patch in this message | | | Subject | Re: [GIT PULL] core/urgent for v5.16-rc6 | From | Dave Hansen <> | Date | Sun, 19 Dec 2021 21:25:44 -0800 |
| |
On 12/19/21 12:14 PM, Linus Torvalds wrote: ... > The SS_DISABLE case shouldn't take the lock at all. > > And the actual modification of the values shouldn't need any locking > at all, since it's all thread-local.
The modification is definitely thread-local, but the implications are wider after the dynamic xfeature and sigframe support went in. Now, (x86-only) no thread is allowed to enable dynamic features unless the entire _process's_ altstacks pass validate_sigaltstack().
> I'm not convinced even the limit checking needs the lock, but > whatever. I think it could maybe just use "read_once()" or something. > > I think the attached patch is an improvement, but I did *not* test > this, and I'm just throwing this out as a "maybe something like this".
The patch definitely makes the code easier to read. But, it looks like we need to invert the sigaltstack_size_valid() condition from the patch:
> + if (unlikely(ss_size < min_ss_size) || > + unlikely(sigaltstack_size_valid(ss_size))) { ^^^^^ > + sigaltstack_unlock(); > + return -ENOMEM; > }
That should be !sigaltstack_size_valid(ss_size).
Also, the sigaltstack_lock() lock really is needed over the assignments like this:
> t->sas_ss_sp = (unsigned long) ss_sp; > t->sas_ss_size = ss_size; > t->sas_ss_flags = ss_flags; to prevent races with validate_sigaltstack(). We desperately need a comment in there, but we probably shouldn't reference validate_sigaltstack() itself since it's deeply x86-only. I've got a shot at a comment in the attached patch.
As for the the:
> if (ss_mode == SS_DISABLE) { > t->sas_ss_sp = 0; > t->sas_ss_size = 0; > t->sas_ss_flags = ss_flags; > return 0; > }
hunk, I think it's OK. Shrinking t->sas_ss_size without the lock is safe-ish because it will never cause validate_sigaltstack() to fail. I need to think about that bit more, though.
Another blatantly untested patch is attached. I'll try to give it a go on some real hardware tomorrow.
1 file changed, 19 insertions(+), 17 deletions(-)
index dfcee3888b00..f58f1d574931 100644
---
b/kernel/signal.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-)
diff -puN kernel/signal.c~linux-sigaltstack kernel/signal.c --- a/kernel/signal.c~linux-sigaltstack 2021-12-19 16:50:41.411762535 -0800 +++ b/kernel/signal.c 2021-12-19 21:14:14.605399136 -0800 @@ -4161,7 +4161,6 @@ do_sigaltstack (const stack_t *ss, stack size_t min_ss_size) { struct task_struct *t = current; - int ret = 0; if (oss) { memset(oss, 0, sizeof(stack_t)); @@ -4181,8 +4180,15 @@ do_sigaltstack (const stack_t *ss, stack return -EPERM; ss_mode = ss_flags & ~SS_FLAG_BITS; - if (unlikely(ss_mode != SS_DISABLE && ss_mode != SS_ONSTACK && - ss_mode != 0)) + + if (ss_mode == SS_DISABLE) { + t->sas_ss_sp = 0; + t->sas_ss_size = 0; + t->sas_ss_flags = ss_flags; + return 0; + } + + if (unlikely(ss_mode != SS_ONSTACK && ss_mode != 0)) return -EINVAL; /* @@ -4194,24 +4200,25 @@ do_sigaltstack (const stack_t *ss, stack t->sas_ss_flags == ss_flags) return 0; + /* + * Lock out any changes to sigaltstack_size_valid() + * until the t->sas_ss_* changes are complete: + */ sigaltstack_lock(); - if (ss_mode == SS_DISABLE) { - ss_size = 0; - ss_sp = NULL; - } else { - if (unlikely(ss_size < min_ss_size)) - ret = -ENOMEM; - if (!sigaltstack_size_valid(ss_size)) - ret = -ENOMEM; - } - if (!ret) { - t->sas_ss_sp = (unsigned long) ss_sp; - t->sas_ss_size = ss_size; - t->sas_ss_flags = ss_flags; + + if (unlikely(ss_size < min_ss_size) || + unlikely(!sigaltstack_size_valid(ss_size))) { + sigaltstack_unlock(); + return -ENOMEM; } + + t->sas_ss_sp = (unsigned long) ss_sp; + t->sas_ss_size = ss_size; + t->sas_ss_flags = ss_flags; + sigaltstack_unlock(); } - return ret; + return 0; } SYSCALL_DEFINE2(sigaltstack,const stack_t __user *,uss, stack_t __user *,uoss) _ | |