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 <[email protected]>
---
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
On Wed, May 9, 2012 at 3:09 PM, Jan Kiszka <[email protected]> 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
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 <[email protected]>
---
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
On Thu, May 10, 2012 at 6:04 AM, Jan Kiszka <[email protected]> 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
On Thu, May 10, 2012 at 8:44 AM, Linus Torvalds
<[email protected]> 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
On 2012-05-10 12:58, Linus Torvalds wrote:
> On Thu, May 10, 2012 at 8:44 AM, Linus Torvalds
> <[email protected]> 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
On 10.05.2012 19:58, Linus Torvalds wrote:
> On Thu, May 10, 2012 at 8:44 AM, Linus Torvalds
> <[email protected]> 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 <[email protected]>
or, if you prefer,
Signed-off-By: Michael Tokarev <[email protected]>
for the combined patch.
Thank you for the excellent catch Jan!
/mjt