This is just sending out the tweaked fix from Tycho and the selftest changes needed to support it. I intend to send this to Linus directly after it's been in -next for a few days for v5.1 fixes.
Thanks!
-Kees
Kees Cook (1): selftests/seccomp: Prepare for exclusive seccomp flags
Tycho Andersen (1): seccomp: Make NEW_LISTENER and TSYNC flags exclusive
kernel/seccomp.c | 17 ++++++++-- tools/testing/selftests/seccomp/seccomp_bpf.c | 34 ++++++++++++++----- 2 files changed, 40 insertions(+), 11 deletions(-)
Some seccomp flags will become exclusive, so the selftest needs to be adjusted to mask those out and test them individually for the "all flags" tests.
Cc: stable@vger.kernel.org # v5.0+ Signed-off-by: Kees Cook keescook@chromium.org --- tools/testing/selftests/seccomp/seccomp_bpf.c | 34 ++++++++++++++----- 1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index f69d2ee29742..5019cdae5d0b 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -2166,11 +2166,14 @@ TEST(detect_seccomp_filter_flags) SECCOMP_FILTER_FLAG_LOG, SECCOMP_FILTER_FLAG_SPEC_ALLOW, SECCOMP_FILTER_FLAG_NEW_LISTENER }; - unsigned int flag, all_flags; + unsigned int exclusive[] = { + SECCOMP_FILTER_FLAG_TSYNC, + SECCOMP_FILTER_FLAG_NEW_LISTENER }; + unsigned int flag, all_flags, exclusive_mask; int i; long ret;
- /* Test detection of known-good filter flags */ + /* Test detection of individual known-good filter flags */ for (i = 0, all_flags = 0; i < ARRAY_SIZE(flags); i++) { int bits = 0;
@@ -2197,16 +2200,29 @@ TEST(detect_seccomp_filter_flags) all_flags |= flag; }
- /* Test detection of all known-good filter flags */ - ret = seccomp(SECCOMP_SET_MODE_FILTER, all_flags, NULL); - EXPECT_EQ(-1, ret); - EXPECT_EQ(EFAULT, errno) { - TH_LOG("Failed to detect that all known-good filter flags (0x%X) are supported!", - all_flags); + /* + * Test detection of all known-good filter flags combined. But + * for the exclusive flags we need to mask them out and try them + * individually for the "all flags" testing. + */ + exclusive_mask = 0; + for (i = 0; i < ARRAY_SIZE(exclusive); i++) + exclusive_mask |= exclusive[i]; + for (i = 0; i < ARRAY_SIZE(exclusive); i++) { + flag = all_flags & ~exclusive_mask; + flag |= exclusive[i]; + + ret = seccomp(SECCOMP_SET_MODE_FILTER, flag, NULL); + EXPECT_EQ(-1, ret); + EXPECT_EQ(EFAULT, errno) { + TH_LOG("Failed to detect that all known-good filter flags (0x%X) are supported!", + flag); + } }
- /* Test detection of an unknown filter flag */ + /* Test detection of an unknown filter flags, without exclusives. */ flag = -1; + flag &= ~exclusive_mask; ret = seccomp(SECCOMP_SET_MODE_FILTER, flag, NULL); EXPECT_EQ(-1, ret); EXPECT_EQ(EINVAL, errno) {
On Wed, Apr 24, 2019 at 09:37:55AM -0700, Kees Cook wrote:
Some seccomp flags will become exclusive, so the selftest needs to be adjusted to mask those out and test them individually for the "all flags" tests.
Cc: stable@vger.kernel.org # v5.0+ Signed-off-by: Kees Cook keescook@chromium.org
Whoops, thanks for this too.
Reviewed-by: Tycho Andersen tycho@tycho.ws
On Wed, 24 Apr 2019, Kees Cook wrote:
Some seccomp flags will become exclusive, so the selftest needs to be adjusted to mask those out and test them individually for the "all flags" tests.
Cc: stable@vger.kernel.org # v5.0+ Signed-off-by: Kees Cook keescook@chromium.org
Acked-by: James Morris jamorris@linux.microsoft.com
tools/testing/selftests/seccomp/seccomp_bpf.c | 34 ++++++++++++++----- 1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index f69d2ee29742..5019cdae5d0b 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -2166,11 +2166,14 @@ TEST(detect_seccomp_filter_flags) SECCOMP_FILTER_FLAG_LOG, SECCOMP_FILTER_FLAG_SPEC_ALLOW, SECCOMP_FILTER_FLAG_NEW_LISTENER };
- unsigned int flag, all_flags;
- unsigned int exclusive[] = {
SECCOMP_FILTER_FLAG_TSYNC,SECCOMP_FILTER_FLAG_NEW_LISTENER };- unsigned int flag, all_flags, exclusive_mask; int i; long ret;
- /* Test detection of known-good filter flags */
- /* Test detection of individual known-good filter flags */ for (i = 0, all_flags = 0; i < ARRAY_SIZE(flags); i++) { int bits = 0;
@@ -2197,16 +2200,29 @@ TEST(detect_seccomp_filter_flags) all_flags |= flag; }
- /* Test detection of all known-good filter flags */
- ret = seccomp(SECCOMP_SET_MODE_FILTER, all_flags, NULL);
- EXPECT_EQ(-1, ret);
- EXPECT_EQ(EFAULT, errno) {
TH_LOG("Failed to detect that all known-good filter flags (0x%X) are supported!",all_flags);
- /*
* Test detection of all known-good filter flags combined. But* for the exclusive flags we need to mask them out and try them* individually for the "all flags" testing.*/- exclusive_mask = 0;
- for (i = 0; i < ARRAY_SIZE(exclusive); i++)
exclusive_mask |= exclusive[i];- for (i = 0; i < ARRAY_SIZE(exclusive); i++) {
flag = all_flags & ~exclusive_mask;flag |= exclusive[i];ret = seccomp(SECCOMP_SET_MODE_FILTER, flag, NULL);EXPECT_EQ(-1, ret);EXPECT_EQ(EFAULT, errno) {TH_LOG("Failed to detect that all known-good filter flags (0x%X) are supported!",flag);}
- /* Test detection of an unknown filter flag */
- /* Test detection of an unknown filter flags, without exclusives. */ flag = -1;
- flag &= ~exclusive_mask; ret = seccomp(SECCOMP_SET_MODE_FILTER, flag, NULL); EXPECT_EQ(-1, ret); EXPECT_EQ(EINVAL, errno) {
From: Tycho Andersen tycho@tycho.ws
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 --- kernel/seccomp.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c index df27e499956a..3582eeb59893 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -502,7 +502,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) @@ -1258,6 +1261,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 + * 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)) @@ -1304,7 +1317,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);
On Wed, 24 Apr 2019, Kees Cook wrote:
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
linux-kselftest-mirror@lists.linaro.org
-
James Morris -
Kees Cook -
Tycho Andersen