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] [day] [month] [year] [list]

Message-ID: <CABqD9hYgn28EcF=AbyKSv-+pZH+yzV1L_P9dTnrezNRd220YtA@mail.gmail.com>
Date:	Wed, 15 Jan 2014 13:33:37 -0600
From:	Will Drewry <wad@...omium.org>
To:	Oleg Nesterov <oleg@...hat.com>
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 Wed, Jan 15, 2014 at 1:04 PM, Oleg Nesterov <oleg@...hat.com> wrote:
> 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.

Yeah - not at all.  I'll rethink it. I was too excited about how easy
is_ancestor works, but the locking is really the hard part.

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

Yup - I was thinking of tasklist_lock as a non-multi-reader lock,
which is wrong.
The task_lock(current) would clearly cover that case, but I need to walk through
all the interactions paying more attention to the lock being used.

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

It certainly needs to be better applied :)

thanks!
--
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/