linux-kernel - Re: [PATCH 2/2] sys, seccomp: add PR_SECCOMP_EXT and SECCOMP_EXT_ACT

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: <20140115190445.GA18166@redhat.com>
Date:	Wed, 15 Jan 2014 20:04:45 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Will Drewry <wad@...omium.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Nicolas Schichan <nschichan@...ebox.fr>,
	Kees Cook <keescook@...omium.org>,
	James Morris <james.l.morris@...cle.com>,
	Andrew Morton <akpm@...ux-foundation.org>, holt@....com,
	Alexander Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH 2/2] sys, seccomp: add PR_SECCOMP_EXT and
	SECCOMP_EXT_ACT_TSYNC

On 01/14, Will Drewry wrote:
>
> On Tue, Jan 14, 2014 at 1:21 PM, Oleg Nesterov <oleg@...hat.com> wrote:
>
> >> +                     get_seccomp_filter(caller);
> >> +                     /*
> >> +                      * Drop the task reference to the shared ancestor since
> >> +                      * current's path will hold a reference.  (This also
> >> +                      * allows a put before the assignment.)
> >> +                      */
> >> +                     put_seccomp_filter(thread);
> >> +                     thread->seccomp.filter = caller->seccomp.filter;
> >
> > As I said, I do not understand this patch yet, but this looks suspicious.
> >
> > Why we can't race with this thread doing clone(CLONE_THREAD) ? We do
> > not the the new thread yet, but its ->seccomp can be already copied
> > by copy_process(), no?
>
> Yeah I missed that. That said, I think the worst of it would be that
> the new thread
> gets the old filter.

Yes, but this means you can trust SECCOMP_EXT_ACT_TSYNC.

> I'll see if
> the siglock helps
> here and walk the clone() code again to see what else I missed.

No, siglock itself can't help to avoid this race. Unless you move
copy_process()->get_seccomp_filter() under the same lock, and in
this case it should also re-copy ->seccomp. Not nice.

But note task_lock() (or any other per-thread locking) is wrong.
Just look at the code above. We hold task_lock(thread) but not
task_lock(caller). What if another thread calls seccomp_sync_threads()
and changes caller->seccomp right after get_seccomp_filter(caller).

And even get_seccomp_filter() itself becomes racy. I think the
locking is seriously broken in this series.

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/