| From 065add3941bdca54fe04ed3471a96bce9af88793 Mon Sep 17 00:00:00 2001 |
| From: Oleg Nesterov <oleg@redhat.com> |
| Date: Wed, 26 May 2010 14:42:54 -0700 |
| Subject: signals: check_kill_permission(): don't check creds if same_thread_group() |
| |
| From: Oleg Nesterov <oleg@redhat.com> |
| |
| commit 065add3941bdca54fe04ed3471a96bce9af88793 upstream. |
| |
| Andrew Tridgell reports that aio_read(SIGEV_SIGNAL) can fail if the |
| notification from the helper thread races with setresuid(), see |
| http://samba.org/~tridge/junkcode/aio_uid.c |
| |
| This happens because check_kill_permission() doesn't permit sending a |
| signal to the task with the different cred->xids. But there is not any |
| security reason to check ->cred's when the task sends a signal (private or |
| group-wide) to its sub-thread. Whatever we do, any thread can bypass all |
| security checks and send SIGKILL to all threads, or it can block a signal |
| SIG and do kill(gettid(), SIG) to deliver this signal to another |
| sub-thread. Not to mention that CLONE_THREAD implies CLONE_VM. |
| |
| Change check_kill_permission() to avoid the credentials check when the |
| sender and the target are from the same thread group. |
| |
| Also, move "cred = current_cred()" down to avoid calling get_current() |
| twice. |
| |
| Note: David Howells pointed out we could relax this even more, the |
| CLONE_SIGHAND (without CLONE_THREAD) case probably does not need |
| these checks too. |
| |
| Roland said: |
| : The glibc (libpthread) that does set*id across threads has |
| : been in use for a while (2.3.4?), probably in distro's using kernels as old |
| : or older than any active -stable streams. In the race in question, this |
| : kernel bug is breaking valid POSIX application expectations. |
| |
| Reported-by: Andrew Tridgell <tridge@samba.org> |
| Signed-off-by: Oleg Nesterov <oleg@redhat.com> |
| Acked-by: Roland McGrath <roland@redhat.com> |
| Acked-by: David Howells <dhowells@redhat.com> |
| Cc: Eric Paris <eparis@parisplace.org> |
| Cc: Jakub Jelinek <jakub@redhat.com> |
| Cc: James Morris <jmorris@namei.org> |
| Cc: Roland McGrath <roland@redhat.com> |
| Cc: Stephen Smalley <sds@tycho.nsa.gov> |
| Signed-off-by: Andrew Morton <akpm@linux-foundation.org> |
| Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> |
| |
| --- |
| kernel/signal.c | 6 ++++-- |
| 1 file changed, 4 insertions(+), 2 deletions(-) |
| |
| --- a/kernel/signal.c |
| +++ b/kernel/signal.c |
| @@ -591,7 +591,7 @@ static int rm_from_queue(unsigned long m |
| static int check_kill_permission(int sig, struct siginfo *info, |
| struct task_struct *t) |
| { |
| - const struct cred *cred = current_cred(), *tcred; |
| + const struct cred *cred, *tcred; |
| struct pid *sid; |
| int error; |
| |
| @@ -605,8 +605,10 @@ static int check_kill_permission(int sig |
| if (error) |
| return error; |
| |
| + cred = current_cred(); |
| tcred = __task_cred(t); |
| - if ((cred->euid ^ tcred->suid) && |
| + if (!same_thread_group(current, t) && |
| + (cred->euid ^ tcred->suid) && |
| (cred->euid ^ tcred->uid) && |
| (cred->uid ^ tcred->suid) && |
| (cred->uid ^ tcred->uid) && |