wait/ptrace: always assume __WALL if the child is traced

The following program (simplified version of generated by syzkaller)

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

	void *thread_func(void *arg)
	{
		ptrace(PTRACE_TRACEME, 0,0,0);
		return 0;
	}

	int main(void)
	{
		pthread_t thread;

		if (fork())
			return 0;

		while (getppid() != 1)
			;

		pthread_create(&thread, NULL, thread_func, NULL);
		pthread_join(thread, NULL);
		return 0;
	}

creates the unreapable zombie if /sbin/init doesn't use __WALL.

This is not a kernel bug, at least in a sense that everything works as
expected: debugger should reap a traced sub-thread before it can reap the leader, but without __WALL/__WCLONE do_wait() ignores sub-threads.

Unfortunately, it seems that /sbin/init in most (all?) distributions doesn't use it and we have to change the kernel to avoid the problem.

Quoted 35 lines · On Tue, 20 Oct 2015 19:17:54 +0200 Oleg Nesterov <[email protected]> wrote:

The following program (simplified version of generated by syzkaller)

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

	void *thread_func(void *arg)
	{
		ptrace(PTRACE_TRACEME, 0,0,0);
		return 0;
	}

	int main(void)
	{
		pthread_t thread;

		if (fork())
			return 0;

		while (getppid() != 1)
			;

		pthread_create(&thread, NULL, thread_func, NULL);
		pthread_join(thread, NULL);
		return 0;
	}

creates the unreapable zombie if /sbin/init doesn't use __WALL.

This is not a kernel bug, at least in a sense that everything works as
expected: debugger should reap a traced sub-thread before it can reap the leader, but without __WALL/__WCLONE do_wait() ignores sub-threads.

Quoted 2 lines

Unfortunately, it seems that /sbin/init in most (all?) distributions doesn't use it and we have to change the kernel to avoid the problem.

Well, to fix this a distro needs to roll out a new kernel. Or a new init(8). Is there any reason to believe that distributing/deploying a new kernel is significantly easier for everyone? Because fixing init sounds like a much preferable solution to this problem.

Quoted 35 lines · On Tue, 20 Oct 2015 19:17:54 +0200 Oleg Nesterov <[email protected]> wrote:

The following program (simplified version of generated by syzkaller)

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

	void *thread_func(void *arg)
	{
		ptrace(PTRACE_TRACEME, 0,0,0);
		return 0;
	}

	int main(void)
	{
		pthread_t thread;

		if (fork())
			return 0;

		while (getppid() != 1)
			;

		pthread_create(&thread, NULL, thread_func, NULL);
		pthread_join(thread, NULL);
		return 0;
	}

creates the unreapable zombie if /sbin/init doesn't use __WALL.

This is not a kernel bug, at least in a sense that everything works as
expected: debugger should reap a traced sub-thread before it can reap the leader, but without __WALL/__WCLONE do_wait() ignores sub-threads.

Quoted 2 lines

Unfortunately, it seems that /sbin/init in most (all?) distributions doesn't use it and we have to change the kernel to avoid the problem.

Well, to fix this a distro needs to roll out a new kernel. Or a new init(8). Is there any reason to believe that distributing/deploying a new kernel is significantly easier for everyone? Because fixing init sounds like a much preferable solution to this problem.

Quoted 41 lines · Andrew Morton

Quoted 37 lines · On Tue, 20 Oct 2015 19:17:54 +0200 Oleg Nesterov <[email protected]> wrote:

The following program (simplified version of generated by syzkaller)

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

	void *thread_func(void *arg)
	{
		ptrace(PTRACE_TRACEME, 0,0,0);
		return 0;
	}

	int main(void)
	{
		pthread_t thread;

		if (fork())
			return 0;

		while (getppid() != 1)
			;

		pthread_create(&thread, NULL, thread_func, NULL);
		pthread_join(thread, NULL);
		return 0;
	}

creates the unreapable zombie if /sbin/init doesn't use __WALL.

This is not a kernel bug, at least in a sense that everything works as
expected: debugger should reap a traced sub-thread before it can reap the leader, but without __WALL/__WCLONE do_wait() ignores sub-threads.

