| From 7a0df7fbc14505e2e2be19ed08654a09e1ed5bf6 Mon Sep 17 00:00:00 2001 |
| From: Tycho Andersen <tycho@tycho.ws> |
| Date: Wed, 6 Mar 2019 13:14:13 -0700 |
| Subject: seccomp: Make NEW_LISTENER and TSYNC flags exclusive |
| |
| From: Tycho Andersen <tycho@tycho.ws> |
| |
| commit 7a0df7fbc14505e2e2be19ed08654a09e1ed5bf6 upstream. |
| |
| 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. |
| |
| Reported-by: syzbot+b562969adb2e04af3442@syzkaller.appspotmail.com |
| Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") |
| CC: stable@vger.kernel.org # v5.0+ |
| Signed-off-by: Tycho Andersen <tycho@tycho.ws> |
| Signed-off-by: Kees Cook <keescook@chromium.org> |
| Acked-by: James Morris <jamorris@linux.microsoft.com> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| kernel/seccomp.c | 17 +++++++++++++++-- |
| 1 file changed, 15 insertions(+), 2 deletions(-) |
| |
| --- a/kernel/seccomp.c |
| +++ b/kernel/seccomp.c |
| @@ -500,7 +500,10 @@ out: |
| * |
| * 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(unsi |
| 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 |
| + * succeeded 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 @@ out: |
| 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); |