Messages in this thread | | | Date | Tue, 3 Mar 2020 07:56:35 -0700 | From | Tycho Andersen <> | Subject | Re: [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
| |