Message-ID: <202006011225.E58FC9CCA@keescook>
Date:   Mon, 1 Jun 2020 12:29:27 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Christian Brauner <christian.brauner@...ntu.com>
Cc:     linux-kernel@...r.kernel.org, Andy Lutomirski <luto@...nel.org>,
        Tycho Andersen <tycho@...ho.ws>,
        Matt Denton <mpdenton@...gle.com>,
        Sargun Dhillon <sargun@...gun.me>,
        Jann Horn <jannh@...gle.com>, Chris Palmer <palmer@...gle.com>,
        Aleksa Sarai <cyphar@...har.com>,
        Robert Sesek <rsesek@...gle.com>,
        Jeffrey Vander Stoep <jeffv@...gle.com>,
        Linux Containers <containers@...ts.linux-foundation.org>
Subject: Re: [PATCH v3 3/4] seccomp: notify about unused filter

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@...ho.ws>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Matt Denton <mpdenton@...gle.com>
> Cc: Sargun Dhillon <sargun@...gun.me>
> Cc: Jann Horn <jannh@...gle.com>
> Cc: Chris Palmer <palmer@...gle.com>
> Cc: Aleksa Sarai <cyphar@...har.com>
> Cc: Robert Sesek <rsesek@...gle.com>
> Cc: Jeffrey Vander Stoep <jeffv@...gle.com>
> Cc: Linux Containers <containers@...ts.linux-foundation.org>
> Signed-off-by: Christian Brauner <christian.brauner@...ntu.com>
> ---
> /* v2 */
> - Jann Horn <jannh@...gle.com>:
>   - Use more descriptive instead of seccomp_filter_notify().
>     (I went with seccomp_filter_release().)
> 
> /* v3 */
> - Kees Cook <keescook@...omium.org>:
>   - 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.

-- 
Kees Cook

Powered by blists - more mailing lists