Unfortunately, it seems that /sbin/init in most (all?) distributions doesn't use it and we have to change the kernel to avoid the problem.

Quoted 4 lines

Well, to fix this a distro needs to roll out a new kernel. Or a new init(8). Is there any reason to believe that distributing/deploying a new kernel is significantly easier for everyone? Because fixing init sounds like a much preferable solution to this problem.

I will be happy if we decide that this is userpace problem and we should not fix the kernel. I simply do not know.

However, please look at 2/2 which imho makes sense regardless and looks "obviously safe". Without this patch waitid() can not use __WALL, so if /sbin/init uses waitid() then the userspace fix won't be one-liner. And at least Fedora22 and Ubuntu use waitid().

So personally I'd prefer 2/2 + fix-init, not sure if this can work...

Quoted 16 lines · On Wed, 21 Oct 2015 19:41:50 +0200 Oleg Nesterov <[email protected]> wrote:

Quoted 14 lines · Andrew Morton

Quoted 6 lines · On Tue, 20 Oct 2015 19:17:54 +0200 Oleg Nesterov <[email protected]> wrote:

This is not a kernel bug, at least in a sense that everything works as
expected: debugger should reap a traced sub-thread before it can reap the leader, but without __WALL/__WCLONE do_wait() ignores sub-threads.

Unfortunately, it seems that /sbin/init in most (all?) distributions doesn't use it and we have to change the kernel to avoid the problem.

Well, to fix this a distro needs to roll out a new kernel. Or a new init(8). Is there any reason to believe that distributing/deploying a new kernel is significantly easier for everyone? Because fixing init sounds like a much preferable solution to this problem.

Quoted 2 lines

I will be happy if we decide that this is userpace problem and we should not fix the kernel. I simply do not know.

The kernel patch sounds pretty sketchy - something we should avoid doing if at all possible.

Quoted 4 lines

However, please look at 2/2 which imho makes sense regardless and looks "obviously safe". Without this patch waitid() can not use __WALL, so if /sbin/init uses waitid() then the userspace fix won't be one-liner. And at least Fedora22 and Ubuntu use waitid().

