All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] ptrace: make ptrace() fail if the tracee changed its pid unexpectedly
@ 2020-12-17 14:29 Oleg Nesterov
  2020-12-17 23:39 ` Eric W. Biederman
  0 siblings, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2020-12-17 14:29 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Eric W. Biederman, Eugene Syromiatnikov, Jan Kratochvil,
	Linus Torvalds, Mathieu Desnoyers, Michael Kerrisk, Pedro Alves,
	Simon Marchi, linux-kernel

Suppose we have 2 threads, the group-leader L and a sub-theread T,
both parked in ptrace_stop(). Debugger tries to resume both threads
and does

	ptrace(PTRACE_CONT, T);
	ptrace(PTRACE_CONT, L);

If the sub-thread T execs in between, the 2nd PTRACE_CONT doesn not
resume the old leader L, it resumes the post-exec thread T which was
actually now stopped in PTHREAD_EVENT_EXEC. In this case the
PTHREAD_EVENT_EXEC event is lost, and the tracer can't know that the
tracee changed its pid.

This patch makes ptrace() fail in this case until debugger does wait()
and consumes PTHREAD_EVENT_EXEC which reports old_pid. This affects all
ptrace requests except the "asynchronous" PTRACE_INTERRUPT/KILL.

The patch doesn't add the new PTRACE_ option to not complicate the API,
and I _hope_ this won't cause any noticeable regression:

	- If debugger uses PTRACE_O_TRACEEXEC and the thread did an exec
	  and the tracer does a ptrace request without having consumed
	  the exec event, it's 100% sure that the thread the ptracer
	  thinks it is targeting does not exist anymore, or isn't the
	  same as the one it thinks it is targeting.

	- To some degree this patch adds nothing new. In the scenario
	  above ptrace(L) can fail with -ESRCH if it is called after the
	  execing sub-thread wakes the leader up and before it "steals"
	  the leader's pid.

Test-case:

	#include <stdio.h>
	#include <unistd.h>
	#include <signal.h>
	#include <sys/ptrace.h>
	#include <sys/wait.h>
	#include <errno.h>
	#include <pthread.h>
	#include <assert.h>

	void *tf(void *arg)
	{
		execve("/usr/bin/true", NULL, NULL);
		assert(0);

		return NULL;
	}

	int main(void)
	{
		int leader = fork();
		if (!leader) {
			kill(getpid(), SIGSTOP);

			pthread_t th;
			pthread_create(&th, NULL, tf, NULL);
			for (;;)
				pause();

			return 0;
		}

		waitpid(leader, NULL, WSTOPPED);

		ptrace(PTRACE_SEIZE, leader, 0,
				PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC);
		waitpid(leader, NULL, 0);

		ptrace(PTRACE_CONT, leader, 0,0);
		waitpid(leader, NULL, 0);

		int status, thread = waitpid(-1, &status, 0);
		assert(thread > 0 && thread != leader);
		assert(status == 0x80137f);

		ptrace(PTRACE_CONT, thread, 0,0);
		/*
		 * waitid() because waitpid(leader, &status, WNOWAIT) does not
		 * report status. Why ????
		 *
		 * Why WEXITED? because we have another kernel problem connected
		 * to mt-exec.
		 */
		siginfo_t info;
		assert(waitid(P_PID, leader, &info, WSTOPPED|WEXITED|WNOWAIT) == 0);
		assert(info.si_pid == leader && info.si_status == 0x0405);

		/* OK, it sleeps in ptrace(PTRACE_EVENT_EXEC == 0x04) */
		assert(ptrace(PTRACE_CONT, leader, 0,0) == -1);
		assert(errno == ESRCH);

		assert(leader == waitpid(leader, &status, WNOHANG));
		assert(status == 0x04057f);

		assert(ptrace(PTRACE_CONT, leader, 0,0) == 0);

		return 0;
	}

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/ptrace.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 43d6179508d6..1037251ae4a5 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -169,6 +169,27 @@ void __ptrace_unlink(struct task_struct *child)
 	spin_unlock(&child->sighand->siglock);
 }
 
+static bool looks_like_a_spurious_pid(struct task_struct *task)
+{
+	int pid;
+
+	if (task->exit_code != ((PTRACE_EVENT_EXEC << 8) | SIGTRAP))
+		return false;
+
+	rcu_read_lock();
+	pid = task_pid_nr_ns(task, task_active_pid_ns(task->parent));
+	rcu_read_unlock();
+
+	if (pid == task->ptrace_message)
+		return false;
+	/*
+	 * The tracee changed its pid but the PTRACE_EVENT_EXEC event
+	 * was not wait()'ed, most probably debugger targets the old
+	 * leader which was destroyed in de_thread().
+	 */
+	return true;
+}
+
 /* Ensure that nothing can wake it up, even SIGKILL */
 static bool ptrace_freeze_traced(struct task_struct *task)
 {
@@ -179,7 +200,8 @@ static bool ptrace_freeze_traced(struct task_struct *task)
 		return ret;
 
 	spin_lock_irq(&task->sighand->siglock);
-	if (task_is_traced(task) && !__fatal_signal_pending(task)) {
+	if (task_is_traced(task) && !looks_like_a_spurious_pid(task) &&
+	    !__fatal_signal_pending(task)) {
 		task->state = __TASK_TRACED;
 		ret = true;
 	}
-- 
2.25.1.362.g51ebf55



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] ptrace: make ptrace() fail if the tracee changed its pid unexpectedly
  2020-12-17 14:29 [RFC PATCH] ptrace: make ptrace() fail if the tracee changed its pid unexpectedly Oleg Nesterov
@ 2020-12-17 23:39 ` Eric W. Biederman
  2020-12-18 14:10   ` Oleg Nesterov
  2020-12-19 16:19   ` Pedro Alves
  0 siblings, 2 replies; 7+ messages in thread
