Fix ptrace events across pid namespaces

Quoted 5 lines · Oleg Nesterov

Quoted 5 lines

kernel/fork.c +0 -0

@@ -1621,12 +1621,18 @@ long do_fork(unsigned long clone_flags,
              wake_up_new_task(p);
 

Quoted 10 lines

kernel/fork.c long do_fork(unsigned long clone_flags, · line 1627

Quoted 7 lines

kernel/fork.c +4 -2

              /* forking complete and child started to run, tell ptracer */
-             if (unlikely(trace))
-                     ptrace_event(trace, nr);
+             if (unlikely(trace)) {
+                     pid_t pid = task_pid_nr_ns(p,
+                             task_active_pid_ns(current->parent));
+                     ptrace_event(trace, pid);

This is unsafe, both "p" or ->parent can go away. In fact "p" can be already dead/freed/etc.

True. I suspect the problem of "p" going away could be solved by calculating the pid prior to calling wake_up_new_task(p)?

But how can I guard against current->parent disappearing? Do I need to grab a read lock on the tasklist and rcu?

Quoted 5 lines

Quoted 5 lines

kernel/ptrace.c +0 -0

@@ -80,6 +80,7 @@ void __ptrace_unlink(struct task_struct *child)
      BUG_ON(!child->ptrace);
 

Quoted 4 lines

kernel/ptrace.c void __ptrace_unlink(struct task_struct *child) · line 83

Quoted 2 lines

kernel/ptrace.c +1 -0

      child->ptrace = 0;
+     child->ptrace_message = 0;

It is too late for me to try to understand the reason for this change ;)

Basically, if process A is being ptraced by process B and we record an event for it, I don't want process B to detach and then process C to attach and be able to still view the event.

Quoted 3 lines

Probably this is related to detach/attach, but then it would be better to do this in PTRACE_ATTACH. Because ->ptrace_message can be non-zero if, say, a traced task forks.

I think zeroing out ptrace_message at attach time works too, and I can do it that way if you prefer.

Sorry for slow response,

Quoted 2 lines

But how can I guard against current->parent disappearing? Do I need to grab a read lock on the tasklist and rcu?

rcu_read_lock() should be enough.

The main question is how we can ensure that we use the correct namespace, iow, how we can ensure that current->parent won't change after this.

Suppose that debugger detaches right after we calculated pid_t we were going to report.

Now suppose that another task from the different namespace does ptrace(PTRACE_SEIZE, PTRACE_O_TRACEFORK). Or PTRACE_O_TRACEVFORKDONE.

In this case ptrace_event() will still report the wrong pid. I simply do not see how we can prevent this race without the really ugly complications.

But perhaps we do not really care? The race is very unlikely, and nothing really bad can happen. So perhaps your simple patch makes sense anyway, just the problem should be documented. Currently "nr" is always wrong if the tracer is from another ns, this change will obviously makes the things better in any case.

Quoted 3 lines

Basically, if process A is being ptraced by process B and we record an event for it, I don't want process B to detach and then process C to attach and be able to still view the event.

Aha, so I got it right. But see above, this won't help anyway, C can attach before ptrace_event() sets ->ptrace_message.

Perhaps this change makes sense anyway. I think it needs a separate patch but I won't insist.

Quoted 4 lines

Quoted 3 lines

Probably this is related to detach/attach, but then it would be better to do this in PTRACE_ATTACH. Because ->ptrace_message can be non-zero if, say, a traced task forks.

Quoted 2 lines

I think zeroing out ptrace_message at attach time works too, and I can do it that way if you prefer.

I do not really mind. But once again, unless we clear it in PTRACE_ATTACH, PTRACE_GETEVENTMSG still can return non-zero right after PTRACE_ATTACH. Of course we can change copy_process() to clear child->ptrace_message, but I don't think this would be better.

Matthew, I just noticed we discuss this off-list, I added lkml.

Revised patch below. Unfortunately, I just realized I don't have my test machine handy, so I won't be able to test it until tomorrow.

kernel/fork.c +15 -1

+		if (unlikely(trace)) {
+			/*
+			 * We want to report the child's pid as seen from the
+			 * tracer's pid namespace.
+			 * FIXME: We still risk sending a bogus event message if
+			 * debuggers from different pid namespaces detach and
+			 * reattach between rcu_read_unlock() and ptrace_stop().
+			 */
+			unsigned long message;
+			rcu_read_lock();
+			message = pid_nr_ns(pid,
+				task_active_pid_ns(current->parent));
+			rcu_read_unlock();
+			ptrace_event(trace, message);
+		}
 
 		if (clone_flags & CLONE_VFORK) {
-			if (!wait_for_vfork_done(p, &vfork))

Especially because this code needs a fix. Yes, rcu_read_lock() should be enough to ensure that ->parent and its namespace (if !NULL) can not go away, but task_active_pid_ns() can return NULL release_task(->parent) was already (although this race is pure theoretical). So this helper should also check it is !NULL under rcu_read_lock(), afaics.

(Hmm... off-topic, but get_pidns looks buggy by the same reason, I'll
send a fix).

pidns_get()->get_pid_ns() can hit ns == NULL. This task_struct can't go away, but task_active_pid_ns(task) is NULL if release_task(task) was already called. Alternatively we could change get_pid_ns(ns) to check ns != NULL, but it seems that other callers are fine.

Signed-off-by Oleg Nesterov <[email protected]>

kernel/pid_namespace.c +3 -1

@@ -318,7 +318,9 @@ static void *pidns_get(struct task_struct *task)
 	struct pid_namespace *ns;
 
 	rcu_read_lock();
-	ns = get_pid_ns(task_active_pid_ns(task));
+	ns = task_active_pid_ns(task);
+	if (ns)
+		get_pid_ns(ns);
 	rcu_read_unlock();
 
 	return ns;

Quoted 4 lines · Oleg Nesterov

pidns_get()->get_pid_ns() can hit ns == NULL. This task_struct can't go away, but task_active_pid_ns(task) is NULL if release_task(task) was already called. Alternatively we could change get_pid_ns(ns) to check ns != NULL, but it seems that other callers are fine.

get_pid_ns already has a ns == NULL check, so no code changes should be needed.

Quoted 10 lines

Quoted 4 lines · Oleg Nesterov

pidns_get()->get_pid_ns() can hit ns == NULL. This task_struct can't go away, but task_active_pid_ns(task) is NULL if release_task(task) was already called. Alternatively we could change get_pid_ns(ns) to check ns != NULL, but it seems that other callers are fine.

get_pid_ns already has a ns == NULL check, so no code changes should be needed.

Hmm...

	static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns)
	{
		if (ns != &init_pid_ns)
			kref_get(&ns->kref);
		return ns;
	}

?

Quoted 7 lines · Matthew Dempsky

kernel/fork.c +1 -0

@@ -1605,10 +1605,12 @@ long do_fork(unsigned long clone_flags,
       */
      if (!IS_ERR(p)) {
              struct completion vfork;
+             struct pid *pid;
 

kernel/fork.c long do_fork(unsigned long clone_flags, · line 1612

Quoted 4 lines

kernel/fork.c +1 -1

              trace_sched_process_fork(current, p);
 
-             nr = task_pid_vnr(p);
+             pid = get_task_pid(p, PIDTYPE_PID);

So you decided to use get_pid/put_pid ;) Honestly, I'd prefer to just calculate "pid_t trace_pid" before wake_up_new_task(), but I won't argue. Plus this way the race window becomes really small, OK.

Quoted 18 lines

kernel/fork.c +15 -1

+             if (unlikely(trace)) {
+                     /*
+                      * We want to report the child's pid as seen from the
+                      * tracer's pid namespace.
+                      * FIXME: We still risk sending a bogus event message if
+                      * debuggers from different pid namespaces detach and
+                      * reattach between rcu_read_unlock() and ptrace_stop().
+                      */
+                     unsigned long message;
+                     rcu_read_lock();
+                     message = pid_nr_ns(pid,
+                             task_active_pid_ns(current->parent));
+                     rcu_read_unlock();
+                     ptrace_event(trace, message);
+             }
 
              if (clone_flags & CLONE_VFORK) {
-                     if (!wait_for_vfork_done(p, &vfork))

kernel/fork.c long do_fork(unsigned long clone_flags, · line 1652

Quoted 10 lines

kernel/fork.c +9 -1

-                             ptrace_event(PTRACE_EVENT_VFORK_DONE, nr);
+                     if (!wait_for_vfork_done(p, &vfork)) {
+                             /* See comment above about pid namespaces. */
+                             unsigned long message;
+                             rcu_read_lock();
+                             message = pid_nr_ns(pid,
+                                     task_active_pid_ns(current->parent));
+                             rcu_read_unlock();
+                             ptrace_event(PTRACE_EVENT_VFORK_DONE, message);
+                     }

OK, but may I suggest you to make a helper? Note that the code under "if (trace)" and "if (CLONE_VFORK)" is the same. Even the comment above equally applies to the CLONE_VFORK branch.

Especially because this code needs a fix. Yes, rcu_read_lock() should be enough to ensure that ->parent and its namespace (if !NULL) can not go away, but task_active_pid_ns() can return NULL release_task(->parent) was already (although this race is pure theoretical). So this helper should also check it is !NULL under rcu_read_lock(), afaics.

And I forgot to mention, please send v5 to akpm. We usually route ptrace patches via -mm tree.

Otherwise, it looks like (e.g.) the attempts to get the parent's pid in fill_prstatus() and tomoyo_sys_getppid() are also theoretical races of the same kind?

Quoted 3 lines · Matthew Dempsky

Otherwise, it looks like (e.g.) the attempts to get the parent's pid in fill_prstatus() and tomoyo_sys_getppid() are also theoretical races of the same kind?

Doh, nevermind, those functions would potentially be use null "struct pid" pointers, not "struct pid_namespace" pointers; and pid_nr_ns() guards against pid being null.

When tracing a process in another pid namespace, it's important for fork event messages to contain the child's pid as seen from the tracer's pid namespace, not the parent's. Otherwise, the tracer won't be able to correlate the fork event with later SIGTRAP signals it receives from the child.

We still risk a race condition if a ptracer from a different pid namespace attaches after we compute the pid_t value. However, sending a bogus fork event message in this unlikely scenario is still a vast improvement over the status quo where we always send bogus fork event messages to debuggers in a different pid namespace than the forking process.

Signed-off-by Matthew Dempsky <[email protected]>

Some notes for potential future changes...

- I do not not see any potential user of ptrace_event_pid() outside of fork.c, so perhaps this helper should not be exported.

In fact I wouldn't mind if you send v5 which moves it into fork.c ;)

Quoted 14 lines · Matthew Dempsky

When tracing a process in another pid namespace, it's important for fork event messages to contain the child's pid as seen from the tracer's pid namespace, not the parent's. Otherwise, the tracer won't be able to correlate the fork event with later SIGTRAP signals it receives from the child.

We still risk a race condition if a ptracer from a different pid namespace attaches after we compute the pid_t value. However, sending a bogus fork event message in this unlikely scenario is still a vast improvement over the status quo where we always send bogus fork event messages to debuggers in a different pid namespace than the forking process.

Quoted 1 line

Signed-off-by Matthew Dempsky <[email protected]>

Thanks,

Acked-by Oleg Nesterov <[email protected]>

Some notes for potential future changes...

- I do not not see any potential user of ptrace_event_pid() outside of fork.c, so perhaps this helper should not be exported.

In fact I wouldn't mind if you send v5 which moves it into fork.c ;)

- The usage of "trace" doesn't look very consistent after this patch... OK, probably I'll try to cleanup this later.

- OTOH, calculating pid_nr in the namespace of ->parent can probably go into another simple (exported) helper. do_notify_parent_*() and exec_binprm() could use it, even they do not have the problem with task_active_pid_ns(parent) == NULL. Not sure.

- I am thinking about another approach... Suppose that we change ptrace_attach() to nullify ->ptrace_message, as we already discussed this probably makes sense anyway.

Now (ignoring CLONE_VFORK to simplify the discussion), do_fork() can do something like the following:

...

if (trace)
current->ptrace_message = calc_its_pid_in_parent_ns();

wake_up_new_task(p);

if (trace && current->ptrace_message && ptrace_event_enabled(trace))
ptrace_notify(...);

so that if we race with detach + attach we should likely see ->ptrace_message == 0 and report nothing as if the new debugger attached after do_fork().

Not sure this is really possible (without additional complications), and of course we still can't close the race completely.

But even if this can work and will not look too ugly, it would be better to do this on top of your simple patch.

Matthew, I see other emails from you, will try to reply tomorrow.

Quoted 2 lines · Oleg Nesterov

Some notes for potential future changes...

Quoted 4 lines

- I do not not see any potential user of ptrace_event_pid() outside of fork.c, so perhaps this helper should not be exported.

In fact I wouldn't mind if you send v5 which moves it into fork.c ;)

Like you mentioned, it's potentially used by fs/exec.c too, which I was intending to send a followup patch for.

Quoted 4 lines

- OTOH, calculating pid_nr in the namespace of ->parent can probably go into another simple (exported) helper. do_notify_parent_*() and exec_binprm() could use it, even they do not have the problem with task_active_pid_ns(parent) == NULL. Not sure.

I think do_notify_parent_*() are safe from task_active_pid_ns(parent) == NULL because they're under tasklist_lock,

but it looks like
exec_binprm() is theoretically racy

When tracing a process in another pid namespace, it's important for fork event messages to contain the child's pid as seen from the tracer's pid namespace, not the parent's. Otherwise, the tracer won't be able to correlate the fork event with later SIGTRAP signals it receives from the child.

We still risk a race condition if a ptracer from a different pid namespace attaches after we compute the pid_t value. However, sending a bogus fork event message in this unlikely scenario is still a vast improvement over the status quo where we always send bogus fork event messages to debuggers in a different pid namespace than the forking process.

include/linux/ptrace.h +7 -0

@@ -129,6 +130,36 @@ static inline void ptrace_event(int event, unsigned long message)
 }
 
 /**
+ * ptrace_event_pid - possibly stop for a ptrace event notification
+ * @event:	%PTRACE_EVENT_* value to report
+ * @pid:	process identifier for %PTRACE_GETEVENTMSG to return
+ *
+ * Check whether @event is enabled and, if so, report @event and @pid
+ * to the ptrace parent.  @pid is reported as the pid_t seen from the
+ * the ptrace parent's pid namespace.

include/linux/ptrace.h +11 -0

+	unsigned long message = 0;
+	struct pid_namespace *ns;
+
+	rcu_read_lock();
+	ns = task_active_pid_ns(rcu_dereference(current->parent));
+	if (ns)
+		message = pid_nr_ns(pid, ns);
+	rcu_read_unlock();
+
+	ptrace_event(event, message);
+}
 
...

More Oleg review would be nice, please ;)

Well that's a scary comment. If we're going to leave the code in this state then please carefully describe (within this comment) the *consequences* of the race. Does the kernel crash? Give away your ssh keys? If not then what.

And how would userspace recognize and/or recover from the race?

Quoted 1 line · Andrew Morton

More Oleg review would be nice, please ;)

FWIW, Oleg "acked" v4 earlier in the thread. Are you asking for further review from him beyond that?

Quoted 4 lines

Well that's a scary comment. If we're going to leave the code in this state then please carefully describe (within this comment) the *consequences* of the race. Does the kernel crash? Give away your ssh keys? If not then what.

Sorry, I can see how that comment could be scary without proper context. I added another sentence explaining the consequences are limited to the ptracer receiving a bogus pid_t value from PTRACE_GETEVENTMSG.

Quoted 1 line

And how would userspace recognize and/or recover from the race?

If the ptracer attaches via PTRACE_ATTACH, then there shouldn't be a
race: the ptracer can't use PTRACE_SETOPTIONS to request fork events until after the child has already stopped. So any SIGTRAP fork events that it receives before using PTRACE_SETOPTIONS it should disregard, because it hasn't asked the kernel to send them yet.

If the ptracer attaches via PTRACE_SEIZE and also requests fork events at the same time, then it would need to discard the first SIGTRAP it receives for the child if:

1. it's a fork event;
2. the ptracer can't otherwise prove the fork happened after the PTRACE_SEIZE rather than concurrently; and
3. the ptracer is concerned a ptracer from a different pid namespace may have just detached.

Quoted 1 line · Andrew Morton

More Oleg review would be nice, please ;)

FWIW, Oleg "acked" v4 earlier in the thread. Are you asking for further review from him beyond that?