All of lore.kernel.org
 help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>,
	"Eric W. Biederman" <ebiederm@xmission.com>
Cc: security@kernel.org, "Robert O'Callahan" <robert@ocallahan.org>,
	Kees Cook <keescook@chromium.org>,
	Andy Lutomirski <luto@kernel.org>,
	Dmitry Vyukov <dvyukov@google.com>, Kyle Huey <me@kylehuey.com>,
	linux-kernel@vger.kernel.org
Subject: [PATCH 3/3] remove the no longer needed SIGNAL_UNKILLABLE check in complete_signal()
Date: Fri, 3 Nov 2017 19:42:46 +0100	[thread overview]
Message-ID: <20171103184246.GD21036@redhat.com> (raw)
In-Reply-To: <20171103184144.GA21036@redhat.com>

complete_signal() checks SIGNAL_UNKILLABLE before it starts to destroy the
thread group, today this is wrong in many ways.

If nothing else, fatal_signal_pending() should always imply that the whole
thread group (except ->group_exit_task if it is not NULL) is killed, this
check breaks the rule.

After the previous changes we can rely on sig_task_ignored(); sig_fatal(sig)
&& SIGNAL_UNKILLABLE can only be true if we actually want to kill this task
and sig == SIGKILL OR it is traced and debugger can intercept the signal.

This should hopefully fix the problem reported by Dmitry. This test-case

	static int init(void *arg)
	{
		for (;;)
			pause();
	}

	int main(void)
	{
		char stack[16 * 1024];

		for (;;) {
			int pid = clone(init, stack + sizeof(stack)/2,
					CLONE_NEWPID | SIGCHLD, NULL);
			assert(pid > 0);

			assert(ptrace(PTRACE_ATTACH, pid, 0, 0) == 0);
			assert(waitpid(-1, NULL, WSTOPPED) == pid);

			assert(ptrace(PTRACE_DETACH, pid, 0, SIGSTOP) == 0);
			assert(syscall(__NR_tkill, pid, SIGKILL) == 0);
			assert(pid == wait(NULL));
		}
	}

triggers the WARN_ON_ONCE(!(task->jobctl & JOBCTL_STOP_PENDING)) in
task_participate_group_stop(). do_signal_stop()->signal_group_exit()
checks SIGNAL_GROUP_EXIT and return false, but task_set_jobctl_pending()
checks fatal_signal_pending() and does not set JOBCTL_STOP_PENDING.

And his should fix the minor security problem reported by Kyle,
SECCOMP_RET_TRACE can miss fatal_signal_pending() the same way if
the task is the root of a pid namespace.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Reported-by: Kyle Huey <me@kylehuey.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/signal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 8fc0182..7e15b56 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -931,9 +931,9 @@ static void complete_signal(int sig, struct task_struct *p, int group)
 	 * then start taking the whole group down immediately.
 	 */
 	if (sig_fatal(p, sig) &&
-	    !(signal->flags & (SIGNAL_UNKILLABLE | SIGNAL_GROUP_EXIT)) &&
+	    !(signal->flags & SIGNAL_GROUP_EXIT) &&
 	    !sigismember(&t->real_blocked, sig) &&
-	    (sig == SIGKILL || !t->ptrace)) {
+	    (sig == SIGKILL || !p->ptrace)) {
 		/*
 		 * This signal will be fatal to the whole group.
 		 */
-- 
2.5.0

  parent reply	other threads:[~2017-11-03 18:42 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CAP045Ap9Xv67GaeskDt_gAajp1Cni_S0Z0u_vsuw_ptRUuJD6Q@mail.gmail.com>
     [not found] ` <20171102160705.GA11973@redhat.com>
2017-11-03 18:41   ` [PATCH 0/3] fix SIGNAL_UNKILLABLE && SIGKILL interaction Oleg Nesterov
2017-11-03 18:42     ` [PATCH 1/3] protect the traced SIGNAL_UNKILLABLE tasks from SIGKILL Oleg Nesterov
2017-11-03 18:42     ` [PATCH 2/3] protect the SIGNAL_UNKILLABLE tasks from !sig_kernel_only() signals Oleg Nesterov
2017-11-03 18:42     ` Oleg Nesterov [this message]
2017-11-03 19:14       ` [PATCH 3/3] remove the no longer needed SIGNAL_UNKILLABLE check in complete_signal() Kees Cook
2017-11-13 14:53     ` [PATCH 0/3] fix SIGNAL_UNKILLABLE && SIGKILL interaction Oleg Nesterov
2017-11-13 20:00       ` Kyle Huey

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171103184246.GD21036@redhat.com \
    --to=oleg@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=dvyukov@google.com \
    --cc=ebiederm@xmission.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=me@kylehuey.com \
    --cc=robert@ocallahan.org \
    --cc=security@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.