[PATCH v1 0/6] seccomp test fixes

List overview All Threads
Download

newer

older

[PATCH backport for 4.19]...

selftests/bpf/test_netcnt failed...

Tycho Andersen

19 Jan 2019 19 Jan '19

12:12 a.m.

Hi all,

Here are the fixes I previously mentioned I would send. I previously assumed that the tests were mostly run as root, but it turns out everything else besides the stuff I wrote in the seccomp tests either sets NNP and doesn't require real root, so it all actually works. This set of fixes should make most of the other tests work unprivileged, while XFAIL-ing the one that requires real root.

Cheers,

Tycho

Tycho Andersen (6): selftests: don't kill child immediately in get_metadata() test selftests: fix typo in seccomp_bpf.c selftest: include stdio.h in kselftest.h selftests: skip seccomp get_metadata test if not real root selftests: set NO_NEW_PRIVS bit in seccomp user tests selftests: unshare userns in seccomp pidns testcases

tools/testing/selftests/kselftest.h | 1 + tools/testing/selftests/seccomp/seccomp_bpf.c | 42 ++++++++++++++++--- 2 files changed, 38 insertions(+), 5 deletions(-)

-- 2.19.1

Show replies by thread

Tycho Andersen

19 Jan 19 Jan

12:12 a.m.

New subject: [PATCH 1/6] selftests: don't kill child immediately in get_metadata() test

This this test forks a child, and then the parent waits for a write() to a pipe signalling the child is ready to be attached to. If something in the child ASSERTs before it does this write, the test will hang waiting for it. Instead, let's EXPECT, so that execution continues until we do the write. Any failure after that is fine and can ASSERT.

Signed-off-by: Tycho Andersen tycho@tycho.ws --- tools/testing/selftests/seccomp/seccomp_bpf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 496a9a8c773a..9aba1b904089 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -2943,11 +2943,11 @@ TEST(get_metadata) };

/* one with log, one without */ - ASSERT_EQ(0, seccomp(SECCOMP_SET_MODE_FILTER, + EXPECT_EQ(0, seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_LOG, &prog)); - ASSERT_EQ(0, seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog)); + EXPECT_EQ(0, seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog));

- ASSERT_EQ(0, close(pipefd[0])); + EXPECT_EQ(0, close(pipefd[0])); ASSERT_EQ(1, write(pipefd[1], "1", 1)); ASSERT_EQ(0, close(pipefd[1]));

-- 2.19.1

Tycho Andersen

12:12 a.m.

New subject: [PATCH 2/6] selftests: fix typo in seccomp_bpf.c

There used to be an explanation here because it could trigger lockdep previously, but now we're not doing recursive locking, so it really is just for grins.

Signed-off-by: Tycho Andersen tycho@tycho.ws --- tools/testing/selftests/seccomp/seccomp_bpf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 9aba1b904089..912a2a5430dc 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -3035,7 +3035,7 @@ TEST(user_notification_basic) EXPECT_EQ(true, WIFEXITED(status)); EXPECT_EQ(0, WEXITSTATUS(status));

- /* Add some no-op filters so for grins. */ + /* Add some no-op filters for grins. */ EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0); EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0); EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0);

-- 2.19.1

Tycho Andersen

12:12 a.m.

New subject: [PATCH 3/6] selftest: include stdio.h in kselftest.h

While playing around with a way to skip the seccomp get_metadata test, I noticed that this header uses printf() without defining it, leading to,

../kselftest.h: In function ‘ksft_print_header’: ../kselftest.h:61:3: warning: implicit declaration of function ‘printf’ [-Wimplicit-function-declaration] printf("TAP version 13\n"); ^~~~~~ ../kselftest.h:61:3: warning: incompatible implicit declaration of built-in function ‘printf’ ../kselftest.h:61:3: note: include ‘<stdio.h>’ or provide a declaration of ‘printf’

if user code doesn't also use printf.

Signed-off-by: Tycho Andersen tycho@tycho.ws --- tools/testing/selftests/kselftest.h | 1 + 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h index a3edb2c8e43d..47e1d995c182 100644 --- a/tools/testing/selftests/kselftest.h +++ b/tools/testing/selftests/kselftest.h @@ -13,6 +13,7 @@ #include <stdlib.h> #include <unistd.h> #include <stdarg.h> +#include <stdio.h>

/* define kselftest exit codes */ #define KSFT_PASS 0

-- 2.19.1

Tycho Andersen

12:12 a.m.

New subject: [PATCH 4/6] selftests: skip seccomp get_metadata test if not real root

The get_metadata() test requires real root, so let's skip it if we're not real root.

Note that I used XFAIL here because that's what the test does later if CONFIG_CHEKCKPOINT_RESTORE happens to not be enabled. After looking at the code, there doesn't seem to be a nice way to skip tests defined as TEST(), since there's no return code (I tried exit(KSFT_SKIP), but that didn't work either...). So let's do it this way to be consistent, and easier to fix when someone comes along and fixes it.

Signed-off-by: Tycho Andersen tycho@tycho.ws --- tools/testing/selftests/seccomp/seccomp_bpf.c | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 912a2a5430dc..ab6b6620f522 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -2929,6 +2929,12 @@ TEST(get_metadata) struct seccomp_metadata md; long ret;

+ /* Only real root can get metadata. */ + if (geteuid()) { + XFAIL(return, "get_metadata requires real root"); + return; + } + ASSERT_EQ(0, pipe(pipefd));

pid = fork();

-- 2.19.1