From: Eric W. Biederman @ 2020-12-17 23:39 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, Eugene Syromiatnikov, Jan Kratochvil,
	Linus Torvalds, Mathieu Desnoyers, Michael Kerrisk, Pedro Alves,
	Simon Marchi, linux-kernel

Oleg Nesterov <oleg@redhat.com> writes:

> Suppose we have 2 threads, the group-leader L and a sub-theread T,
> both parked in ptrace_stop(). Debugger tries to resume both threads
> and does
>
> 	ptrace(PTRACE_CONT, T);
> 	ptrace(PTRACE_CONT, L);
>
> If the sub-thread T execs in between, the 2nd PTRACE_CONT doesn not
> resume the old leader L, it resumes the post-exec thread T which was
> actually now stopped in PTHREAD_EVENT_EXEC. In this case the
> PTHREAD_EVENT_EXEC event is lost, and the tracer can't know that the
> tracee changed its pid.

The change seems sensible.  I don't expect this is common but it looks
painful to deal with if it happens.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

I am wondering if this should be expanded to all ptrace types for
consistency.  Or maybe we should set a flag to make this happen for
all ptrace events.

It just seems really odd to only worry about missing this event.
I admit this a threaded PTRACE_EVENT_EXEC is the only event we are
likely to miss but still.

Do you by any chance have any debugger/strace test cases?

I would think that would be the way to test to see if this breaks
anything.  I think I remember strace having a good test suite.

