LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Eric Biederman <ebiederm@xmission.com>,
	Oleg Nesterov <oleg@redhat.com>,
	linux-kernel@vger.kernel.org
Subject: [PATCH][RFC] fix PTRACE_KILL
Date: Mon, 16 Aug 2021 21:50:52 +0000	[thread overview]
Message-ID: <YRrdvKEu2JQxLI5n@zeniv-ca.linux.org.uk> (raw)

[Cc'd to security@k.o, *NOT* because I consider it a serious security hole;
it's just that the odds of catching relevant reviewers there are higher
than on l-k and there doesn't seem to be any lists where that would be
on-topic.  My apologies for misuse of security@k.o ;-/]

Current implementation is racy in quite a few ways - we check that
the child is traced by us and use ptrace_resume() to feed it
SIGKILL, provided that it's still alive.

What we do not do is making sure that the victim is in ptrace stop;
as the result, it can go and violate all kinds of assumptions,
starting with "child->sighand won't change under ptrace_resume()",
"child->ptrace won't get changed under user_disable_single_step()",
etc.

Note that ptrace(2) manpage has this to say:
    
PTRACE_KILL
      Send  the  tracee a SIGKILL to terminate it.  (addr and data are
      ignored.)
    
      This operation is deprecated; do not use it!   Instead,  send  a
      SIGKILL  directly  using kill(2) or tgkill(2).  The problem with
      PTRACE_KILL is that it requires the  tracee  to  be  in  signal-
      delivery-stop,  otherwise  it  may  not work (i.e., may complete
      successfully but won't kill the tracee).  By contrast, sending a
      SIGKILL directly has no such limitation.
    
So let it check (under tasklist_lock) that the victim is traced by us
and call sig_send_info() to feed it SIGKILL.  It's easier that trying
to force ptrace_resume() into handling that mess and it's less brittle
that way.
    
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index f8589bf8d7dc..7f46be488b36 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -1220,11 +1220,6 @@ int ptrace_request(struct task_struct *child, long request,
 	case PTRACE_CONT:
 		return ptrace_resume(child, request, data);
 
-	case PTRACE_KILL:
-		if (child->exit_state)	/* already dead */
-			return 0;
-		return ptrace_resume(child, request, SIGKILL);
-
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
 	case PTRACE_GETREGSET:
 	case PTRACE_SETREGSET: {
@@ -1270,6 +1265,20 @@ int ptrace_request(struct task_struct *child, long request,
 	return ret;
 }
 
+static int ptrace_kill(struct task_struct *child)
+{
+	int ret = -ESRCH;
+
+	read_lock(&tasklist_lock);
+	if (child->ptrace && child->parent == current) {
+		if (!child->exit_state)
+			send_sig_info(SIGKILL, SEND_SIG_PRIV, child);
+		ret = 0;
+	}
+	read_unlock(&tasklist_lock);
+	return ret;
+}
+
 #ifndef arch_ptrace_attach
 #define arch_ptrace_attach(child)	do { } while (0)
 #endif
@@ -1304,8 +1313,12 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
 		goto out_put_task_struct;
 	}
 
-	ret = ptrace_check_attach(child, request == PTRACE_KILL ||
-				  request == PTRACE_INTERRUPT);
+	if (request == PTRACE_KILL) {
+		ret = ptrace_kill(child);
+		goto out_put_task_struct;
+	}
+
+	ret = ptrace_check_attach(child, request == PTRACE_INTERRUPT);
 	if (ret < 0)
 		goto out_put_task_struct;
 
@@ -1449,8 +1462,12 @@ COMPAT_SYSCALL_DEFINE4(ptrace, compat_long_t, request, compat_long_t, pid,
 		goto out_put_task_struct;
 	}
 
-	ret = ptrace_check_attach(child, request == PTRACE_KILL ||
-				  request == PTRACE_INTERRUPT);
+	if (request == PTRACE_KILL) {
+		ret = ptrace_kill(child);
+		goto out_put_task_struct;
+	}
+
+	ret = ptrace_check_attach(child, request == PTRACE_INTERRUPT);
 	if (!ret) {
 		ret = compat_arch_ptrace(child, request, addr, data);
 		if (ret || request != PTRACE_DETACH)

             reply	other threads:[~2021-08-16 21:51 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-16 21:50 Al Viro [this message]
2021-08-17 18:21 ` Eric W. Biederman
2021-08-25  5:12   ` Al Viro
2021-08-27 18:54     ` Linus Torvalds
2021-08-27 22:05       ` Eric W. Biederman

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=YRrdvKEu2JQxLI5n@zeniv-ca.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=ebiederm@xmission.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@redhat.com \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [PATCH][RFC] fix PTRACE_KILL' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).