Tycho Andersen

12:12 a.m.

New subject: [PATCH 5/6] selftests: set NO_NEW_PRIVS bit in seccomp user tests

seccomp() doesn't allow users who aren't root in their userns to attach filters unless they have the nnp bit set, so let's set it so that these tests can pass when run as an unprivileged user.

This idea stolen from the other seccomp tests, which use this trick :)

Signed-off-by: Tycho Andersen tycho@tycho.ws --- tools/testing/selftests/seccomp/seccomp_bpf.c | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index ab6b6620f522..a4a7dce1a91b 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -3026,6 +3026,11 @@ TEST(user_notification_basic) .filter = filter, };

+ ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); + ASSERT_EQ(0, ret) { + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!"); + } + pid = fork(); ASSERT_GE(pid, 0);

@@ -3107,6 +3112,11 @@ TEST(user_notification_kill_in_middle) struct seccomp_notif req = {}; struct seccomp_notif_resp resp = {};

+ ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); + ASSERT_EQ(0, ret) { + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!"); + } + listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER); ASSERT_GE(listener, 0); @@ -3154,6 +3164,11 @@ TEST(user_notification_signal) struct seccomp_notif_resp resp = {}; char c;

+ ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); + ASSERT_EQ(0, ret) { + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!"); + } + ASSERT_EQ(socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair), 0);

listener = user_trap_syscall(__NR_gettid, @@ -3219,6 +3234,11 @@ TEST(user_notification_closed_listener) long ret; int status, listener;

+ ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); + ASSERT_EQ(0, ret) { + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!"); + } + listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER); ASSERT_GE(listener, 0); @@ -3350,6 +3370,10 @@ TEST(user_notification_fault_recv) struct seccomp_notif req = {}; struct seccomp_notif_resp resp = {};

+ ASSERT_EQ(prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0), 0) { + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!"); + } + listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER); ASSERT_GE(listener, 0);

-- 2.19.1

Tycho Andersen

12:12 a.m.

New subject: [PATCH 6/6] selftests: unshare userns in seccomp pidns testcases

The pid ns cannot be unshare()d as an unprivileged user without owning the userns as well. Let's unshare the userns so that we can subsequently unshare the pidns.

This also means that we don't need to set the no new privs bit as in the other test cases, since we're unsharing the userns.

Signed-off-by: Tycho Andersen tycho@tycho.ws --- tools/testing/selftests/seccomp/seccomp_bpf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index a4a7dce1a91b..8f6e95773225 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -3271,7 +3271,7 @@ TEST(user_notification_child_pid_ns) struct seccomp_notif req = {}; struct seccomp_notif_resp resp = {};

- ASSERT_EQ(unshare(CLONE_NEWPID), 0); + ASSERT_EQ(unshare(CLONE_NEWUSER | CLONE_NEWPID), 0);

listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER); ASSERT_GE(listener, 0); @@ -3308,6 +3308,8 @@ TEST(user_notification_sibling_pid_ns) struct seccomp_notif req = {}; struct seccomp_notif_resp resp = {};

+ ASSERT_EQ(unshare(CLONE_NEWUSER), 0); + listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER); ASSERT_GE(listener, 0);

-- 2.19.1

Kees Cook

20 Jan 20 Jan

12:43 a.m.

On Fri, Jan 18, 2019 at 4:12 PM Tycho Andersen tycho@tycho.ws wrote:

...

Hi all,

Here are the fixes I previously mentioned I would send. I previously assumed that the tests were mostly run as root, but it turns out everything else besides the stuff I wrote in the seccomp tests either sets NNP and doesn't require real root, so it all actually works. This set of fixes should make most of the other tests work unprivileged, while XFAIL-ing the one that requires real root.

Awesome. This all looks good to me. :)

Acked-by: Kees Cook keescook@chromium.org

Shuah, can you take this series?

-Kees

...

Cheers,

Tycho

Tycho Andersen (6): selftests: don't kill child immediately in get_metadata() test selftests: fix typo in seccomp_bpf.c selftest: include stdio.h in kselftest.h selftests: skip seccomp get_metadata test if not real root selftests: set NO_NEW_PRIVS bit in seccomp user tests selftests: unshare userns in seccomp pidns testcases

tools/testing/selftests/kselftest.h | 1 + tools/testing/selftests/seccomp/seccomp_bpf.c | 42 ++++++++++++++++--- 2 files changed, 38 insertions(+), 5 deletions(-)

-- 2.19.1

-- Kees Cook

shuah

7:28 p.m.

On 1/19/19 5:43 PM, Kees Cook wrote:

...

On Fri, Jan 18, 2019 at 4:12 PM Tycho Andersen tycho@tycho.ws wrote:

...
Hi all,

Here are the fixes I previously mentioned I would send. I previously assumed that the tests were mostly run as root, but it turns out everything else besides the stuff I wrote in the seccomp tests either sets NNP and doesn't require real root, so it all actually works. This set of fixes should make most of the other tests work unprivileged, while XFAIL-ing the one that requires real root.

Tycho, Thanks for a quick response in fixing the problems.

...

Awesome. This all looks good to me. :)

Acked-by: Kees Cook keescook@chromium.org

Shuah, can you take this series?

Yes. I will take these in for rc5.

thanks, -- Shuah

1945

days inactive

1946

days old

linux-kselftest-mirror@lists.linaro.org

8 comments

participants

tags (0)

participants (3)

Kees Cook
shuah
Tycho Andersen