linux-kernel - Re: [PATCH v4] 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]

Message-ID: <20140403154413.GA16684@redhat.com>
Date:	Thu, 3 Apr 2014 17:44:13 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Matthew Dempsky <mdempsky@...omium.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	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 v4] ptrace: Fix fork event messages across pid
	namespaces

On 04/02, Matthew Dempsky wrote:
>
> 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 <mdempsky@...omium.org>

Thanks,

Acked-by: Oleg Nesterov <oleg@...hat.com>



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.

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/