Hi,
This expands the seccomp selftest to poke a architectural behavior corner that Keno Fischer noticed[1]. In the process, I took the opportunity to do the kselftest harness variant refactoring I'd been meaning to do, which made adding this test much nicer.
I'd prefer this went via the seccomp tree, as it builds on top of the other recent seccomp feature addition tests. Testing and reviews are welcome! :)
Thanks,
-Kees
[1] https://lore.kernel.org/lkml/CABV8kRxA9mXPZwtYrjbAfOfFewhABHddipccgk-LQJO+ZY...
Kees Cook (3): selftests/harness: Clean up kern-doc for fixtures selftests/seccomp: Refactor to use fixture variants selftests/seccomp: Check ENOSYS under tracing
tools/testing/selftests/kselftest_harness.h | 15 +- tools/testing/selftests/seccomp/seccomp_bpf.c | 217 ++++++------------ 2 files changed, 72 insertions(+), 160 deletions(-)
The FIXTURE*() macro kern-doc examples had the wrong names for the C code examples associated with them. Fix those and clarify that FIXTURE_DATA() usage should be avoided.
Cc: Shuah Khan shuah@kernel.org Cc: Jakub Kicinski kuba@kernel.org Fixes: 74bc7c97fa88 ("kselftest: add fixture variants") Signed-off-by: Kees Cook keescook@chromium.org --- tools/testing/selftests/kselftest_harness.h | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index c9f03ef93338..7f32a7099a81 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -195,8 +195,9 @@ * * .. code-block:: c * - * FIXTURE_DATA(datatype name) + * FIXTURE_DATA(datatype_name) * + * Almost always, you want just FIXTURE() instead (see below). * This call may be used when the type of the fixture data * is needed. In general, this should not be needed unless * the *self* is being passed to a helper directly. @@ -211,7 +212,7 @@ * * .. code-block:: c * - * FIXTURE(datatype name) { + * FIXTURE(fixture_name) { * type property1; * ... * }; @@ -238,7 +239,7 @@ * * .. code-block:: c * - * FIXTURE_SETUP(fixture name) { implementation } + * FIXTURE_SETUP(fixture_name) { implementation } * * Populates the required "setup" function for a fixture. An instance of the * datatype defined with FIXTURE_DATA() will be exposed as *self* for the @@ -264,7 +265,7 @@ * * .. code-block:: c * - * FIXTURE_TEARDOWN(fixture name) { implementation } + * FIXTURE_TEARDOWN(fixture_name) { implementation } * * Populates the required "teardown" function for a fixture. An instance of the * datatype defined with FIXTURE_DATA() will be exposed as *self* for the @@ -285,7 +286,7 @@ * * .. code-block:: c * - * FIXTURE_VARIANT(datatype name) { + * FIXTURE_VARIANT(fixture_name) { * type property1; * ... * }; @@ -305,8 +306,8 @@ * * .. code-block:: c * - * FIXTURE_ADD(datatype name) { - * .property1 = val1; + * FIXTURE_VARIANT_ADD(fixture_name, variant_name) { + * .property1 = val1, * ... * }; *
On Sat, 4 Jul 2020 23:12:30 -0700 Kees Cook wrote:
The FIXTURE*() macro kern-doc examples had the wrong names for the C code examples associated with them. Fix those and clarify that FIXTURE_DATA() usage should be avoided.
Cc: Shuah Khan shuah@kernel.org Cc: Jakub Kicinski kuba@kernel.org Fixes: 74bc7c97fa88 ("kselftest: add fixture variants") Signed-off-by: Kees Cook keescook@chromium.org
Reviewed-by: Jakub Kicinski kuba@kernel.org
Now that the selftest harness has variants, use them to eliminate a bunch of copy/paste duplication.
Cc: Andy Lutomirski luto@amacapital.net Cc: Will Drewry wad@chromium.org Cc: Jakub Kicinski kuba@kernel.org Signed-off-by: Kees Cook keescook@chromium.org --- tools/testing/selftests/seccomp/seccomp_bpf.c | 199 ++++-------------- 1 file changed, 42 insertions(+), 157 deletions(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 6439c031a85d..966dec340ea8 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -1541,6 +1541,7 @@ pid_t setup_trace_fixture(struct __test_metadata *_metadata,
return tracer_pid; } + void teardown_trace_fixture(struct __test_metadata *_metadata, pid_t tracer) { @@ -1820,7 +1821,7 @@ void change_syscall(struct __test_metadata *_metadata, EXPECT_EQ(0, ret); }
-void tracer_syscall(struct __test_metadata *_metadata, pid_t tracee, +void tracer_seccomp(struct __test_metadata *_metadata, pid_t tracee, int status, void *args) { int ret; @@ -1897,6 +1898,24 @@ FIXTURE(TRACE_syscall) { pid_t tracer, mytid, mypid, parent; };
+FIXTURE_VARIANT(TRACE_syscall) { + /* + * All of the SECCOMP_RET_TRACE behaviors can be tested with either + * SECCOMP_RET_TRACE+PTRACE_CONT or plain ptrace()+PTRACE_SYSCALL. + * This indicates if we should use SECCOMP_RET_TRACE (false), or + * ptrace (true). + */ + bool use_ptrace; +}; + +FIXTURE_VARIANT_ADD(TRACE_syscall, ptrace) { + .use_ptrace = true, +}; + +FIXTURE_VARIANT_ADD(TRACE_syscall, seccomp) { + .use_ptrace = false, +}; + FIXTURE_SETUP(TRACE_syscall) { struct sock_filter filter[] = { @@ -1912,12 +1931,11 @@ FIXTURE_SETUP(TRACE_syscall) BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1005), BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW), }; - - memset(&self->prog, 0, sizeof(self->prog)); - self->prog.filter = malloc(sizeof(filter)); - ASSERT_NE(NULL, self->prog.filter); - memcpy(self->prog.filter, filter, sizeof(filter)); - self->prog.len = (unsigned short)ARRAY_SIZE(filter); + struct sock_fprog prog = { + .len = (unsigned short)ARRAY_SIZE(filter), + .filter = filter, + }; + long ret;
/* Prepare some testable syscall results. */ self->mytid = syscall(__NR_gettid); @@ -1935,60 +1953,28 @@ FIXTURE_SETUP(TRACE_syscall) ASSERT_NE(self->parent, self->mypid);
/* Launch tracer. */ - self->tracer = setup_trace_fixture(_metadata, tracer_syscall, NULL, - false); -} - -FIXTURE_TEARDOWN(TRACE_syscall) -{ - teardown_trace_fixture(_metadata, self->tracer); - if (self->prog.filter) - free(self->prog.filter); -} + self->tracer = setup_trace_fixture(_metadata, + variant->use_ptrace ? tracer_ptrace + : tracer_seccomp, + NULL, variant->use_ptrace);
-TEST_F(TRACE_syscall, ptrace_syscall_redirected) -{ - /* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */ - teardown_trace_fixture(_metadata, self->tracer); - self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL, - true); - - /* Tracer will redirect getpid to getppid. */ - EXPECT_NE(self->mypid, syscall(__NR_getpid)); -} + ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); + ASSERT_EQ(0, ret);
-TEST_F(TRACE_syscall, ptrace_syscall_errno) -{ - /* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */ - teardown_trace_fixture(_metadata, self->tracer); - self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL, - true); + if (variant->use_ptrace) + return;
- /* Tracer should skip the open syscall, resulting in ESRCH. */ - EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat)); + ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog, 0, 0); + ASSERT_EQ(0, ret); }
-TEST_F(TRACE_syscall, ptrace_syscall_faked) +FIXTURE_TEARDOWN(TRACE_syscall) { - /* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */ teardown_trace_fixture(_metadata, self->tracer); - self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL, - true); - - /* Tracer should skip the gettid syscall, resulting fake pid. */ - EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid)); }
TEST_F(TRACE_syscall, syscall_allowed) { - long ret; - - ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); - ASSERT_EQ(0, ret); - - ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->prog, 0, 0); - ASSERT_EQ(0, ret); - /* getppid works as expected (no changes). */ EXPECT_EQ(self->parent, syscall(__NR_getppid)); EXPECT_NE(self->mypid, syscall(__NR_getppid)); @@ -1996,14 +1982,6 @@ TEST_F(TRACE_syscall, syscall_allowed)
TEST_F(TRACE_syscall, syscall_redirected) { - long ret; - - ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); - ASSERT_EQ(0, ret); - - ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->prog, 0, 0); - ASSERT_EQ(0, ret); - /* getpid has been redirected to getppid as expected. */ EXPECT_EQ(self->parent, syscall(__NR_getpid)); EXPECT_NE(self->mypid, syscall(__NR_getpid)); @@ -2011,33 +1989,17 @@ TEST_F(TRACE_syscall, syscall_redirected)
TEST_F(TRACE_syscall, syscall_errno) { - long ret; - - ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); - ASSERT_EQ(0, ret); - - ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->prog, 0, 0); - ASSERT_EQ(0, ret); - - /* openat has been skipped and an errno return. */ + /* Tracer should skip the open syscall, resulting in ESRCH. */ EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat)); }
TEST_F(TRACE_syscall, syscall_faked) { - long ret; - - ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); - ASSERT_EQ(0, ret); - - ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->prog, 0, 0); - ASSERT_EQ(0, ret); - - /* gettid has been skipped and an altered return value stored. */ + /* Tracer skips the gettid syscall and store altered return value. */ EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid)); }
-TEST_F(TRACE_syscall, skip_after_RET_TRACE) +TEST_F(TRACE_syscall, skip_after) { struct sock_filter filter[] = { BPF_STMT(BPF_LD|BPF_W|BPF_ABS, @@ -2052,14 +2014,7 @@ TEST_F(TRACE_syscall, skip_after_RET_TRACE) }; long ret;
- ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); - ASSERT_EQ(0, ret); - - /* Install fixture filter. */ - ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->prog, 0, 0); - ASSERT_EQ(0, ret); - - /* Install "errno on getppid" filter. */ + /* Install additional "errno on getppid" filter. */ ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog, 0, 0); ASSERT_EQ(0, ret);
@@ -2069,7 +2024,7 @@ TEST_F(TRACE_syscall, skip_after_RET_TRACE) EXPECT_EQ(EPERM, errno); }
-TEST_F_SIGNAL(TRACE_syscall, kill_after_RET_TRACE, SIGSYS) +TEST_F_SIGNAL(TRACE_syscall, kill_after, SIGSYS) { struct sock_filter filter[] = { BPF_STMT(BPF_LD|BPF_W|BPF_ABS, @@ -2084,77 +2039,7 @@ TEST_F_SIGNAL(TRACE_syscall, kill_after_RET_TRACE, SIGSYS) }; long ret;
- ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); - ASSERT_EQ(0, ret); - - /* Install fixture filter. */ - ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->prog, 0, 0); - ASSERT_EQ(0, ret); - - /* Install "death on getppid" filter. */ - ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog, 0, 0); - ASSERT_EQ(0, ret); - - /* Tracer will redirect getpid to getppid, and we should die. */ - EXPECT_NE(self->mypid, syscall(__NR_getpid)); -} - -TEST_F(TRACE_syscall, skip_after_ptrace) -{ - struct sock_filter filter[] = { - BPF_STMT(BPF_LD|BPF_W|BPF_ABS, - offsetof(struct seccomp_data, nr)), - BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1), - BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ERRNO | EPERM), - BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW), - }; - struct sock_fprog prog = { - .len = (unsigned short)ARRAY_SIZE(filter), - .filter = filter, - }; - long ret; - - /* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */ - teardown_trace_fixture(_metadata, self->tracer); - self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL, - true); - - ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); - ASSERT_EQ(0, ret); - - /* Install "errno on getppid" filter. */ - ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog, 0, 0); - ASSERT_EQ(0, ret); - - /* Tracer will redirect getpid to getppid, and we should see EPERM. */ - EXPECT_EQ(-1, syscall(__NR_getpid)); - EXPECT_EQ(EPERM, errno); -} - -TEST_F_SIGNAL(TRACE_syscall, kill_after_ptrace, SIGSYS) -{ - struct sock_filter filter[] = { - BPF_STMT(BPF_LD|BPF_W|BPF_ABS, - offsetof(struct seccomp_data, nr)), - BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1), - BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL), - BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW), - }; - struct sock_fprog prog = { - .len = (unsigned short)ARRAY_SIZE(filter), - .filter = filter, - }; - long ret; - - /* Swap SECCOMP_RET_TRACE tracer for PTRACE_SYSCALL tracer. */ - teardown_trace_fixture(_metadata, self->tracer); - self->tracer = setup_trace_fixture(_metadata, tracer_ptrace, NULL, - true); - - ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); - ASSERT_EQ(0, ret); - - /* Install "death on getppid" filter. */ + /* Install additional "death on getppid" filter. */ ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog, 0, 0); ASSERT_EQ(0, ret);
On Sat, 4 Jul 2020 23:12:31 -0700 Kees Cook wrote:
Now that the selftest harness has variants, use them to eliminate a bunch of copy/paste duplication.
Cc: Andy Lutomirski luto@amacapital.net Cc: Will Drewry wad@chromium.org Cc: Jakub Kicinski kuba@kernel.org Signed-off-by: Kees Cook keescook@chromium.org
Acked-by: Jakub Kicinski kuba@kernel.org
There should be no difference between -1 and other negative syscalls while tracing.
Cc: Andy Lutomirski luto@amacapital.net Cc: Will Drewry wad@chromium.org Cc: Will Deacon will@kernel.org Cc: Keno Fischer keno@juliacomputing.com Signed-off-by: Kees Cook keescook@chromium.org --- tools/testing/selftests/seccomp/seccomp_bpf.c | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 966dec340ea8..bf6aa06c435c 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -1973,6 +1973,32 @@ FIXTURE_TEARDOWN(TRACE_syscall) teardown_trace_fixture(_metadata, self->tracer); }
+TEST(negative_ENOSYS) +{ + /* Untraced negative syscalls should return ENOSYS. */ + errno = 0; + EXPECT_EQ(-1, syscall(-1)); + EXPECT_EQ(errno, ENOSYS); + errno = 0; + EXPECT_EQ(-1, syscall(-101)); + EXPECT_EQ(errno, ENOSYS); +} + +TEST_F(TRACE_syscall, negative_ENOSYS) +{ + /* + * There should be no difference between an "internal" skip + * and userspace asking for syscall "-1". + */ + errno = 0; + EXPECT_EQ(-1, syscall(-1)); + EXPECT_EQ(errno, ENOSYS); + /* And no difference for "still not valid but not -1". */ + errno = 0; + EXPECT_EQ(-1, syscall(-101)); + EXPECT_EQ(errno, ENOSYS); +} + TEST_F(TRACE_syscall, syscall_allowed) { /* getppid works as expected (no changes). */
On Sat, Jul 04, 2020 at 11:12:32PM -0700, Kees Cook wrote:
There should be no difference between -1 and other negative syscalls while tracing.
Cc: Andy Lutomirski luto@amacapital.net Cc: Will Drewry wad@chromium.org Cc: Will Deacon will@kernel.org Cc: Keno Fischer keno@juliacomputing.com Signed-off-by: Kees Cook keescook@chromium.org
tools/testing/selftests/seccomp/seccomp_bpf.c | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 966dec340ea8..bf6aa06c435c 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -1973,6 +1973,32 @@ FIXTURE_TEARDOWN(TRACE_syscall) teardown_trace_fixture(_metadata, self->tracer); } +TEST(negative_ENOSYS) +{
- /* Untraced negative syscalls should return ENOSYS. */
- errno = 0;
- EXPECT_EQ(-1, syscall(-1));
- EXPECT_EQ(errno, ENOSYS);
- errno = 0;
- EXPECT_EQ(-1, syscall(-101));
- EXPECT_EQ(errno, ENOSYS);
+}
+TEST_F(TRACE_syscall, negative_ENOSYS) +{
- /*
* There should be no difference between an "internal" skip* and userspace asking for syscall "-1".*/- errno = 0;
- EXPECT_EQ(-1, syscall(-1));
- EXPECT_EQ(errno, ENOSYS);
- /* And no difference for "still not valid but not -1". */
- errno = 0;
- EXPECT_EQ(-1, syscall(-101));
- EXPECT_EQ(errno, ENOSYS);
+}
I realized after sending this that the second function could just be:
+TEST_F(TRACE_syscall, negative_ENOSYS) +{ + negative_ENOSYS(_metadata); +}
:)
TEST_F(TRACE_syscall, syscall_allowed) { /* getppid works as expected (no changes). */ -- 2.25.1
On Sat, Jul 04, 2020 at 11:12:29PM -0700, Kees Cook wrote:
This expands the seccomp selftest to poke a architectural behavior corner that Keno Fischer noticed[1]. In the process, I took the opportunity to do the kselftest harness variant refactoring I'd been meaning to do, which made adding this test much nicer.
I'd prefer this went via the seccomp tree, as it builds on top of the other recent seccomp feature addition tests. Testing and reviews are welcome! :)
Thanks! I tested these on arm64 (qemu) and they helped me to find a bug in some patches I was writing.
Tested-by: Will Deacon will@kernel.org
Will
On Fri, Jul 10, 2020 at 01:40:33PM +0100, Will Deacon wrote:
On Sat, Jul 04, 2020 at 11:12:29PM -0700, Kees Cook wrote:
This expands the seccomp selftest to poke a architectural behavior corner that Keno Fischer noticed[1]. In the process, I took the opportunity to do the kselftest harness variant refactoring I'd been meaning to do, which made adding this test much nicer.
I'd prefer this went via the seccomp tree, as it builds on top of the other recent seccomp feature addition tests. Testing and reviews are welcome! :)
Thanks! I tested these on arm64 (qemu) and they helped me to find a bug in some patches I was writing.
Hurray for tests! :)
Tested-by: Will Deacon will@kernel.org
Thanks!
linux-kselftest-mirror@lists.linaro.org
-
Jakub Kicinski -
Kees Cook -
Will Deacon