Re: [PATCH v3 3/4] seccomp: notify about unused filter

[Christian Brauner]

On Mon, Jun 01, 2020 at 12:29:27PM -0700, Kees Cook wrote:
> On Sun, May 31, 2020 at 01:50:30PM +0200, Christian Brauner wrote:
> > We've been making heavy use of the seccomp notifier to intercept and
> > handle certain syscalls for containers. This patch allows a syscall
> > supervisor listening on a given notifier to be notified when a seccomp
> > filter has become unused.
> > 
> > A container is often managed by a singleton supervisor process the
> > so-called "monitor". This monitor process has an event loop which has
> > various event handlers registered. If the user specified a seccomp
> > profile that included a notifier for various syscalls then we also
> > register a seccomp notify even handler. For any container using a
> > separate pid namespace the lifecycle of the seccomp notifier is bound to
> > the init process of the pid namespace, i.e. when the init process exits
> > the filter must be unused.
> > If a new process attaches to a container we force it to assume a seccomp
> > profile. This can either be the same seccomp profile as the container
> > was started with or a modified one. If the attaching process makes use
> > of the seccomp notifier we will register a new seccomp notifier handler
> > in the monitor's event loop. However, when the attaching process exits
> > we can't simply delete the handler since other child processes could've
> > been created (daemons spawned etc.) that have inherited the seccomp
> > filter and so we need to keep the seccomp notifier fd alive in the event
> > loop. But this is problematic since we don't get a notification when the
> > seccomp filter has become unused and so we currently never remove the
> > seccomp notifier fd from the event loop and just keep accumulating fds
> > in the event loop. We've had this issue for a while but it has recently
> > become more pressing as more and larger users make use of this.
> > 
> > To fix this, we introduce a new "users" reference counter that tracks
> > any tasks and dependent filters making use of a filter. When a notifier is
> > registered waiting tasks will be notified that the filter is now empty by
> > receiving a (E)POLLHUP event.
> > The concept in this patch introduces is the same as for signal_struct,
> > i.e. reference counting for life-cycle management is decoupled from
> > reference counting taks using the object.
> > 
> > There's probably some trickery possible but the second counter is just
> > the correct way of doing this imho and has precedence. The patch also
> > lifts the waitqeue from struct notification into sruct seccomp_filter.
> > This is cleaner overall and let's us avoid having to take the notifier
> > mutex since we neither need to read nor modify the notifier specific
> > aspects of the seccomp filter. In the exit path I'd very much like to
> > avoid having to take the notifier mutex for each filter in the task's
> > filter hierarchy.
> > 
> > Cc: Tycho Andersen <tycho@xxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > Cc: Matt Denton <mpdenton@xxxxxxxxxx>
> > Cc: Sargun Dhillon <sargun@xxxxxxxxx>
> > Cc: Jann Horn <jannh@xxxxxxxxxx>
> > Cc: Chris Palmer <palmer@xxxxxxxxxx>
> > Cc: Aleksa Sarai <cyphar@xxxxxxxxxx>
> > Cc: Robert Sesek <rsesek@xxxxxxxxxx>
> > Cc: Jeffrey Vander Stoep <jeffv@xxxxxxxxxx>
> > Cc: Linux Containers <containers@xxxxxxxxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> > ---
> > /* v2 */
> > - Jann Horn <jannh@xxxxxxxxxx>:
> >   - Use more descriptive instead of seccomp_filter_notify().
> >     (I went with seccomp_filter_release().)
> > 
> > /* v3 */
> > - Kees Cook <keescook@xxxxxxxxxxxx>:
> >   - Rename counter from "live" to "users".
> > ---
> >  kernel/seccomp.c | 68 +++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 53 insertions(+), 15 deletions(-)
> > 
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index 55251b1fe03f..45244f1ba148 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -94,13 +94,11 @@ struct seccomp_knotif {
> >   *           filter->notify_lock.
> >   * @next_id: The id of the next request.
> >   * @notifications: A list of struct seccomp_knotif elements.
> > - * @wqh: A wait queue for poll.
> >   */
> 
> I split the wait queue changes into a separate patch...
> 
> >  /**
> > - * seccomp_filter_release - Detach the task from its filter tree
> > - *			    and drop its reference count during
> > - *			    exit.
> > + * seccomp_filter_release - Detach the task from its filter tree,
> > + *			    drop its reference count, and notify
> > + *			    about unused filters
> >   *
> >   * This function should only be called when the task is exiting as
> >   * it detaches it from its filter tree.
> >   */
> >  void seccomp_filter_release(struct task_struct *tsk)
> >  {
> > -	struct seccomp_filter *cur = tsk->seccomp.filter;
> > +	struct seccomp_filter *orig = tsk->seccomp.filter;
> >  
> > +	/* Detach task from its filter tree. */
> >  	tsk->seccomp.filter = NULL;
> > -	__put_seccomp_filter(cur);
> > +	/* Notify about any unused filters in the task's former filter tree. */
> > +	__seccomp_filter_orphan(orig);
> > +	/* Finally drop all references to the task's former tree. */
> > +	__put_seccomp_filter(orig);
> >  }
> 
> I added __seccomp_filter_release() to do the filter-specific parts (the
> two functions passing "orig" above, so that it can be reused later...
> 
> >  
> >  /**
> > @@ -419,18 +441,29 @@ static inline void seccomp_sync_threads(unsigned long flags)
> >  	/* Synchronize all threads. */
> >  	caller = current;
> >  	for_each_thread(caller, thread) {
> > +		struct seccomp_filter *cur = thread->seccomp.filter;
> > +
> >  		/* Skip current, since it needs no changes. */
> >  		if (thread == caller)
> >  			continue;
> >  
> >  		/* Get a task reference for the new leaf node. */
> >  		get_seccomp_filter(caller);
> > +
> > +		/*
> > +		 * Notify everyone as we're forcing the thread
> > +		 * to orphan its current filter tree.
> > +		 */
> > +		__seccomp_filter_orphan(cur);
> > +
> >  		/*
> > -		 * Drop the task reference to the shared ancestor since
> > -		 * current's path will hold a reference.  (This also
> > -		 * allows a put before the assignment.)
> > +		 * Drop the task's 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->seccomp.filter);
> > +		__put_seccomp_filter(cur);
> 
> I switched this around to just call the new __seccomp_release_filter()
> (there's no need to open-code this and add "cur"). I also removed the
> comment about the notification, because that's not possible: "thread"
> shares the same filter hierarchy as "caller", so the counts on "cur"
> cannot reach 0 (no notifications can ever happen due to TSYNC).
> 
> Everything else looks great! I've applied it to for-next/seccomp.

Excellent, thanks!

Just in case this isn't obvious to everyone, I want to point out, that
this patchset means the seccomp notifier can be (ab)used to receive exit
notifications for a task tree (given some caveats) sharing the same
filter. In any case, I'd rather have a proper thread management
implementation of this through pidfds in the future if there's a need
for this. But one can abuse seccomp to achieve something similar with
my change here.
(Technically, we could also expand the notifier to have a "listen" mode
 whereby it only notifies about e.g. filter installation and filter)
 orphanage.)

Christian