Re: [GIT PULL] core/urgent for v5.16-rc6

[Dave Hansen <dave.hansen@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]

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.

[-- Attachment #2: patch2.diff --]
[-- Type: text/x-patch, Size: 2089 bytes --]


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