> This patch makes ptrace() fail in this case until debugger does wait()
> and consumes PTHREAD_EVENT_EXEC which reports old_pid. This affects all
> ptrace requests except the "asynchronous" PTRACE_INTERRUPT/KILL.
>
> The patch doesn't add the new PTRACE_ option to not complicate the API,
> and I _hope_ this won't cause any noticeable regression:
>
> 	- If debugger uses PTRACE_O_TRACEEXEC and the thread did an exec
> 	  and the tracer does a ptrace request without having consumed
> 	  the exec event, it's 100% sure that the thread the ptracer
> 	  thinks it is targeting does not exist anymore, or isn't the
> 	  same as the one it thinks it is targeting.
>
> 	- To some degree this patch adds nothing new. In the scenario
> 	  above ptrace(L) can fail with -ESRCH if it is called after the
> 	  execing sub-thread wakes the leader up and before it "steals"
> 	  the leader's pid.
>
> Test-case:
>
> 	#include <stdio.h>
> 	#include <unistd.h>
> 	#include <signal.h>
> 	#include <sys/ptrace.h>
> 	#include <sys/wait.h>
> 	#include <errno.h>
> 	#include <pthread.h>
> 	#include <assert.h>
>
> 	void *tf(void *arg)
> 	{
> 		execve("/usr/bin/true", NULL, NULL);
> 		assert(0);
>
> 		return NULL;
> 	}
>
> 	int main(void)
> 	{
> 		int leader = fork();
> 		if (!leader) {
> 			kill(getpid(), SIGSTOP);
>
> 			pthread_t th;
> 			pthread_create(&th, NULL, tf, NULL);
> 			for (;;)
> 				pause();
>
> 			return 0;
> 		}
>
> 		waitpid(leader, NULL, WSTOPPED);
>
> 		ptrace(PTRACE_SEIZE, leader, 0,
> 				PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC);
> 		waitpid(leader, NULL, 0);
>
> 		ptrace(PTRACE_CONT, leader, 0,0);
> 		waitpid(leader, NULL, 0);
>
> 		int status, thread = waitpid(-1, &status, 0);
> 		assert(thread > 0 && thread != leader);
> 		assert(status == 0x80137f);
>
> 		ptrace(PTRACE_CONT, thread, 0,0);
> 		/*
> 		 * waitid() because waitpid(leader, &status, WNOWAIT) does not
> 		 * report status. Why ????
> 		 *
> 		 * Why WEXITED? because we have another kernel problem connected
> 		 * to mt-exec.
> 		 */
> 		siginfo_t info;
> 		assert(waitid(P_PID, leader, &info, WSTOPPED|WEXITED|WNOWAIT) == 0);
> 		assert(info.si_pid == leader && info.si_status == 0x0405);
>
> 		/* OK, it sleeps in ptrace(PTRACE_EVENT_EXEC == 0x04) */
> 		assert(ptrace(PTRACE_CONT, leader, 0,0) == -1);
> 		assert(errno == ESRCH);
>
> 		assert(leader == waitpid(leader, &status, WNOHANG));
> 		assert(status == 0x04057f);
>
> 		assert(ptrace(PTRACE_CONT, leader, 0,0) == 0);
>
> 		return 0;
> 	}
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/ptrace.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 43d6179508d6..1037251ae4a5 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -169,6 +169,27 @@ void __ptrace_unlink(struct task_struct *child)
>  	spin_unlock(&child->sighand->siglock);
>  }
>  
> +static bool looks_like_a_spurious_pid(struct task_struct *task)
> +{
> +	int pid;
> +
> +	if (task->exit_code != ((PTRACE_EVENT_EXEC << 8) | SIGTRAP))
> +		return false;
> +
> +	rcu_read_lock();
> +	pid = task_pid_nr_ns(task, task_active_pid_ns(task->parent));
> +	rcu_read_unlock();
> +
> +	if (pid == task->ptrace_message)
> +		return false;
> +	/*
> +	 * The tracee changed its pid but the PTRACE_EVENT_EXEC event
> +	 * was not wait()'ed, most probably debugger targets the old
> +	 * leader which was destroyed in de_thread().
> +	 */
> +	return true;
> +}
> +
>  /* Ensure that nothing can wake it up, even SIGKILL */
>  static bool ptrace_freeze_traced(struct task_struct *task)
>  {
> @@ -179,7 +200,8 @@ static bool ptrace_freeze_traced(struct task_struct *task)
>  		return ret;
>  
>  	spin_lock_irq(&task->sighand->siglock);
> -	if (task_is_traced(task) && !__fatal_signal_pending(task)) {
> +	if (task_is_traced(task) && !looks_like_a_spurious_pid(task) &&
> +	    !__fatal_signal_pending(task)) {
>  		task->state = __TASK_TRACED;
>  		ret = true;
>  	}

Eric

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] ptrace: make ptrace() fail if the tracee changed its pid unexpectedly
  2020-12-17 23:39 ` Eric W. Biederman
@ 2020-12-18 14:10   ` Oleg Nesterov
  2020-12-21 20:04     ` Eric W. Biederman
  2020-12-19 16:19   ` Pedro Alves
  1 sibling, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2020-12-18 14:10 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Andrew Morton, Eugene Syromiatnikov, Jan Kratochvil,
	Linus Torvalds, Mathieu Desnoyers, Michael Kerrisk, Pedro Alves,
	Simon Marchi, linux-kernel

On 12/17, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > Suppose we have 2 threads, the group-leader L and a sub-theread T,
> > both parked in ptrace_stop(). Debugger tries to resume both threads
> > and does
> >
> > 	ptrace(PTRACE_CONT, T);
> > 	ptrace(PTRACE_CONT, L);
> >
> > If the sub-thread T execs in between, the 2nd PTRACE_CONT doesn not
> > resume the old leader L, it resumes the post-exec thread T which was
> > actually now stopped in PTHREAD_EVENT_EXEC. In this case the
> > PTHREAD_EVENT_EXEC event is lost, and the tracer can't know that the
> > tracee changed its pid.
>
> The change seems sensible.  I don't expect this is common but it looks
> painful to deal with if it happens.

Yes, this is not a bug, but gdb can't handle this case without some help
from the kernel.

> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Thanks!



> I am wondering if this should be expanded to all ptrace types for
> consistency.  Or maybe we should set a flag to make this happen for
> all ptrace events.

But for what? ptrace is the very old API, I don't think we want to
suddenly enforce the rule that every reported event must be wait()'ed.
Plus this needs some complications to support WNOWAIT.

I would like to kill JOBCTL_TRAPPING_BIT which ensures that the tracer
does NOT need wait() after PTRACE_ATTACH(stopped-task) (see
wait_on_bit() in ptrace_attach()). I think this makes no sense but
who knows, perhaps even this change can break something.

> It just seems really odd to only worry about missing this event.

Agreed,

> I admit this a threaded PTRACE_EVENT_EXEC is the only event we are
> likely to miss but still.

Yes, this is the only event debugger can miss even if it uses wait()
correctly.

> Do you by any chance have any debugger/strace test cases?
>
> I would think that would be the way to test to see if this breaks
> anything.  I think I remember strace having a good test suite.

Heh. You can never know what other people do with ptrace ;)
For example, see

    fab840fc2d54 ptrace: PTRACE_DETACH should do flush_ptrace_hw_breakpoint(child)
    35114fcbe0b9 Revert "ptrace: PTRACE_DETACH should do flush_ptrace_hw_breakpoint(child)"

