From: Oleg Nesterov <oleg@redhat.com> Date: Sun, 30 May 2010 15:33:53 -0400 Subject: [misc] signals: avoid unnecessary credentials check Message-id: <20100530153353.GA7513@redhat.com> Patchwork-id: 25902 O-Subject: [RHEL5 PATCH] bz#459901: signals: check_kill_permission(): don't check creds if same_thread_group() Bugzilla: 459901 RH-Acked-by: Roland McGrath <roland@redhat.com> RH-Acked-by: Jiri Olsa <jolsa@redhat.com> RH-Acked-by: Ivan Vecera <ivecera@redhat.com> see https://bugzilla.redhat.com/show_bug.cgi?id=459901 Upstream: trivial backport of 065add3941bdca54fe04ed3471a96bce9af88793 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 credentials. But there is not any security reason to check ->xuid'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, fix the indentation. Reported-by: Andrew Tridgell <tridge@samba.org> Upstream-acked-by: Roland McGrath <roland@redhat.com> Upstream-acked-by: David Howells <dhowells@redhat.com> Signed-off-by: Oleg Nesterov <oleg@redhat.com> diff --git a/kernel/signal.c b/kernel/signal.c index df009a9..b0261a5 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -487,12 +487,13 @@ static int check_kill_permission(int sig, struct siginfo *info, if (error) return error; error = -EPERM; - if (((sig != SIGCONT) || + if ((current->tgid != t->tgid) + && ((sig != SIGCONT) || (current->signal->session != t->signal->session)) && (current->euid ^ t->suid) && (current->euid ^ t->uid) && (current->uid ^ t->suid) && (current->uid ^ t->uid) && !capable(CAP_KILL)) - return error; + return error; } return security_task_kill(t, info, sig, 0);