linux-kernel - Re: [PATCH v5] 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, 30 Apr 2014 13:51:55 +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 v5] ptrace: Fix fork event messages across pid
	namespaces

On 04/29, Matthew Dempsky wrote:
>
> On Tue, Apr 29, 2014 at 3:11 PM, Andrew Morton <akpm@...ux-foundation.org> wrote:
> > 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?

Yes, still/again

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



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

And I think we should just ignore this very unlikely and harmless race.

We do not see a simple way to close it and in fact this ptrace_event() is
inherently racy anyway. Even without namespaces, if we race with DETACH +
ATTACH, the new tracer gets the correct child's pid, but the child can be
already untraced.

_Perhaps_ we can do something better later (to remind, we can setup
->ptrace_message beforehand and change ATTACH to clear it), but this is
more subtle and needs more changes.

This patch is straightforward, and it fixes the old/known problem: currently
this pid_t is always wrong unless the tracer is from the root namespace.

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/