2/2 does look sensible (needs a better changelog if it's to be a standalone thing),

but if we're expecting distros to fix this with an
updated init(8) only, then they can't assume that the kernel's waitid() has been altered.

So init will need the not-one-liner version of the fix.

Quoted 1 line

So personally I'd prefer 2/2 + fix-init, not sure if this can work...

I'm just guessing here. Are you (or someone) able to find out which approach the distros will prefer, and why?

And what has to be done to init(8) to fix this bug when running current kernels?

Quoted 35 lines · On Tue, 20 Oct 2015 19:17:54 +0200 Oleg Nesterov <[email protected]> wrote:

The following program (simplified version of generated by syzkaller)

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

	void *thread_func(void *arg)
	{
		ptrace(PTRACE_TRACEME, 0,0,0);
		return 0;
	}

	int main(void)
	{
		pthread_t thread;

		if (fork())
			return 0;

		while (getppid() != 1)
			;

		pthread_create(&thread, NULL, thread_func, NULL);
		pthread_join(thread, NULL);
		return 0;
	}

creates the unreapable zombie if /sbin/init doesn't use __WALL.

This is not a kernel bug, at least in a sense that everything works as
expected: debugger should reap a traced sub-thread before it can reap the leader, but without __WALL/__WCLONE do_wait() ignores sub-threads.

Quoted 2 lines

Unfortunately, it seems that /sbin/init in most (all?) distributions doesn't use it and we have to change the kernel to avoid the problem.

Well, to fix this a distro needs to roll out a new kernel. Or a new init(8). Is there any reason to believe that distributing/deploying a new kernel is significantly easier for everyone? Because fixing init sounds like a much preferable solution to this problem.

Quoted 37 lines · Andrew Morton

Quoted 35 lines · On Tue, 20 Oct 2015 19:17:54 +0200 Oleg Nesterov <[email protected]> wrote:

The following program (simplified version of generated by syzkaller)

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

	void *thread_func(void *arg)
	{
		ptrace(PTRACE_TRACEME, 0,0,0);
		return 0;
	}

	int main(void)
	{
		pthread_t thread;

		if (fork())
			return 0;

		while (getppid() != 1)
			;

		pthread_create(&thread, NULL, thread_func, NULL);
		pthread_join(thread, NULL);
		return 0;
	}

creates the unreapable zombie if /sbin/init doesn't use __WALL.

This is not a kernel bug, at least in a sense that everything works as
expected: debugger should reap a traced sub-thread before it can reap the leader, but without __WALL/__WCLONE do_wait() ignores sub-threads.

Quoted 7 lines

Quoted 2 lines

Unfortunately, it seems that /sbin/init in most (all?) distributions doesn't use it and we have to change the kernel to avoid the problem.

Well, to fix this a distro needs to roll out a new kernel. Or a new init(8). Is there any reason to believe that distributing/deploying a new kernel is significantly easier for everyone? Because fixing init sounds like a much preferable solution to this problem.

People will continue to write new init(8) implementations, and they will miss this obscure case.

Before this bug was found, it was considered possible to use a shell script as init process. What now, every shell needs to add __WALL to its waitpids?

The use of PTRACE_TRACEME in this reproducer is clearly pathological:
PTRACE_TRACEME was never intended to be used to attach to unsuspecting processes.

How about making PTRACE_TRACEME fail in this case?

Quoted 6 lines · Denys Vlasenko

Quoted 4 lines · Andrew Morton

Well, to fix this a distro needs to roll out a new kernel. Or a new init(8). Is there any reason to believe that distributing/deploying a new kernel is significantly easier for everyone? Because fixing init sounds like a much preferable solution to this problem.

Quoted 6 lines

People will continue to write new init(8) implementations, and they will miss this obscure case.

Before this bug was found, it was considered possible to use a shell script as init process. What now, every shell needs to add __WALL to its waitpids?

Quoted 3 lines

The use of PTRACE_TRACEME in this reproducer is clearly pathological:
PTRACE_TRACEME was never intended to be used to attach to unsuspecting processes.

Quoted 8 lines · Denys Vlasenko

Quoted 6 lines · Denys Vlasenko

Quoted 4 lines · Andrew Morton

Well, to fix this a distro needs to roll out a new kernel. Or a new init(8). Is there any reason to believe that distributing/deploying a new kernel is significantly easier for everyone? Because fixing init sounds like a much preferable solution to this problem.

Quoted 6 lines

Quoted 6 lines

People will continue to write new init(8) implementations, and they will miss this obscure case.

Before this bug was found, it was considered possible to use a shell script as init process. What now, every shell needs to add __WALL to its waitpids?

Why not? I think it can safely use __WALL too.

Quoted 3 lines

Quoted 3 lines

The use of PTRACE_TRACEME in this reproducer is clearly pathological:
PTRACE_TRACEME was never intended to be used to attach to unsuspecting processes.

Sure. But people do the things which were never intended to be used all the time. We simply can not know if this "feature" already has a creative user or not.

Quoted 16 lines · Oleg Nesterov

Quoted 14 lines · Denys Vlasenko

Quoted 12 lines · Denys Vlasenko

Quoted 4 lines · Andrew Morton

Well, to fix this a distro needs to roll out a new kernel. Or a new init(8). Is there any reason to believe that distributing/deploying a new kernel is significantly easier for everyone? Because fixing init sounds like a much preferable solution to this problem.

People will continue to write new init(8) implementations, and they will miss this obscure case.

Before this bug was found, it was considered possible to use a shell script as init process. What now, every shell needs to add __WALL to its waitpids?

Quoted 1 line

Why not? I think it can safely use __WALL too.

Because having any userspace program which can happen to be init, which includes all shells out there in the wild
(bash, dash, ash, ksh, zsh, msh, hush,...)
learn about __WALL is wrong: apart from this wart, they do not have to use any Linux-specific code. It can all be perfectly legitimate POSIX.

Quoted 3 lines

Sure. But people do the things which were never intended to be used all the time. We simply can not know if this "feature" already has a creative user or not.

It won't be unfixable: they will just have to switch from PTRACE_TRACEME to PTRACE_ATTACH.

As of now we do not know any people craz^W creative enough to create a cross between init and strace. If such specimens would materialize, don't they deserve to have to make that change?

Quoted 18 lines · Denys Vlasenko

Quoted 16 lines · Oleg Nesterov

Quoted 14 lines · Denys Vlasenko

Quoted 12 lines · Denys Vlasenko

Quoted 4 lines · Andrew Morton

Well, to fix this a distro needs to roll out a new kernel. Or a new init(8). Is there any reason to believe that distributing/deploying a new kernel is significantly easier for everyone? Because fixing init sounds like a much preferable solution to this problem.

People will continue to write new init(8) implementations, and they will miss this obscure case.

Before this bug was found, it was considered possible to use a shell script as init process. What now, every shell needs to add __WALL to its waitpids?

Quoted 7 lines

Quoted 1 line

Why not? I think it can safely use __WALL too.

Because having any userspace program which can happen to be init, which includes all shells out there in the wild
(bash, dash, ash, ksh, zsh, msh, hush,...)
learn about __WALL is wrong: apart from this wart, they do not have to use any Linux-specific code. It can all be perfectly legitimate POSIX.

Yes, this is true. I meant that they could safely use __WALL to, but I understand that this change can be painful.

Quoted 10 lines

Quoted 3 lines

Sure. But people do the things which were never intended to be used all the time. We simply can not know if this "feature" already has a creative user or not.

It won't be unfixable: they will just have to switch from PTRACE_TRACEME to PTRACE_ATTACH.

As of now we do not know any people craz^W creative enough to create a cross between init and strace. If such specimens would materialize, don't they deserve to have to make that change?

This also applies to people who use bash/whatever as /sbin/init and allow the untrusted users to run the exploits ;) I do not know who is more crazy.

