| Message ID | 20190516102023.19004-2-po-hsu.lin@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/seccomp: Enhance per-arch ptracesyscall skip tests | expand |
On 16.05.19 12:20, Po-Hsu Lin wrote: > From: Kees Cook <keescook@chromium.org> > > BugLink: https://bugs.launchpad.net/bugs/1812796 > > Passing EPERM during syscall skipping was confusing since the test wasn't > actually exercising the errno evaluation -- it was just passing a literal > "1" (EPERM). Instead, expand the tests to check both direct value returns > (positive, 45000 in this case), and errno values (negative, -ESRCH in this > case) to check both fake success and fake failure during syscall skipping. > > Reported-by: Colin Ian King <colin.king@canonical.com> > Fixes: a33b2d0359a0 ("selftests/seccomp: Add tests for basic ptrace actions") > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Shuah Khan <shuah@kernel.org> > (cherry picked from commit ed5f13261cb65b02c611ae9971677f33581d4286) > Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > tools/testing/selftests/seccomp/seccomp_bpf.c | 72 +++++++++++++++++++++------ > 1 file changed, 57 insertions(+), 15 deletions(-) > > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c > index e350cf3..292bbf0 100644 > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c > @@ -1563,7 +1563,16 @@ TEST_F(TRACE_poke, getpid_runs_normally) > #ifdef SYSCALL_NUM_RET_SHARE_REG > # define EXPECT_SYSCALL_RETURN(val, action) EXPECT_EQ(-1, action) > #else > -# define EXPECT_SYSCALL_RETURN(val, action) EXPECT_EQ(val, action) > +# define EXPECT_SYSCALL_RETURN(val, action) \ > + do { \ > + errno = 0; \ > + if (val < 0) { \ > + EXPECT_EQ(-1, action); \ > + EXPECT_EQ(-(val), errno); \ > + } else { \ > + EXPECT_EQ(val, action); \ > + } \ > + } while (0) > #endif > > /* Use PTRACE_GETREGS and PTRACE_SETREGS when available. This is useful for > @@ -1602,7 +1611,7 @@ int get_syscall(struct __test_metadata *_metadata, pid_t tracee) > > /* Architecture-specific syscall changing routine. */ > void change_syscall(struct __test_metadata *_metadata, > - pid_t tracee, int syscall) > + pid_t tracee, int syscall, int result) > { > int ret; > ARCH_REGS regs; > @@ -1661,7 +1670,7 @@ void change_syscall(struct __test_metadata *_metadata, > #ifdef SYSCALL_NUM_RET_SHARE_REG > TH_LOG("Can't modify syscall return on this architecture"); > #else > - regs.SYSCALL_RET = EPERM; > + regs.SYSCALL_RET = result; > #endif > > #ifdef HAVE_GETREGS > @@ -1689,14 +1698,19 @@ void tracer_syscall(struct __test_metadata *_metadata, pid_t tracee, > case 0x1002: > /* change getpid to getppid. */ > EXPECT_EQ(__NR_getpid, get_syscall(_metadata, tracee)); > - change_syscall(_metadata, tracee, __NR_getppid); > + change_syscall(_metadata, tracee, __NR_getppid, 0); > break; > case 0x1003: > - /* skip gettid. */ > + /* skip gettid with valid return code. */ > EXPECT_EQ(__NR_gettid, get_syscall(_metadata, tracee)); > - change_syscall(_metadata, tracee, -1); > + change_syscall(_metadata, tracee, -1, 45000); > break; > case 0x1004: > + /* skip openat with error. */ > + EXPECT_EQ(__NR_openat, get_syscall(_metadata, tracee)); > + change_syscall(_metadata, tracee, -1, -ESRCH); > + break; > + case 0x1005: > /* do nothing (allow getppid) */ > EXPECT_EQ(__NR_getppid, get_syscall(_metadata, tracee)); > break; > @@ -1729,9 +1743,11 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee, > nr = get_syscall(_metadata, tracee); > > if (nr == __NR_getpid) > - change_syscall(_metadata, tracee, __NR_getppid); > + change_syscall(_metadata, tracee, __NR_getppid, 0); > + if (nr == __NR_gettid) > + change_syscall(_metadata, tracee, -1, 45000); > if (nr == __NR_openat) > - change_syscall(_metadata, tracee, -1); > + change_syscall(_metadata, tracee, -1, -ESRCH); > } > > FIXTURE_DATA(TRACE_syscall) { > @@ -1748,8 +1764,10 @@ FIXTURE_SETUP(TRACE_syscall) > BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1002), > BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_gettid, 0, 1), > BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1003), > - BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1), > + BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_openat, 0, 1), > BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1004), > + BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1), > + BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1005), > BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW), > }; > > @@ -1797,15 +1815,26 @@ TEST_F(TRACE_syscall, ptrace_syscall_redirected) > EXPECT_NE(self->mypid, syscall(__NR_getpid)); > } > > -TEST_F(TRACE_syscall, ptrace_syscall_dropped) > +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); > + > + /* Tracer should skip the open syscall, resulting in ESRCH. */ > + EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat)); > +} > + > +TEST_F(TRACE_syscall, ptrace_syscall_faked) > { > /* 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 open syscall, resulting in EPERM. */ > - EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_openat)); > + /* Tracer should skip the gettid syscall, resulting fake pid. */ > + EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid)); > } > > TEST_F(TRACE_syscall, syscall_allowed) > @@ -1838,7 +1867,21 @@ TEST_F(TRACE_syscall, syscall_redirected) > EXPECT_NE(self->mypid, syscall(__NR_getpid)); > } > > -TEST_F(TRACE_syscall, syscall_dropped) > +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. */ > + EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat)); > +} > + > +TEST_F(TRACE_syscall, syscall_faked) > { > long ret; > > @@ -1849,8 +1892,7 @@ TEST_F(TRACE_syscall, syscall_dropped) > ASSERT_EQ(0, ret); > > /* gettid has been skipped and an altered return value stored. */ > - EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_gettid)); > - EXPECT_NE(self->mytid, syscall(__NR_gettid)); > + EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid)); > } > > TEST_F(TRACE_syscall, skip_after_RET_TRACE) >
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index e350cf3..292bbf0 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -1563,7 +1563,16 @@ TEST_F(TRACE_poke, getpid_runs_normally) #ifdef SYSCALL_NUM_RET_SHARE_REG # define EXPECT_SYSCALL_RETURN(val, action) EXPECT_EQ(-1, action) #else -# define EXPECT_SYSCALL_RETURN(val, action) EXPECT_EQ(val, action) +# define EXPECT_SYSCALL_RETURN(val, action) \ + do { \ + errno = 0; \ + if (val < 0) { \ + EXPECT_EQ(-1, action); \ + EXPECT_EQ(-(val), errno); \ + } else { \ + EXPECT_EQ(val, action); \ + } \ + } while (0) #endif /* Use PTRACE_GETREGS and PTRACE_SETREGS when available. This is useful for @@ -1602,7 +1611,7 @@ int get_syscall(struct __test_metadata *_metadata, pid_t tracee) /* Architecture-specific syscall changing routine. */ void change_syscall(struct __test_metadata *_metadata, - pid_t tracee, int syscall) + pid_t tracee, int syscall, int result) { int ret; ARCH_REGS regs; @@ -1661,7 +1670,7 @@ void change_syscall(struct __test_metadata *_metadata, #ifdef SYSCALL_NUM_RET_SHARE_REG TH_LOG("Can't modify syscall return on this architecture"); #else - regs.SYSCALL_RET = EPERM; + regs.SYSCALL_RET = result; #endif #ifdef HAVE_GETREGS @@ -1689,14 +1698,19 @@ void tracer_syscall(struct __test_metadata *_metadata, pid_t tracee, case 0x1002: /* change getpid to getppid. */ EXPECT_EQ(__NR_getpid, get_syscall(_metadata, tracee)); - change_syscall(_metadata, tracee, __NR_getppid); + change_syscall(_metadata, tracee, __NR_getppid, 0); break; case 0x1003: - /* skip gettid. */ + /* skip gettid with valid return code. */ EXPECT_EQ(__NR_gettid, get_syscall(_metadata, tracee)); - change_syscall(_metadata, tracee, -1); + change_syscall(_metadata, tracee, -1, 45000); break; case 0x1004: + /* skip openat with error. */ + EXPECT_EQ(__NR_openat, get_syscall(_metadata, tracee)); + change_syscall(_metadata, tracee, -1, -ESRCH); + break; + case 0x1005: /* do nothing (allow getppid) */ EXPECT_EQ(__NR_getppid, get_syscall(_metadata, tracee)); break; @@ -1729,9 +1743,11 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee, nr = get_syscall(_metadata, tracee); if (nr == __NR_getpid) - change_syscall(_metadata, tracee, __NR_getppid); + change_syscall(_metadata, tracee, __NR_getppid, 0); + if (nr == __NR_gettid) + change_syscall(_metadata, tracee, -1, 45000); if (nr == __NR_openat) - change_syscall(_metadata, tracee, -1); + change_syscall(_metadata, tracee, -1, -ESRCH); } FIXTURE_DATA(TRACE_syscall) { @@ -1748,8 +1764,10 @@ FIXTURE_SETUP(TRACE_syscall) BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1002), BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_gettid, 0, 1), BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1003), - BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1), + BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_openat, 0, 1), BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1004), + BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getppid, 0, 1), + BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_TRACE | 0x1005), BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW), }; @@ -1797,15 +1815,26 @@ TEST_F(TRACE_syscall, ptrace_syscall_redirected) EXPECT_NE(self->mypid, syscall(__NR_getpid)); } -TEST_F(TRACE_syscall, ptrace_syscall_dropped) +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); + + /* Tracer should skip the open syscall, resulting in ESRCH. */ + EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat)); +} + +TEST_F(TRACE_syscall, ptrace_syscall_faked) { /* 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 open syscall, resulting in EPERM. */ - EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_openat)); + /* Tracer should skip the gettid syscall, resulting fake pid. */ + EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid)); } TEST_F(TRACE_syscall, syscall_allowed) @@ -1838,7 +1867,21 @@ TEST_F(TRACE_syscall, syscall_redirected) EXPECT_NE(self->mypid, syscall(__NR_getpid)); } -TEST_F(TRACE_syscall, syscall_dropped) +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. */ + EXPECT_SYSCALL_RETURN(-ESRCH, syscall(__NR_openat)); +} + +TEST_F(TRACE_syscall, syscall_faked) { long ret; @@ -1849,8 +1892,7 @@ TEST_F(TRACE_syscall, syscall_dropped) ASSERT_EQ(0, ret); /* gettid has been skipped and an altered return value stored. */ - EXPECT_SYSCALL_RETURN(EPERM, syscall(__NR_gettid)); - EXPECT_NE(self->mytid, syscall(__NR_gettid)); + EXPECT_SYSCALL_RETURN(45000, syscall(__NR_gettid)); } TEST_F(TRACE_syscall, skip_after_RET_TRACE)