Oleg.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] ptrace: make ptrace() fail if the tracee changed its pid unexpectedly
  2020-12-17 23:39 ` Eric W. Biederman
  2020-12-18 14:10   ` Oleg Nesterov
@ 2020-12-19 16:19   ` Pedro Alves
  2020-12-19 19:33     ` Oleg Nesterov
  1 sibling, 1 reply; 7+ messages in thread
From: Pedro Alves @ 2020-12-19 16:19 UTC (permalink / raw)
  To: Eric W. Biederman, Oleg Nesterov
  Cc: Andrew Morton, Eugene Syromiatnikov, Jan Kratochvil,
	Linus Torvalds, Mathieu Desnoyers, Michael Kerrisk, Simon Marchi,
	linux-kernel

On 12/17/20 11:39 PM, Eric W. Biederman wrote:

>> resume the old leader L, it resumes the post-exec thread T which was
>> actually now stopped in PTHREAD_EVENT_EXEC. In this case the
>> PTHREAD_EVENT_EXEC event is lost, and the tracer can't know that the
>> tracee changed its pid.
> 
> The change seems sensible.  I don't expect this is common but it looks
> painful to deal with if it happens.

Yeah, the debug session becomes completely messed up, as the ptracer has no
idea the process is running a new image, breakpoints were wiped out, and
the post-exec process is resumed without the ptracer having had a chance
to install new breakpoints.  I don't see any way to deal with it without
kernel help.

> 
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> I am wondering if this should be expanded to all ptrace types for
> consistency.  Or maybe we should set a flag to make this happen for
> all ptrace events.
> 
> It just seems really odd to only worry about missing this event.
> I admit this a threaded PTRACE_EVENT_EXEC is the only event we are
> likely to miss but still.

It's more about the tid stealing than the event itself.  I mean,
we lose the event because the tid changes magically without warning.
An exec is the only scenario where this happens.

> 
> Do you by any chance have any debugger/strace test cases?
> 
> I would think that would be the way to test to see if this breaks
> anything.  I think I remember strace having a good test suite.

I ran the GDB testsuite against this, no regressions showed up.

BTW, the problem was discovered by Simon Marchi when he tried to write
a GDB testcase for a multi-threaded exec scenario:

 https://sourceware.org/bugzilla/show_bug.cgi?id=26754

I've went through GDB's code looking for potential issues with the change and whether
it would affect GDBs already in the wild.  Tricky corner cases abound, but I think
we're good.  Feel free to add my ack:

Acked-by: Pedro Alves <palves@redhat.com>

