linux-kernel - Re: [PATCH v3] ptrace: Fix fork event messages across pid namespaces

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 2 Apr 2014 16:58:50 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Matthew Dempsky <mdempsky@...omium.org>
Cc:	Kees Cook <keescook@...omium.org>,
	Julien Tinnes <jln@...omium.org>,
	Roland McGrath <mcgrathr@...omium.org>,
	Jan Kratochvil <jan.kratochvil@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] ptrace: Fix fork event messages across pid
	namespaces

On 04/01, Matthew Dempsky wrote:
>
> @@ -1605,10 +1605,12 @@ long do_fork(unsigned long clone_flags,
>  	 */
>  	if (!IS_ERR(p)) {
>  		struct completion vfork;
> +		struct pid *pid;
>
>  		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.

> +		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))
> -				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.

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

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

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/