lkml.org 
[lkml]   [2020]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 3/4] seccomp: notify about unused filter
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@tycho.ws>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Matt Denton <mpdenton@google.com>
> > Cc: Sargun Dhillon <sargun@sargun.me>
> > Cc: Jann Horn <jannh@google.com>
> > Cc: Chris Palmer <palmer@google.com>
> > Cc: Aleksa Sarai <cyphar@cyphar.com>
> > Cc: Robert Sesek <rsesek@google.com>
> > Cc: Jeffrey Vander Stoep <jeffv@google.com>
> > Cc: Linux Containers <containers@lists.linux-foundation.org>
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > /* v2 */
> > - Jann Horn <jannh@google.com>:
> > - Use more descriptive instead of seccomp_filter_notify().
> > (I went with seccomp_filter_release().)
> >
> > /* v3 */
> > - Kees Cook <keescook@chromium.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.

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

\
 
 \ /
  Last update: 2020-06-02 13:35    [W:1.806 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site