Thanks,
Pedro Alves


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] ptrace: make ptrace() fail if the tracee changed its pid unexpectedly
  2020-12-19 16:19   ` Pedro Alves
@ 2020-12-19 19:33     ` Oleg Nesterov
  2020-12-20  4:48       ` Simon Marchi
  0 siblings, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2020-12-19 19:33 UTC (permalink / raw)
  To: Pedro Alves
  Cc: Eric W. Biederman, Andrew Morton, Eugene Syromiatnikov,
	Jan Kratochvil, Linus Torvalds, Mathieu Desnoyers,
	Michael Kerrisk, Simon Marchi, linux-kernel

On 12/19, Pedro Alves wrote:
>
> BTW, the problem was discovered by Simon Marchi when he tried to write
> a GDB testcase for a multi-threaded exec scenario:

OOPS! Sorry Simon, yes I forgot to add reported-by. Andrew, or Eric, if
you take this patch, could you also add

	Reported-by: Simon Marchi <simon.marchi@efficios.com>

> I've went through GDB's code looking for potential issues with the change and whether
> it would affect GDBs already in the wild.  Tricky corner cases abound, but I think
> we're good.  Feel free to add my ack:
>
> Acked-by: Pedro Alves <palves@redhat.com>

Thanks!

Oleg.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] ptrace: make ptrace() fail if the tracee changed its pid unexpectedly
  2020-12-19 19:33     ` Oleg Nesterov
@ 2020-12-20  4:48       ` Simon Marchi
  0 siblings, 0 replies; 7+ messages in thread
From: Simon Marchi @ 2020-12-20  4:48 UTC (permalink / raw)
  To: Oleg Nesterov, Pedro Alves
  Cc: Eric W. Biederman, Andrew Morton, Eugene Syromiatnikov,
	Jan Kratochvil, Linus Torvalds, Mathieu Desnoyers,
	Michael Kerrisk, linux-kernel

On 2020-12-19 2:33 p.m., Oleg Nesterov wrote:
> OOPS! Sorry Simon, yes I forgot to add reported-by. Andrew, or Eric, if
> you take this patch, could you also add
> 
> 	Reported-by: Simon Marchi <simon.marchi@efficios.com>

I tried the original reproducer on a patched kernel, and it looks good.
GDB's behavior is still not super clean when this situation happens: a
PTRACE_GETREGS on the (disappeared) leader now fails with ESRCH (that's
what we want), and that interrupts the "continue" command and
unexpectedly brings back the prompt while leaving the other thread
running.  But that is all logic that will have to be fixed inside GDB.

So, feel free to add

  Acked-by: Simon Marchi <simon.marchi@efficios.com>

too.

Thanks!

Simon

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] ptrace: make ptrace() fail if the tracee changed its pid unexpectedly
  2020-12-18 14:10   ` Oleg Nesterov
@ 2020-12-21 20:04     ` Eric W. Biederman
  0 siblings, 0 replies; 7+ messages in thread
From: Eric W. Biederman @ 2020-12-21 20:04 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, Eugene Syromiatnikov, Jan Kratochvil,
	Linus Torvalds, Mathieu Desnoyers, Michael Kerrisk, Pedro Alves,
	Simon Marchi, linux-kernel

Oleg Nesterov <oleg@redhat.com> writes:

> On 12/17, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> writes:
>>
>> > Suppose we have 2 threads, the group-leader L and a sub-theread T,
>> > both parked in ptrace_stop(). Debugger tries to resume both threads
>> > and does
>> >
>> > 	ptrace(PTRACE_CONT, T);
>> > 	ptrace(PTRACE_CONT, L);
>> >
>> > If the sub-thread T execs in between, the 2nd PTRACE_CONT doesn not
>> > resume the old leader L, it resumes the post-exec thread T which was
>> > actually now stopped in PTHREAD_EVENT_EXEC. In this case the
>> > PTHREAD_EVENT_EXEC event is lost, and the tracer can't know that the
>> > tracee changed its pid.
>>
>> The change seems sensible.  I don't expect this is common but it looks
>> painful to deal with if it happens.
>
> Yes, this is not a bug, but gdb can't handle this case without some help
> from the kernel.

>> I admit this a threaded PTRACE_EVENT_EXEC is the only event we are
>> likely to miss but still.
>
> Yes, this is the only event debugger can miss even if it uses wait()
> correctly.

I think that is my confusion with the patch.  The uniqueness of this
case is not described well.


Eric

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-12-21 20:06 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-17 14:29 [RFC PATCH] ptrace: make ptrace() fail if the tracee changed its pid unexpectedly Oleg Nesterov
2020-12-17 23:39 ` Eric W. Biederman
2020-12-18 14:10   ` Oleg Nesterov
2020-12-21 20:04     ` Eric W. Biederman
2020-12-19 16:19   ` Pedro Alves
2020-12-19 19:33     ` Oleg Nesterov
2020-12-20  4:48       ` Simon Marchi

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.