Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 3 Mar 2020 07:56:35 -0700
From:   Tycho Andersen <tycho@tycho.ws>
To:     Kees Cook <keescook@chromium.org>
Cc:     linux-kernel@vger.kernel.org, Matthew Denton <mpdenton@google.com>
Subject: Re: [PATCH] seccomp: allow TSYNC and USER_NOTIF together
Message-ID: <20200303145635.GC15783@cisco>
References: <20200206165027.18415-1-tycho@tycho.ws>
 <202003022137.9DAB55E6F0@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202003022137.9DAB55E6F0@keescook>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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