lkml.org 
[lkml]   [2020]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] seccomp: allow TSYNC and USER_NOTIF together
On Mon, Mar 02, 2020 at 09:48:34PM -0800, Kees Cook wrote:
> On Thu, Feb 06, 2020 at 09:50:27AM -0700, Tycho Andersen wrote:
> > The restriction introduced in 7a0df7fbc145 ("seccomp: Make NEW_LISTENER and
> > TSYNC flags exclusive") is mostly artificial: there is enough information
> > in a seccomp user notification to tell which thread triggered a
> > notification. The reason it was introduced is because TSYNC makes the
> > syscall return a thread-id on failure, and NEW_LISTENER returns an fd, and
> > there's no way to distinguish between these two cases (well, I suppose the
> > caller could check all fds it has, then do the syscall, and if the return
> > value was an fd that already existed, then it must be a thread id, but
> > bleh).
> >
> > Matthew would like to use these two flags together in the Chrome sandbox
> > which wants to use TSYNC for video drivers and NEW_LISTENER to proxy
> > syscalls.
> >
> > So, let's fix this ugliness by adding another flag, NO_TID_ON_TSYNC_ERR,
> > which tells the kernel to just return -EAGAIN on a TSYNC error. This way,
> > NEW_LISTENER (and any subsequent seccomp() commands that want to return
> > positive values) don't conflict with each other.
> >
> > Suggested-by: Matthew Denton <mpdenton@google.com>
> > Signed-off-by: Tycho Andersen <tycho@tycho.ws>
>
> Thanks for this! (And thanks for waiting on my review!) Yeah, this
> makes things much more sensible. If we get a third thing that wants
> to be returned, we'll have to rev the userspace struct API to have an
> "output" area. :P

Yeah :)

> Bike shedding below...
>
> > ---
> > include/linux/seccomp.h | 3 +-
> > include/uapi/linux/seccomp.h | 1 +
> > kernel/seccomp.c | 14 +++-
> > tools/testing/selftests/seccomp/seccomp_bpf.c | 74 ++++++++++++++++++-
> > 4 files changed, 86 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> > index 03583b6d1416..e0560a941ed1 100644
> > --- a/include/linux/seccomp.h
> > +++ b/include/linux/seccomp.h
> > @@ -7,7 +7,8 @@
> > #define SECCOMP_FILTER_FLAG_MASK (SECCOMP_FILTER_FLAG_TSYNC | \
> > SECCOMP_FILTER_FLAG_LOG | \
> > SECCOMP_FILTER_FLAG_SPEC_ALLOW | \
> > - SECCOMP_FILTER_FLAG_NEW_LISTENER)
> > + SECCOMP_FILTER_FLAG_NEW_LISTENER | \
> > + SECCOMP_FILTER_FLAG_NO_TID_ON_TSYNC_ERR)
> >
> > #ifdef CONFIG_SECCOMP
> >
> > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> > index be84d87f1f46..64678cc20e18 100644
> > --- a/include/uapi/linux/seccomp.h
> > +++ b/include/uapi/linux/seccomp.h
> > @@ -22,6 +22,7 @@
> > #define SECCOMP_FILTER_FLAG_LOG (1UL << 1)
> > #define SECCOMP_FILTER_FLAG_SPEC_ALLOW (1UL << 2)
> > #define SECCOMP_FILTER_FLAG_NEW_LISTENER (1UL << 3)
> > +#define SECCOMP_FILTER_FLAG_NO_TID_ON_TSYNC_ERR (1UL << 4)
>
> Bikeshed: what do you think about calling this
>
> SECCOMP_FILTER_FLAG_TSYNC_ESRCH
>
> to mean "I don't care _which_ thread, just fail" (See below about the
> ESRCH bit...)

Will do.

> >
> > /*
> > * All BPF programs must return a 32-bit value.
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index b6ea3dcb57bf..fa01ec085d60 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -528,8 +528,12 @@ static long seccomp_attach_filter(unsigned int flags,
> > int ret;
> >
> > ret = seccomp_can_sync_threads();
> > - if (ret)
> > - return ret;
> > + if (ret) {
> > + if (flags & SECCOMP_FILTER_FLAG_NO_TID_ON_TSYNC_ERR)
> > + return -EAGAIN;
>
> Hm hm, I think EAGAIN is wrong here: this isn't likely to be a transient
> failure (unless the offending thread dies). The two ways TSYNC can fail
> are if a thread has seccomp mode 1 set, or if the thread's filter
> ancestry has already diverged. Trying again isn't really going to help
> (which is why the original motivation was to return thread details to
> help debug why TSYNC failed).
>
> In the case where the thread id can't be found (container visibility??),
> we fail with -ESRCH. That might be more sensible than -EAGAIN here. (Or
> maybe -EBUSY?)

-ESRCH seems good, I'll resend with that.

Cheers,

Tycho

\
 
 \ /
  Last update: 2020-03-03 15:59    [W:0.050 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site