[PATCH] compat: Fix RT signal mask corruption via sigprocmask

[PATCH] compat: Fix RT signal mask corruption via sigprocmask

[Jan Kiszka]

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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kernel/compat.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/compat.c b/kernel/compat.c
index 74ff849..03e491d 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -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

[PATCH] compat: Fix RT signal mask corruption via sigprocmask

[Jan Kiszka]

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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kernel/compat.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/compat.c b/kernel/compat.c
index 74ff849..03e491d 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -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

Re: [PATCH] compat: Fix RT signal mask corruption via sigprocmask

[Linus Torvalds]

On Wed, May 9, 2012 at 3:09 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 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.

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

[PATCH v2] compat: Fix RT signal mask corruption via sigprocmask

[Jan Kiszka]

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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kernel/compat.c |   56 ++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/kernel/compat.c b/kernel/compat.c
index 74ff849..39c164e 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -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

[PATCH v2] compat: Fix RT signal mask corruption via sigprocmask

[Jan Kiszka]

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.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kernel/compat.c |   56 ++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/kernel/compat.c b/kernel/compat.c
index 74ff849..39c164e 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -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

Re: [PATCH v2] compat: Fix RT signal mask corruption via sigprocmask

[Linus Torvalds]

On Thu, May 10, 2012 at 6:04 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> +               case SIG_BLOCK:
> +                       sigaddsetmask(&new_blocked, new_set);
> +                       break;
> +               case SIG_UNBLOCK:
> +                       sigdelsetmask(&new_blocked, new_set);
> +                       break;

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

> +               case SIG_SETMASK:
> +                       new_blocked.sig[0] &=
> +                               ~((old_sigset_t)(compat_old_sigset_t)-1);
> +                       new_blocked.sig[0] |= new_set;
> +                       break;

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

Re: [PATCH v2] compat: Fix RT signal mask corruption via sigprocmask

[Linus Torvalds]

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

On Thu, May 10, 2012 at 8:44 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I think I'll just edit your patch directly, no need to send me a new version.

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

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 1097 bytes --]

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

diff --git a/kernel/compat.c b/kernel/compat.c
index 39c164e02d05..d2c67aa49ae6 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -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;

Re: [PATCH v2] compat: Fix RT signal mask corruption via sigprocmask

[Jan Kiszka]

On 2012-05-10 12:58, Linus Torvalds wrote:
> On Thu, May 10, 2012 at 8:44 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> I think I'll just edit your patch directly, no need to send me a new version.
> 
> 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.

I did, and it looks and works fine.

Thanks,
Jan

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

Re: [PATCH v2] compat: Fix RT signal mask corruption via sigprocmask

[Michael Tokarev]

On 10.05.2012 19:58, Linus Torvalds wrote:
> On Thu, May 10, 2012 at 8:44 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> I think I'll just edit your patch directly, no need to send me a new version.
> 
> 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.

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

Tested-By: Michael Tokarev <mjt@tls.msk.ru>

or, if you prefer,

Signed-off-By: Michael Tokarev <mjt@tls.msk.ru>

for the combined patch.

Thank you for the excellent catch Jan!

/mjt