In any case, the real question is whether we should change the kernel to fix the problem, or ask the distros to fix their init's. In the former case 1/2 looks simpler/safer to me than the change in ptrace_traceme(), and you seem to agree that 1/2 is not that bad.

Quoted 5 lines · Oleg Nesterov

In any case, the real question is whether we should change the kernel to fix the problem, or ask the distros to fix their init's. In the former case 1/2 looks simpler/safer to me than the change in ptrace_traceme(), and you seem to agree that 1/2 is not that bad.

A risk here seems to be that waitpid will start returning unexpected (thread) PIDs to parent processes,

and it's not unreasonable to assume
that e.g., a program asserts that waitpid either returns error or a known (process) PID.

That's not an init-only issue,

but something that might affect any
process that runs a child that happens to decide to call PTRACE_TRACEME.

The ptrace man page says:

"A process can initiate a trace by calling fork(2) and having the resulting child do a PTRACE_TRACEME, followed (typically) by an execve(2)."

Given that, can we instead make the kernel error out on PTRACE_TRACEME issued from a non-leader thread? Then between PTRACE_TRACEME and the parent's waitpid, __WALL or !__WALL should make no difference.

(Also, in the original test case, if the child gets/raises a signal or execs before exiting, the bash/init/whatever process won't be issuing PTRACE_CONT, and the child will thus end up stuck (though should be SIGKILLable,

All this because PTRACE_TRACEME is broken by design

Quoted 10 lines · Pedro Alves

Quoted 5 lines · Oleg Nesterov

In any case, the real question is whether we should change the kernel to fix the problem, or ask the distros to fix their init's. In the former case 1/2 looks simpler/safer to me than the change in ptrace_traceme(), and you seem to agree that 1/2 is not that bad.

A risk here seems to be that waitpid will start returning unexpected (thread) PIDs to parent processes,

I don't see how this change can make the things worse,

Quoted 3 lines

and it's not unreasonable to assume
that e.g., a program asserts that waitpid either returns error or a known (process) PID.

Well. /sbin/init can never assume this, obviously.

Quoted 2 lines

That's not an init-only issue,

Yes. Because we have CLONE_PARENT. So "waitpid either returns error or a known (process) PID" is only true if you trust your children.

Quoted 3 lines

(Also, in the original test case, if the child gets/raises a signal or execs before exiting, the bash/init/whatever process won't be issuing PTRACE_CONT, and the child will thus end up stuck (though should be SIGKILLable,

Oh, but if it is killable everything is fine. How does this differ from the case when, say, you jusr reparent to init and do kill(getpid(), SIGSTOP) ?

Quoted 1 line

All this because PTRACE_TRACEME is broken by design

Heh. I agree. But we can't fix it now.

Quoted 6 lines · Oleg Nesterov

Quoted 4 lines · Pedro Alves

(Also, in the original test case, if the child gets/raises a signal or execs before exiting, the bash/init/whatever process won't be issuing PTRACE_CONT, and the child will thus end up stuck (though should be SIGKILLable,

Quoted 2 lines

Oh, but if it is killable everything is fine. How does this differ from the case when, say, you jusr reparent to init and do kill(getpid(), SIGSTOP) ?

The difference is that if the child called PTRACE_TRACEME, then it goes to ptrace-stop instead and no amount of SIGCONT unstucks it -- the only way out is force killing. I agree it's not a major issue as there's a way out (and thus made it a parens), but I wouldn't call it nice either.

Quoted 3 lines

Quoted 1 line

All this because PTRACE_TRACEME is broken by design

Heh. I agree. But we can't fix it now.

Perhaps the man page could document it as deprecated, suggesting PTRACE_ATTACH/PTRACE_SEIZE instead?

creates the unreapable zombie if /sbin/init doesn't use __WALL.

This is not a kernel bug, at least in a sense that everything works as
expected: debugger should reap a traced sub-thread before it can reap the leader, but without __WALL/__WCLONE do_wait() ignores sub-threads.

Unfortunately, it seems that /sbin/init in most (all?) distributions doesn't use it and we have to change the kernel to avoid the problem.

This patch just adds the "ptrace" check into eligible_child(). To some degree this matches the "tsk->ptrace" in exit_notify(), ->exit_signal is mostly ignored when the tracee reports to debugger.

This obviously means the user-visible change: __WCLONE and __WALL no longer have any meaning for debugger. And I can only hope that this won't break something.

We could make a more conservative change. Say, we can take __WCLONE into account, or !thread_group_leader(). But it would be nice to not complicate these historical/confusing checks.

Damn. I simply do not know what should/can we do. From the change log:

And I can only hope that this won't break something.

yet this patch cc's -stable.

Please see the changelog, but in short: this is not a kernel bug but unlikely we can fix all distributions, so I think we have to change the kernel.

HOWEVER. With this change __WCLONE and __WALL have no effect for debugger, do_wait() works as if __WALL is set if the child (natural or not) is traced.

Jan, Pedro, could you please confirm this won't break gdb? I tried to look into gdb-7.1, and at first glance gdb uses __WCLONE only because __WALL doesn't work on older kernels, iow it seems to me that gdb actually wants __WALL so this change should be fine.

Any other ideas?

Oleg.

Jan, Pedro, could you please confirm this won't break gdb? I tried to look into gdb-7.1, and at first glance gdb uses __WCLONE only because __WALL doesn't work on older kernels, iow it seems to me that gdb actually wants __WALL so this change should be fine.

In any case, to make sure existing gdb binaries would still work with your kernel change, I ran GDB's testsuite with this:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

gdb/nat/linux-waitpid.c +14 -0

index cbcdd95..864ba2e 100644
@@ -149,3 +149,17 @@ my_waitpid (int pid, int *status, int flags)
   errno = out_errno;
   return ret;
 }
+
+#include <dlfcn.h>
+
+pid_t
+waitpid (pid_t pid, int *status, int options)
+{
+  static pid_t (*waitpid2) (pid_t pid, int *status, int options) = NULL;
+
+  if (waitpid2 == NULL)
+    waitpid2 = dlsym (RTLD_NEXT, "waitpid");
+
+  options |= __WALL;
+  return waitpid2 (pid, status, options);
+}

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~