* [PATCH 1/2] seccomp: fix up grammar in comment @ 2019-03-06 20:14 Tycho Andersen 2019-03-06 20:14 ` [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags Tycho Andersen 2019-04-23 22:11 ` [PATCH 1/2] seccomp: fix up grammar in comment Kees Cook 0 siblings, 2 replies; 13+ messages in thread From: Tycho Andersen @ 2019-03-06 20:14 UTC (permalink / raw) To: Kees Cook; +Cc: linux-kernel, Tycho Andersen This sentence is kind of a train wreck anyway, but at least dropping the extra pronoun helps somewhat. Signed-off-by: Tycho Andersen <tycho@tycho.ws> --- kernel/seccomp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/seccomp.c b/kernel/seccomp.c index e815781ed751..d0d355ded2f4 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -329,7 +329,7 @@ static int is_ancestor(struct seccomp_filter *parent, * Expects sighand and cred_guard_mutex locks to be held. * * Returns 0 on success, -ve on error, or the pid of a thread which was - * either not in the correct seccomp mode or it did not have an ancestral + * either not in the correct seccomp mode or did not have an ancestral * seccomp filter. */ static inline pid_t seccomp_can_sync_threads(void) -- 2.19.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags 2019-03-06 20:14 [PATCH 1/2] seccomp: fix up grammar in comment Tycho Andersen @ 2019-03-06 20:14 ` Tycho Andersen 2019-03-06 20:39 ` Christian Brauner 2019-04-23 22:09 ` Kees Cook 2019-04-23 22:11 ` [PATCH 1/2] seccomp: fix up grammar in comment Kees Cook 1 sibling, 2 replies; 13+ messages in thread From: Tycho Andersen @ 2019-03-06 20:14 UTC (permalink / raw) To: Kees Cook; +Cc: linux-kernel, Tycho Andersen, stable As the comment notes, the return codes for TSYNC and NEW_LISTENER conflict, because they both return positive values, one in the case of success and one in the case of error. So, let's disallow both of these flags together. While this is technically a userspace break, all the users I know of are still waiting on me to land this feature in libseccomp, so I think it'll be safe. Also, at present my use case doesn't require TSYNC at all, so this isn't a big deal to disallow. If someone wanted to support this, a path forward would be to add a new flag like TSYNC_AND_LISTENER_YES_I_UNDERSTAND_THAT_TSYNC_WILL_JUST_RETURN_EAGAIN, but the use cases are so different I don't see it really happening. Finally, it's worth noting that this does actually fix a UAF issue: at the end of seccomp_set_mode_filter(), we have: if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { if (ret < 0) { listener_f->private_data = NULL; fput(listener_f); put_unused_fd(listener); } else { fd_install(listener, listener_f); ret = listener; } } out_free: seccomp_filter_free(prepared); But if ret > 0 because TSYNC raced, we'll install the listener fd and then free the filter out from underneath it, causing a UAF when the task closes it or dies. This patch also switches the condition to be simply if (ret), so that if someone does add the flag mentioned above, they won't have to remember to fix this too. Signed-off-by: Tycho Andersen <tycho@tycho.ws> Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") CC: stable@vger.kernel.org # v5.0+ --- kernel/seccomp.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/kernel/seccomp.c b/kernel/seccomp.c index d0d355ded2f4..79bada51091b 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -500,7 +500,10 @@ seccomp_prepare_user_filter(const char __user *user_filter) * * Caller must be holding current->sighand->siglock lock. * - * Returns 0 on success, -ve on error. + * Returns 0 on success, -ve on error, or + * - in TSYNC mode: the pid of a thread which was either not in the correct + * seccomp mode or did not have an ancestral seccomp filter + * - in NEW_LISTENER mode: the fd of the new listener */ static long seccomp_attach_filter(unsigned int flags, struct seccomp_filter *filter) @@ -1256,6 +1259,16 @@ static long seccomp_set_mode_filter(unsigned int flags, if (flags & ~SECCOMP_FILTER_FLAG_MASK) return -EINVAL; + /* + * In the successful case, NEW_LISTENER returns the new listener fd. + * But in the failure case, TSYNC returns the thread that died. If you + * combine these two flags, there's no way to tell whether something + * succeded or failed. So, let's disallow this combination. + */ + if ((flags & SECCOMP_FILTER_FLAG_TSYNC) && + (flags && SECCOMP_FILTER_FLAG_NEW_LISTENER)) + return -EINVAL; + /* Prepare the new filter before holding any locks. */ prepared = seccomp_prepare_user_filter(filter); if (IS_ERR(prepared)) @@ -1302,7 +1315,7 @@ static long seccomp_set_mode_filter(unsigned int flags, mutex_unlock(¤t->signal->cred_guard_mutex); out_put_fd: if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { - if (ret < 0) { + if (ret) { listener_f->private_data = NULL; fput(listener_f); put_unused_fd(listener); -- 2.19.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags 2019-03-06 20:14 ` [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags Tycho Andersen @ 2019-03-06 20:39 ` Christian Brauner 2019-03-06 20:46 ` Tycho Andersen 2019-04-23 22:09 ` Kees Cook 1 sibling, 1 reply; 13+ messages in thread From: Christian Brauner @ 2019-03-06 20:39 UTC (permalink / raw) To: Tycho Andersen; +Cc: Kees Cook, linux-kernel, stable On Wed, Mar 06, 2019 at 01:14:13PM -0700, Tycho Andersen wrote: > As the comment notes, the return codes for TSYNC and NEW_LISTENER conflict, > because they both return positive values, one in the case of success and > one in the case of error. So, let's disallow both of these flags together. > > While this is technically a userspace break, all the users I know of are > still waiting on me to land this feature in libseccomp, so I think it'll be > safe. Also, at present my use case doesn't require TSYNC at all, so this > isn't a big deal to disallow. If someone wanted to support this, a path > forward would be to add a new flag like > TSYNC_AND_LISTENER_YES_I_UNDERSTAND_THAT_TSYNC_WILL_JUST_RETURN_EAGAIN, but > the use cases are so different I don't see it really happening. > > Finally, it's worth noting that this does actually fix a UAF issue: at the end > of seccomp_set_mode_filter(), we have: > > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > if (ret < 0) { > listener_f->private_data = NULL; > fput(listener_f); > put_unused_fd(listener); > } else { > fd_install(listener, listener_f); > ret = listener; > } > } > out_free: > seccomp_filter_free(prepared); > > But if ret > 0 because TSYNC raced, we'll install the listener fd and then free > the filter out from underneath it, causing a UAF when the task closes it or > dies. This patch also switches the condition to be simply if (ret), so that > if someone does add the flag mentioned above, they won't have to remember > to fix this too. > > Signed-off-by: Tycho Andersen <tycho@tycho.ws> > Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") > CC: stable@vger.kernel.org # v5.0+ > --- > kernel/seccomp.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index d0d355ded2f4..79bada51091b 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -500,7 +500,10 @@ seccomp_prepare_user_filter(const char __user *user_filter) > * > * Caller must be holding current->sighand->siglock lock. > * > - * Returns 0 on success, -ve on error. > + * Returns 0 on success, -ve on error, or > + * - in TSYNC mode: the pid of a thread which was either not in the correct > + * seccomp mode or did not have an ancestral seccomp filter > + * - in NEW_LISTENER mode: the fd of the new listener > */ > static long seccomp_attach_filter(unsigned int flags, > struct seccomp_filter *filter) > @@ -1256,6 +1259,16 @@ static long seccomp_set_mode_filter(unsigned int flags, > if (flags & ~SECCOMP_FILTER_FLAG_MASK) > return -EINVAL; > > + /* > + * In the successful case, NEW_LISTENER returns the new listener fd. > + * But in the failure case, TSYNC returns the thread that died. If you > + * combine these two flags, there's no way to tell whether something > + * succeded or failed. So, let's disallow this combination. > + */ > + if ((flags & SECCOMP_FILTER_FLAG_TSYNC) && > + (flags && SECCOMP_FILTER_FLAG_NEW_LISTENER)) > + return -EINVAL; May license a manpage entry that this makes it potentially unsafe to use with multiple threads. But I don't see a use-case for this right now so it looks sane to me. :) (Though one simple question below.) Acked-by: Christian Brauner <christian.brauner@ubuntu.com> > + > /* Prepare the new filter before holding any locks. */ > prepared = seccomp_prepare_user_filter(filter); > if (IS_ERR(prepared)) > @@ -1302,7 +1315,7 @@ static long seccomp_set_mode_filter(unsigned int flags, > mutex_unlock(¤t->signal->cred_guard_mutex); > out_put_fd: > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > - if (ret < 0) { > + if (ret) { Why that change but keep checking if (ret < 0) further up? > listener_f->private_data = NULL; > fput(listener_f); > put_unused_fd(listener); > -- > 2.19.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags 2019-03-06 20:39 ` Christian Brauner @ 2019-03-06 20:46 ` Tycho Andersen 2019-03-06 21:02 ` Christian Brauner 0 siblings, 1 reply; 13+ messages in thread From: Tycho Andersen @ 2019-03-06 20:46 UTC (permalink / raw) To: Christian Brauner; +Cc: Kees Cook, linux-kernel, stable On Wed, Mar 06, 2019 at 09:39:35PM +0100, Christian Brauner wrote: > > + > > /* Prepare the new filter before holding any locks. */ > > prepared = seccomp_prepare_user_filter(filter); > > if (IS_ERR(prepared)) > > @@ -1302,7 +1315,7 @@ static long seccomp_set_mode_filter(unsigned int flags, > > mutex_unlock(¤t->signal->cred_guard_mutex); > > out_put_fd: > > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > > - if (ret < 0) { > > + if (ret) { > > Why that change but keep checking if (ret < 0) further up? Not sure what you mean here. The only other place I see that we check something is < 0 in that function is the return value of get_unused_fd_flags(), which looks right to me? Tycho ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags 2019-03-06 20:46 ` Tycho Andersen @ 2019-03-06 21:02 ` Christian Brauner 2019-03-06 21:30 ` Tycho Andersen 0 siblings, 1 reply; 13+ messages in thread From: Christian Brauner @ 2019-03-06 21:02 UTC (permalink / raw) To: Tycho Andersen; +Cc: Kees Cook, Linux Kernel Mailing List, stable On Wed, Mar 6, 2019 at 9:46 PM Tycho Andersen <tycho@tycho.ws> wrote: > > On Wed, Mar 06, 2019 at 09:39:35PM +0100, Christian Brauner wrote: > > > + > > > /* Prepare the new filter before holding any locks. */ > > > prepared = seccomp_prepare_user_filter(filter); > > > if (IS_ERR(prepared)) > > > @@ -1302,7 +1315,7 @@ static long seccomp_set_mode_filter(unsigned int flags, > > > mutex_unlock(¤t->signal->cred_guard_mutex); > > > out_put_fd: > > > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > > > - if (ret < 0) { > > > + if (ret) { > > > > Why that change but keep checking if (ret < 0) further up? > > Not sure what you mean here. The only other place I see that we check > something is < 0 in that function is the return value of > get_unused_fd_flags(), which looks right to me? The change just seemed it had nothing to do with the rest of the patch. Just making sure this didn't happen on accident and would cause regressions. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags 2019-03-06 21:02 ` Christian Brauner @ 2019-03-06 21:30 ` Tycho Andersen 0 siblings, 0 replies; 13+ messages in thread From: Tycho Andersen @ 2019-03-06 21:30 UTC (permalink / raw) To: Christian Brauner; +Cc: Kees Cook, Linux Kernel Mailing List, stable On Wed, Mar 06, 2019 at 10:02:25PM +0100, Christian Brauner wrote: > On Wed, Mar 6, 2019 at 9:46 PM Tycho Andersen <tycho@tycho.ws> wrote: > > > > On Wed, Mar 06, 2019 at 09:39:35PM +0100, Christian Brauner wrote: > > > > + > > > > /* Prepare the new filter before holding any locks. */ > > > > prepared = seccomp_prepare_user_filter(filter); > > > > if (IS_ERR(prepared)) > > > > @@ -1302,7 +1315,7 @@ static long seccomp_set_mode_filter(unsigned int flags, > > > > mutex_unlock(¤t->signal->cred_guard_mutex); > > > > out_put_fd: > > > > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > > > > - if (ret < 0) { > > > > + if (ret) { > > > > > > Why that change but keep checking if (ret < 0) further up? > > > > Not sure what you mean here. The only other place I see that we check > > something is < 0 in that function is the return value of > > get_unused_fd_flags(), which looks right to me? > > The change just seemed it had nothing to do with the rest of the patch. > Just making sure this didn't happen on accident and would cause regressions. No, not on accident :). See the second half of the patch notes. I can split it out into two separate patches if that makes more sense. In fact this hunk alone fixes the UAF, but you still get non-sensical return results even if it doesn't do anything terrible, hence the first hunk. Cheers, Tycho ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags 2019-03-06 20:14 ` [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags Tycho Andersen 2019-03-06 20:39 ` Christian Brauner @ 2019-04-23 22:09 ` Kees Cook 2019-04-23 23:18 ` James Morris 2019-04-23 23:31 ` Kees Cook 1 sibling, 2 replies; 13+ messages in thread From: Kees Cook @ 2019-04-23 22:09 UTC (permalink / raw) To: Tycho Andersen, James Morris; +Cc: LKML, # 3.4.x On Wed, Mar 6, 2019 at 12:14 PM Tycho Andersen <tycho@tycho.ws> wrote: > > As the comment notes, the return codes for TSYNC and NEW_LISTENER conflict, > because they both return positive values, one in the case of success and > one in the case of error. So, let's disallow both of these flags together. > > While this is technically a userspace break, all the users I know of are > still waiting on me to land this feature in libseccomp, so I think it'll be > safe. Also, at present my use case doesn't require TSYNC at all, so this > isn't a big deal to disallow. If someone wanted to support this, a path > forward would be to add a new flag like > TSYNC_AND_LISTENER_YES_I_UNDERSTAND_THAT_TSYNC_WILL_JUST_RETURN_EAGAIN, but > the use cases are so different I don't see it really happening. > > Finally, it's worth noting that this does actually fix a UAF issue: at the end > of seccomp_set_mode_filter(), we have: > > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > if (ret < 0) { > listener_f->private_data = NULL; > fput(listener_f); > put_unused_fd(listener); > } else { > fd_install(listener, listener_f); > ret = listener; > } > } > out_free: > seccomp_filter_free(prepared); > > But if ret > 0 because TSYNC raced, we'll install the listener fd and then free > the filter out from underneath it, causing a UAF when the task closes it or > dies. This patch also switches the condition to be simply if (ret), so that > if someone does add the flag mentioned above, they won't have to remember > to fix this too. > > Signed-off-by: Tycho Andersen <tycho@tycho.ws> > Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") > CC: stable@vger.kernel.org # v5.0+ Thanks! Sorry I missed this. James, can you take this for Linus's fixes for v5.1? (Or should I send a pull request to you?) Acked-by: Kees Cook <keescook@chromium.org> Let's also add: Reported-by: syzbot+b562969adb2e04af3442@syzkaller.appspotmail.com > --- > kernel/seccomp.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index d0d355ded2f4..79bada51091b 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -500,7 +500,10 @@ seccomp_prepare_user_filter(const char __user *user_filter) > * > * Caller must be holding current->sighand->siglock lock. > * > - * Returns 0 on success, -ve on error. > + * Returns 0 on success, -ve on error, or > + * - in TSYNC mode: the pid of a thread which was either not in the correct > + * seccomp mode or did not have an ancestral seccomp filter > + * - in NEW_LISTENER mode: the fd of the new listener > */ > static long seccomp_attach_filter(unsigned int flags, > struct seccomp_filter *filter) > @@ -1256,6 +1259,16 @@ static long seccomp_set_mode_filter(unsigned int flags, > if (flags & ~SECCOMP_FILTER_FLAG_MASK) > return -EINVAL; > > + /* > + * In the successful case, NEW_LISTENER returns the new listener fd. > + * But in the failure case, TSYNC returns the thread that died. If you > + * combine these two flags, there's no way to tell whether something > + * succeded or failed. So, let's disallow this combination. also a tiny typo: succeeded > + */ > + if ((flags & SECCOMP_FILTER_FLAG_TSYNC) && > + (flags && SECCOMP_FILTER_FLAG_NEW_LISTENER)) > + return -EINVAL; > + > /* Prepare the new filter before holding any locks. */ > prepared = seccomp_prepare_user_filter(filter); > if (IS_ERR(prepared)) > @@ -1302,7 +1315,7 @@ static long seccomp_set_mode_filter(unsigned int flags, > mutex_unlock(¤t->signal->cred_guard_mutex); > out_put_fd: > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > - if (ret < 0) { > + if (ret) { > listener_f->private_data = NULL; > fput(listener_f); > put_unused_fd(listener); > -- > 2.19.1 > -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags 2019-04-23 22:09 ` Kees Cook @ 2019-04-23 23:18 ` James Morris 2019-04-23 23:31 ` Kees Cook 1 sibling, 0 replies; 13+ messages in thread From: James Morris @ 2019-04-23 23:18 UTC (permalink / raw) To: Kees Cook; +Cc: Tycho Andersen, LKML, # 3.4.x On Tue, 23 Apr 2019, Kees Cook wrote: > Thanks! Sorry I missed this. James, can you take this for Linus's > fixes for v5.1? (Or should I send a pull request to you?) > > Acked-by: Kees Cook <keescook@chromium.org> These are standalone for v5.1 fixes currently so you can send them directly to Linus. > > Let's also add: > > Reported-by: syzbot+b562969adb2e04af3442@syzkaller.appspotmail.com > > > --- > > kernel/seccomp.c | 17 +++++++++++++++-- > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > > index d0d355ded2f4..79bada51091b 100644 > > --- a/kernel/seccomp.c > > +++ b/kernel/seccomp.c > > @@ -500,7 +500,10 @@ seccomp_prepare_user_filter(const char __user *user_filter) > > * > > * Caller must be holding current->sighand->siglock lock. > > * > > - * Returns 0 on success, -ve on error. > > + * Returns 0 on success, -ve on error, or > > + * - in TSYNC mode: the pid of a thread which was either not in the correct > > + * seccomp mode or did not have an ancestral seccomp filter > > + * - in NEW_LISTENER mode: the fd of the new listener > > */ > > static long seccomp_attach_filter(unsigned int flags, > > struct seccomp_filter *filter) > > @@ -1256,6 +1259,16 @@ static long seccomp_set_mode_filter(unsigned int flags, > > if (flags & ~SECCOMP_FILTER_FLAG_MASK) > > return -EINVAL; > > > > + /* > > + * In the successful case, NEW_LISTENER returns the new listener fd. > > + * But in the failure case, TSYNC returns the thread that died. If you > > + * combine these two flags, there's no way to tell whether something > > + * succeded or failed. So, let's disallow this combination. > > also a tiny typo: succeeded > > > + */ > > + if ((flags & SECCOMP_FILTER_FLAG_TSYNC) && > > + (flags && SECCOMP_FILTER_FLAG_NEW_LISTENER)) > > + return -EINVAL; > > + > > /* Prepare the new filter before holding any locks. */ > > prepared = seccomp_prepare_user_filter(filter); > > if (IS_ERR(prepared)) > > @@ -1302,7 +1315,7 @@ static long seccomp_set_mode_filter(unsigned int flags, > > mutex_unlock(¤t->signal->cred_guard_mutex); > > out_put_fd: > > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > > - if (ret < 0) { > > + if (ret) { > > listener_f->private_data = NULL; > > fput(listener_f); > > put_unused_fd(listener); > > -- > > 2.19.1 > > > > -Kees > > -- James Morris <jmorris@namei.org> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags 2019-04-23 22:09 ` Kees Cook 2019-04-23 23:18 ` James Morris @ 2019-04-23 23:31 ` Kees Cook 2019-04-23 23:34 ` Tycho Andersen 1 sibling, 1 reply; 13+ messages in thread From: Kees Cook @ 2019-04-23 23:31 UTC (permalink / raw) To: Tycho Andersen, James Morris; +Cc: LKML, # 3.4.x On Tue, Apr 23, 2019 at 3:09 PM Kees Cook <keescook@chromium.org> wrote: > > On Wed, Mar 6, 2019 at 12:14 PM Tycho Andersen <tycho@tycho.ws> wrote: > > > > As the comment notes, the return codes for TSYNC and NEW_LISTENER conflict, > > because they both return positive values, one in the case of success and > > one in the case of error. So, let's disallow both of these flags together. > > > > While this is technically a userspace break, all the users I know of are > > still waiting on me to land this feature in libseccomp, so I think it'll be > > safe. Also, at present my use case doesn't require TSYNC at all, so this > > isn't a big deal to disallow. If someone wanted to support this, a path > > forward would be to add a new flag like > > TSYNC_AND_LISTENER_YES_I_UNDERSTAND_THAT_TSYNC_WILL_JUST_RETURN_EAGAIN, but > > the use cases are so different I don't see it really happening. > > > > Finally, it's worth noting that this does actually fix a UAF issue: at the end > > of seccomp_set_mode_filter(), we have: > > > > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > > if (ret < 0) { > > listener_f->private_data = NULL; > > fput(listener_f); > > put_unused_fd(listener); > > } else { > > fd_install(listener, listener_f); > > ret = listener; > > } > > } > > out_free: > > seccomp_filter_free(prepared); > > > > But if ret > 0 because TSYNC raced, we'll install the listener fd and then free > > the filter out from underneath it, causing a UAF when the task closes it or > > dies. This patch also switches the condition to be simply if (ret), so that > > if someone does add the flag mentioned above, they won't have to remember > > to fix this too. > > > > Signed-off-by: Tycho Andersen <tycho@tycho.ws> > > Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") > > CC: stable@vger.kernel.org # v5.0+ > > Thanks! Sorry I missed this. James, can you take this for Linus's > fixes for v5.1? (Or should I send a pull request to you?) > > Acked-by: Kees Cook <keescook@chromium.org> > > Let's also add: > > Reported-by: syzbot+b562969adb2e04af3442@syzkaller.appspotmail.com > > > --- > > kernel/seccomp.c | 17 +++++++++++++++-- > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > > index d0d355ded2f4..79bada51091b 100644 > > --- a/kernel/seccomp.c > > +++ b/kernel/seccomp.c > > @@ -500,7 +500,10 @@ seccomp_prepare_user_filter(const char __user *user_filter) > > * > > * Caller must be holding current->sighand->siglock lock. > > * > > - * Returns 0 on success, -ve on error. > > + * Returns 0 on success, -ve on error, or > > + * - in TSYNC mode: the pid of a thread which was either not in the correct > > + * seccomp mode or did not have an ancestral seccomp filter > > + * - in NEW_LISTENER mode: the fd of the new listener > > */ > > static long seccomp_attach_filter(unsigned int flags, > > struct seccomp_filter *filter) > > @@ -1256,6 +1259,16 @@ static long seccomp_set_mode_filter(unsigned int flags, > > if (flags & ~SECCOMP_FILTER_FLAG_MASK) > > return -EINVAL; > > > > + /* > > + * In the successful case, NEW_LISTENER returns the new listener fd. > > + * But in the failure case, TSYNC returns the thread that died. If you > > + * combine these two flags, there's no way to tell whether something > > + * succeded or failed. So, let's disallow this combination. > > also a tiny typo: succeeded > > > + */ > > + if ((flags & SECCOMP_FILTER_FLAG_TSYNC) && > > + (flags && SECCOMP_FILTER_FLAG_NEW_LISTENER)) also a typo: && should be & > > + return -EINVAL; > > + > > /* Prepare the new filter before holding any locks. */ > > prepared = seccomp_prepare_user_filter(filter); > > if (IS_ERR(prepared)) > > @@ -1302,7 +1315,7 @@ static long seccomp_set_mode_filter(unsigned int flags, > > mutex_unlock(¤t->signal->cred_guard_mutex); > > out_put_fd: > > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > > - if (ret < 0) { > > + if (ret) { > > listener_f->private_data = NULL; > > fput(listener_f); > > put_unused_fd(listener); > > -- > > 2.19.1 > > > > -Kees > > -- > Kees Cook -- Kees Cook ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags 2019-04-23 23:31 ` Kees Cook @ 2019-04-23 23:34 ` Tycho Andersen 2019-04-24 1:02 ` Kees Cook 0 siblings, 1 reply; 13+ messages in thread From: Tycho Andersen @ 2019-04-23 23:34 UTC (permalink / raw) To: Kees Cook; +Cc: James Morris, LKML, # 3.4.x On Tue, Apr 23, 2019 at 04:31:45PM -0700, Kees Cook wrote: > On Tue, Apr 23, 2019 at 3:09 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Wed, Mar 6, 2019 at 12:14 PM Tycho Andersen <tycho@tycho.ws> wrote: > > > > > > As the comment notes, the return codes for TSYNC and NEW_LISTENER conflict, > > > because they both return positive values, one in the case of success and > > > one in the case of error. So, let's disallow both of these flags together. > > > > > > While this is technically a userspace break, all the users I know of are > > > still waiting on me to land this feature in libseccomp, so I think it'll be > > > safe. Also, at present my use case doesn't require TSYNC at all, so this > > > isn't a big deal to disallow. If someone wanted to support this, a path > > > forward would be to add a new flag like > > > TSYNC_AND_LISTENER_YES_I_UNDERSTAND_THAT_TSYNC_WILL_JUST_RETURN_EAGAIN, but > > > the use cases are so different I don't see it really happening. > > > > > > Finally, it's worth noting that this does actually fix a UAF issue: at the end > > > of seccomp_set_mode_filter(), we have: > > > > > > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > > > if (ret < 0) { > > > listener_f->private_data = NULL; > > > fput(listener_f); > > > put_unused_fd(listener); > > > } else { > > > fd_install(listener, listener_f); > > > ret = listener; > > > } > > > } > > > out_free: > > > seccomp_filter_free(prepared); > > > > > > But if ret > 0 because TSYNC raced, we'll install the listener fd and then free > > > the filter out from underneath it, causing a UAF when the task closes it or > > > dies. This patch also switches the condition to be simply if (ret), so that > > > if someone does add the flag mentioned above, they won't have to remember > > > to fix this too. > > > > > > Signed-off-by: Tycho Andersen <tycho@tycho.ws> > > > Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") > > > CC: stable@vger.kernel.org # v5.0+ > > > > Thanks! Sorry I missed this. James, can you take this for Linus's > > fixes for v5.1? (Or should I send a pull request to you?) > > > > Acked-by: Kees Cook <keescook@chromium.org> > > > > Let's also add: > > > > Reported-by: syzbot+b562969adb2e04af3442@syzkaller.appspotmail.com > > > > > --- > > > kernel/seccomp.c | 17 +++++++++++++++-- > > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > > > index d0d355ded2f4..79bada51091b 100644 > > > --- a/kernel/seccomp.c > > > +++ b/kernel/seccomp.c > > > @@ -500,7 +500,10 @@ seccomp_prepare_user_filter(const char __user *user_filter) > > > * > > > * Caller must be holding current->sighand->siglock lock. > > > * > > > - * Returns 0 on success, -ve on error. > > > + * Returns 0 on success, -ve on error, or > > > + * - in TSYNC mode: the pid of a thread which was either not in the correct > > > + * seccomp mode or did not have an ancestral seccomp filter > > > + * - in NEW_LISTENER mode: the fd of the new listener > > > */ > > > static long seccomp_attach_filter(unsigned int flags, > > > struct seccomp_filter *filter) > > > @@ -1256,6 +1259,16 @@ static long seccomp_set_mode_filter(unsigned int flags, > > > if (flags & ~SECCOMP_FILTER_FLAG_MASK) > > > return -EINVAL; > > > > > > + /* > > > + * In the successful case, NEW_LISTENER returns the new listener fd. > > > + * But in the failure case, TSYNC returns the thread that died. If you > > > + * combine these two flags, there's no way to tell whether something > > > + * succeded or failed. So, let's disallow this combination. > > > > also a tiny typo: succeeded > > > > > + */ > > > + if ((flags & SECCOMP_FILTER_FLAG_TSYNC) && > > > + (flags && SECCOMP_FILTER_FLAG_NEW_LISTENER)) > > also a typo: && should be & Oh, yes. Do you want me to send another version? Tycho ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags 2019-04-23 23:34 ` Tycho Andersen @ 2019-04-24 1:02 ` Kees Cook 0 siblings, 0 replies; 13+ messages in thread From: Kees Cook @ 2019-04-24 1:02 UTC (permalink / raw) To: Tycho Andersen; +Cc: James Morris, LKML, # 3.4.x On Tue, Apr 23, 2019 at 4:34 PM Tycho Andersen <tycho@tycho.ws> wrote: > > On Tue, Apr 23, 2019 at 04:31:45PM -0700, Kees Cook wrote: > > On Tue, Apr 23, 2019 at 3:09 PM Kees Cook <keescook@chromium.org> wrote: > > > > > > On Wed, Mar 6, 2019 at 12:14 PM Tycho Andersen <tycho@tycho.ws> wrote: > > > > > > > > As the comment notes, the return codes for TSYNC and NEW_LISTENER conflict, > > > > because they both return positive values, one in the case of success and > > > > one in the case of error. So, let's disallow both of these flags together. > > > > > > > > While this is technically a userspace break, all the users I know of are > > > > still waiting on me to land this feature in libseccomp, so I think it'll be > > > > safe. Also, at present my use case doesn't require TSYNC at all, so this > > > > isn't a big deal to disallow. If someone wanted to support this, a path > > > > forward would be to add a new flag like > > > > TSYNC_AND_LISTENER_YES_I_UNDERSTAND_THAT_TSYNC_WILL_JUST_RETURN_EAGAIN, but > > > > the use cases are so different I don't see it really happening. > > > > > > > > Finally, it's worth noting that this does actually fix a UAF issue: at the end > > > > of seccomp_set_mode_filter(), we have: > > > > > > > > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > > > > if (ret < 0) { > > > > listener_f->private_data = NULL; > > > > fput(listener_f); > > > > put_unused_fd(listener); > > > > } else { > > > > fd_install(listener, listener_f); > > > > ret = listener; > > > > } > > > > } > > > > out_free: > > > > seccomp_filter_free(prepared); > > > > > > > > But if ret > 0 because TSYNC raced, we'll install the listener fd and then free > > > > the filter out from underneath it, causing a UAF when the task closes it or > > > > dies. This patch also switches the condition to be simply if (ret), so that > > > > if someone does add the flag mentioned above, they won't have to remember > > > > to fix this too. > > > > > > > > Signed-off-by: Tycho Andersen <tycho@tycho.ws> > > > > Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") > > > > CC: stable@vger.kernel.org # v5.0+ > > > > > > Thanks! Sorry I missed this. James, can you take this for Linus's > > > fixes for v5.1? (Or should I send a pull request to you?) > > > > > > Acked-by: Kees Cook <keescook@chromium.org> > > > > > > Let's also add: > > > > > > Reported-by: syzbot+b562969adb2e04af3442@syzkaller.appspotmail.com > > > > > > > --- > > > > kernel/seccomp.c | 17 +++++++++++++++-- > > > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > > > > index d0d355ded2f4..79bada51091b 100644 > > > > --- a/kernel/seccomp.c > > > > +++ b/kernel/seccomp.c > > > > @@ -500,7 +500,10 @@ seccomp_prepare_user_filter(const char __user *user_filter) > > > > * > > > > * Caller must be holding current->sighand->siglock lock. > > > > * > > > > - * Returns 0 on success, -ve on error. > > > > + * Returns 0 on success, -ve on error, or > > > > + * - in TSYNC mode: the pid of a thread which was either not in the correct > > > > + * seccomp mode or did not have an ancestral seccomp filter > > > > + * - in NEW_LISTENER mode: the fd of the new listener > > > > */ > > > > static long seccomp_attach_filter(unsigned int flags, > > > > struct seccomp_filter *filter) > > > > @@ -1256,6 +1259,16 @@ static long seccomp_set_mode_filter(unsigned int flags, > > > > if (flags & ~SECCOMP_FILTER_FLAG_MASK) > > > > return -EINVAL; > > > > > > > > + /* > > > > + * In the successful case, NEW_LISTENER returns the new listener fd. > > > > + * But in the failure case, TSYNC returns the thread that died. If you > > > > + * combine these two flags, there's no way to tell whether something > > > > + * succeded or failed. So, let's disallow this combination. > > > > > > also a tiny typo: succeeded > > > > > > > + */ > > > > + if ((flags & SECCOMP_FILTER_FLAG_TSYNC) && > > > > + (flags && SECCOMP_FILTER_FLAG_NEW_LISTENER)) > > > > also a typo: && should be & > > Oh, yes. Do you want me to send another version? Nah, I fixed it up. :) -- Kees Cook ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] seccomp: fix up grammar in comment 2019-03-06 20:14 [PATCH 1/2] seccomp: fix up grammar in comment Tycho Andersen 2019-03-06 20:14 ` [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags Tycho Andersen @ 2019-04-23 22:11 ` Kees Cook 2019-04-23 23:24 ` James Morris 1 sibling, 1 reply; 13+ messages in thread From: Kees Cook @ 2019-04-23 22:11 UTC (permalink / raw) To: Tycho Andersen, James Morris; +Cc: LKML On Wed, Mar 6, 2019 at 12:14 PM Tycho Andersen <tycho@tycho.ws> wrote: > > This sentence is kind of a train wreck anyway, but at least dropping the > extra pronoun helps somewhat. > > Signed-off-by: Tycho Andersen <tycho@tycho.ws> Acked-by: Kees Cook <keescook@chromium.org> James, this can go in for v5.2 -- no rush. -Kees > --- > kernel/seccomp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index e815781ed751..d0d355ded2f4 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -329,7 +329,7 @@ static int is_ancestor(struct seccomp_filter *parent, > * Expects sighand and cred_guard_mutex locks to be held. > * > * Returns 0 on success, -ve on error, or the pid of a thread which was > - * either not in the correct seccomp mode or it did not have an ancestral > + * either not in the correct seccomp mode or did not have an ancestral > * seccomp filter. > */ > static inline pid_t seccomp_can_sync_threads(void) > -- > 2.19.1 > -- Kees Cook ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] seccomp: fix up grammar in comment 2019-04-23 22:11 ` [PATCH 1/2] seccomp: fix up grammar in comment Kees Cook @ 2019-04-23 23:24 ` James Morris 0 siblings, 0 replies; 13+ messages in thread From: James Morris @ 2019-04-23 23:24 UTC (permalink / raw) To: Kees Cook; +Cc: Tycho Andersen, LKML On Tue, 23 Apr 2019, Kees Cook wrote: > On Wed, Mar 6, 2019 at 12:14 PM Tycho Andersen <tycho@tycho.ws> wrote: > > > > This sentence is kind of a train wreck anyway, but at least dropping the > > extra pronoun helps somewhat. > > > > Signed-off-by: Tycho Andersen <tycho@tycho.ws> > > Acked-by: Kees Cook <keescook@chromium.org> > > James, this can go in for v5.2 -- no rush. > Applied to git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general -- James Morris <jmorris@namei.org> ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-04-24 1:02 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-06 20:14 [PATCH 1/2] seccomp: fix up grammar in comment Tycho Andersen 2019-03-06 20:14 ` [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags Tycho Andersen 2019-03-06 20:39 ` Christian Brauner 2019-03-06 20:46 ` Tycho Andersen 2019-03-06 21:02 ` Christian Brauner 2019-03-06 21:30 ` Tycho Andersen 2019-04-23 22:09 ` Kees Cook 2019-04-23 23:18 ` James Morris 2019-04-23 23:31 ` Kees Cook 2019-04-23 23:34 ` Tycho Andersen 2019-04-24 1:02 ` Kees Cook 2019-04-23 22:11 ` [PATCH 1/2] seccomp: fix up grammar in comment Kees Cook 2019-04-23 23:24 ` James Morris
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).