linux-kernel - Re: [PATCH 2/3] seccomp: release task filters when the task exits

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: <CAEWA0a6Z+jGKtz1Bdc2tP4YwR6u1zeEhoYY1hYZqdxCZMcgjcg@mail.gmail.com>
Date: Wed, 22 May 2024 00:06:36 -0700
From: Andrei Vagin <avagin@...gle.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Kees Cook <keescook@...omium.org>, Tycho Andersen <tandersen@...flix.com>, 
	Andy Lutomirski <luto@...capital.net>, Will Drewry <wad@...omium.org>, Jens Axboe <axboe@...nel.dk>, 
	Christian Brauner <brauner@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] seccomp: release task filters when the task exits

> On Thu, May 16, 2024 at 6:10 AM Oleg Nesterov <oleg@...hat.com> wrote:
> >
> > On 05/16, Oleg Nesterov wrote:
> > >
> > > On 05/15, Andrei Vagin wrote:
> > > >
> > > > seccomp_sync_threads and seccomp_can_sync_threads should be considered too.
> > >
> > > Yes. But we only need to consider them in the multi-thread case, right?
> > > In this case exit_signals() sets PF_EXITING under ->siglock, so they can't
> > > miss this flag, seccomp_filter_release() doesn't need to take siglock.
> >                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

PF_EXITING is set without holding ->siglock if tsk->signal has the
SIGNAL_GROUP_EXIT flag. I think it can be a case when one thread is in
seccomp_sync_threads and others are exiting. The first thread can check
that PF_EXITING isn't set for another thread. Then, the second thread calls
exit_signals and seccomp_filter_release(), and finally, the first thread
sets its seccomp.filter to the second thread. If seccomp_filter_release takes
siglock, it will be handled properly.

> >
> > Ah, no. seccomp_filter_release() does need to take ->siglock even if we
> > forget about proc_pid_seccomp_cache().
> >
> > Without siglock
> >
> >         orig = tsk->seccomp.filter;
> >
> > can leak into the critical section in exit_signals() (spin_unlock is the
> > one-way barrier) and this LOAD can be reordered with "flags |= PF_EXITING".
> >
> > Hmm. I thought we have something smp_mb__after_unlock(), but it seems we
> > don't. So we can't add a fast-path

We have smp_mb__after_unlock_lock in include/linux/rcupdate.h.

> >
> >         if (!tsk->seccomp.filter)
> >                 return;
> >
> > check at the start of seccomp_filter_release().
> >
> >
> > Cough... Now that I look at seccomp_can_sync_threads() I think it too
> > doesn't need the PF_EXITING check.
> >
> > If it is called before seccomp_filter_release(), this doesn't really
> > differ from the case when it is called before do_exit/exit_signals.
> >
> > If it is called after seccomp_filter_release(), then is_ancestor()
> > must be true.
> >
> > But perhaps I missed something, I won't insist, up to you.
> >

You are right, this check isn't required in seccomp_can_sync_threads, but
I decided that it is better to be consistent with seccomp_sync_threads.

Thanks,
Andrei