compat: Fix RT signal mask corruption via sigprocmask

compat_sys_sigprocmask reads a smaller signal mask from userspace than sigprogmask accepts for setting. So the high word of blocked.sig[0] will be cleared, releasing any potentially blocked RT signal.

This was discovered via userspace code that relies on get/setcontext. glibc's i386 versions of those functions use sigprogmask instead of rt_sigprogmask to save/restore the signal mask and caused RT signal unblocking this way.

---
 kernel/compat.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

index 74ff849..03e491d 100644
@@ -381,6 +381,8 @@ asmlinkage long compat_sys_sigprocmask(int how, compat_old_sigset_t __user *set,
 
 	if (set && get_user(s, set))
 		return -EFAULT;
+	s |= current->blocked.sig[0] &
+		~((old_sigset_t)(compat_old_sigset_t)-1);
 	old_fs = get_fs();
 	set_fs(KERNEL_DS);
 	ret = sys_sigprocmask(how,

1.7.3.4

I see what you're trying to do, but I think your patch is wrong.

It may work for SIG_BLOCK and SIG_SETMASK to use the current blocked bits in the high bits, but I'm pretty sure it doesn't work at all for SIG_UNBLOCK.

I suspect that compat_sys_sigprocmask() should simply be rewritten to *not* use sys_sigprocmask() at all, but simply open-code the three cases directly. IOW, copy the code from sys_sigprocmask(), and modify it for an incoming/outgoing compat_old_sigset_t.

Hmm?

Linus

compat_sys_sigprocmask reads a smaller signal mask from userspace than sigprogmask accepts for setting. So the high word of blocked.sig[0] will be cleared, releasing any potentially blocked RT signal.

This was discovered via userspace code that relies on get/setcontext. glibc's i386 versions of those functions use sigprogmask instead of rt_sigprogmask to save/restore signal mask and caused RT signal unblocking this way.

As suggested by Linus, this replaces the sys_sigprocmask based compat version with one that open-codes the required logic, including the merge of the existing blocked set with the new one provided on SIG_SETMASK.

---
 kernel/compat.c |   56 ++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 39 insertions(+), 17 deletions(-)

index 74ff849..39c164e 100644
@@ -372,25 +372,47 @@ asmlinkage long compat_sys_sigpending(compat_old_sigset_t __user *set)
 
 #ifdef __ARCH_WANT_SYS_SIGPROCMASK
 
-asmlinkage long compat_sys_sigprocmask(int how, compat_old_sigset_t __user *set,
-		compat_old_sigset_t __user *oset)
+asmlinkage long compat_sys_sigprocmask(int how,
+				       compat_old_sigset_t __user *nset,
+				       compat_old_sigset_t __user *oset)
 {
-	old_sigset_t s;
-	long ret;
-	mm_segment_t old_fs;
+	old_sigset_t old_set, new_set;
+	sigset_t new_blocked;
 
-	if (set && get_user(s, set))
-		return -EFAULT;
-	old_fs = get_fs();
-	set_fs(KERNEL_DS);
-	ret = sys_sigprocmask(how,
-			      set ? (old_sigset_t __user *) &s : NULL,
-			      oset ? (old_sigset_t __user *) &s : NULL);
-	set_fs(old_fs);
-	if (ret == 0)
-		if (oset)
-			ret = put_user(s, oset);
-	return ret;
+	old_set = current->blocked.sig[0];
+
+	if (nset) {
+		if (get_user(new_set, nset))
+			return -EFAULT;
+		new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP));
+
+		new_blocked = current->blocked;
+
+		switch (how) {
+		case SIG_BLOCK:
+			sigaddsetmask(&new_blocked, new_set);
+			break;
+		case SIG_UNBLOCK:
+			sigdelsetmask(&new_blocked, new_set);
+			break;
+		case SIG_SETMASK:
+			new_blocked.sig[0] &=
+				~((old_sigset_t)(compat_old_sigset_t)-1);
+			new_blocked.sig[0] |= new_set;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		set_current_blocked(&new_blocked);
+	}
+
+	if (oset) {
+		if (put_user(old_set, oset))
+			return -EFAULT;
+	}
+
+	return 0;
 }
 
 #endif

1.7.3.4

Ok, I think SIG_[UN]BLOCK are now clearly right. However:

I don't think this is clear.

The semantics for the *native* SIG_SETMASK has been to only change the lower word of the sigset_t.

And that was actually defined in terms of "compat_sigset_word", not "compat_old_sigset_t".

Now, they are both generally the same, and so I think your code does the right thing, but I have to say that I really had to look closely to make sure that yes, your code was right.

Anyway, my *gut* feel is that it would be much clearer to write the above as

    compat_sigset_word x = new_set;
    memcpy(new_blocked.sig, &x, sizeof(x));

together with a comment to the effect that sigprocmask(SIG_SETMASK..) only changes the first word of the structure.

That said, I think your patch does look technically correct, so maybe it's just me who thinks it is very non-obvious and hard to read.

The memcpy approach will also generate better code. This is the "mask-and-set":

        movabsq $-4294967296, %rax      #, tmp89
        andq    -32(%rbp), %rax # new_blocked.sig, tmp89
        orq     %rdx, %rax      # new_set, tmp89
        movq    %rax, -32(%rbp) # tmp89, new_blocked.sig

and this is the memcpy:

        movl    %edx, -32(%rbp) # new_set,

ie it is done as a simple 32-bit store.

I think I'll just edit your patch directly, no need to send me a new version.

Linus

Ok, just FYI, this is the incremental diff I ended up with. I'll commit it as this, but I'm sending it out for people to verify before I do. Although just from looking at the code generation, it really does just change those four instructions that do a mask to a single 'movl', so I think it's pretty safe even though I haven't tested it.

Linus

 kernel/compat.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

index 39c164e02d05..d2c67aa49ae6 100644
@@ -372,6 +372,15 @@ asmlinkage long compat_sys_sigpending(compat_old_sigset_t __user *set)
 
 #ifdef __ARCH_WANT_SYS_SIGPROCMASK
 
+/*
+ * sys_sigprocmask SIG_SETMASK sets the first (compat) word of the
+ * blocked set of signals to the supplied signal set
+ */
+static inline void compat_sig_setmask(sigset_t *blocked, compat_sigset_word set)
+{
+	memcpy(blocked->sig, &set, sizeof(set));
+}
+
 asmlinkage long compat_sys_sigprocmask(int how,
 				       compat_old_sigset_t __user *nset,
 				       compat_old_sigset_t __user *oset)

@@ -396,9 +405,7 @@ asmlinkage long compat_sys_sigprocmask(int how,
 			sigdelsetmask(&new_blocked, new_set);
 			break;
 		case SIG_SETMASK:
-			new_blocked.sig[0] &=
-				~((old_sigset_t)(compat_old_sigset_t)-1);
-			new_blocked.sig[0] |= new_set;
+			compat_sig_setmask(&new_blocked, new_set);
 			break;
 		default:
 			return -EINVAL;

I did, and it looks and works fine.

Thanks,
Jan

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

I tested the changes (from Jan and Linus's patches) and the result a) looks sane, and b) actually works and fixes our long-standing issues.

So you can add my

or, if you prefer,

for the combined patch.

Thank you for the excellent catch Jan!

/mjt