| Message ID | 20200912110820.597135-14-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selftests/seccomp: Refactor change_syscall() | expand |
Kees Cook <keescook@chromium.org> writes: > Some archs (like ppc) only support changing the return code during > syscall exit when ptrace is used. As the syscall number might not > be available anymore during syscall exit, it needs to be saved > during syscall enter. Adjust the ptrace tests to do this. I'm not that across all the fixture stuff, but if I'm reading it right you're now calling change_syscall() on both entry and exit for all arches. That should work, but it no longer tests changing the return code on entry on the arches that support it, which seems like a backward step? cheers > Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > Suggested-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > Link: https://lore.kernel.org/linux-kselftest/20200911181012.171027-1-cascardo@canonical.com/ > Fixes: 58d0a862f573 ("seccomp: add tests for ptrace hole") > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > tools/testing/selftests/seccomp/seccomp_bpf.c | 34 +++++++++++-------- > 1 file changed, 20 insertions(+), 14 deletions(-) > > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c > index bbab2420d708..26c712c6a575 100644 > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c > @@ -1949,12 +1949,19 @@ void tracer_seccomp(struct __test_metadata *_metadata, pid_t tracee, > > } > > +FIXTURE(TRACE_syscall) { > + struct sock_fprog prog; > + pid_t tracer, mytid, mypid, parent; > + long syscall_nr; > +}; > + > void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee, > int status, void *args) > { > - int ret, nr; > + int ret; > unsigned long msg; > static bool entry; > + FIXTURE_DATA(TRACE_syscall) *self = args; > > /* > * The traditional way to tell PTRACE_SYSCALL entry/exit > @@ -1968,24 +1975,23 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee, > EXPECT_EQ(entry ? PTRACE_EVENTMSG_SYSCALL_ENTRY > : PTRACE_EVENTMSG_SYSCALL_EXIT, msg); > > - if (!entry) > - return; > - > - nr = get_syscall(_metadata, tracee); > + /* > + * Some architectures only support setting return values during > + * syscall exit under ptrace, and on exit the syscall number may > + * no longer be available. Therefore, save it here, and call > + * "change syscall and set return values" on both entry and exit. > + */ > + if (entry) > + self->syscall_nr = get_syscall(_metadata, tracee); > > - if (nr == __NR_getpid) > + if (self->syscall_nr == __NR_getpid) > change_syscall(_metadata, tracee, __NR_getppid, 0); > - if (nr == __NR_gettid) > + if (self->syscall_nr == __NR_gettid) > change_syscall(_metadata, tracee, -1, 45000); > - if (nr == __NR_openat) > + if (self->syscall_nr == __NR_openat) > change_syscall(_metadata, tracee, -1, -ESRCH); > } > > -FIXTURE(TRACE_syscall) { > - struct sock_fprog prog; > - pid_t tracer, mytid, mypid, parent; > -}; > - > FIXTURE_VARIANT(TRACE_syscall) { > /* > * All of the SECCOMP_RET_TRACE behaviors can be tested with either > @@ -2044,7 +2050,7 @@ FIXTURE_SETUP(TRACE_syscall) > self->tracer = setup_trace_fixture(_metadata, > variant->use_ptrace ? tracer_ptrace > : tracer_seccomp, > - NULL, variant->use_ptrace); > + self, variant->use_ptrace); > > ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); > ASSERT_EQ(0, ret); > -- > 2.25.1
On Mon, Sep 14, 2020 at 03:47:13PM +1000, Michael Ellerman wrote: > Kees Cook <keescook@chromium.org> writes: > > Some archs (like ppc) only support changing the return code during > > syscall exit when ptrace is used. As the syscall number might not > > be available anymore during syscall exit, it needs to be saved > > during syscall enter. Adjust the ptrace tests to do this. > > I'm not that across all the fixture stuff, but if I'm reading it right > you're now calling change_syscall() on both entry and exit for all > arches. Correct. > That should work, but it no longer tests changing the return code on > entry on the arches that support it, which seems like a backward step? That's a good point. I wouldn't be in a position to notice a regression for the other architectures. I will refactor this one...
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index bbab2420d708..26c712c6a575 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -1949,12 +1949,19 @@ void tracer_seccomp(struct __test_metadata *_metadata, pid_t tracee, } +FIXTURE(TRACE_syscall) { + struct sock_fprog prog; + pid_t tracer, mytid, mypid, parent; + long syscall_nr; +}; + void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee, int status, void *args) { - int ret, nr; + int ret; unsigned long msg; static bool entry; + FIXTURE_DATA(TRACE_syscall) *self = args; /* * The traditional way to tell PTRACE_SYSCALL entry/exit @@ -1968,24 +1975,23 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee, EXPECT_EQ(entry ? PTRACE_EVENTMSG_SYSCALL_ENTRY : PTRACE_EVENTMSG_SYSCALL_EXIT, msg); - if (!entry) - return; - - nr = get_syscall(_metadata, tracee); + /* + * Some architectures only support setting return values during + * syscall exit under ptrace, and on exit the syscall number may + * no longer be available. Therefore, save it here, and call + * "change syscall and set return values" on both entry and exit. + */ + if (entry) + self->syscall_nr = get_syscall(_metadata, tracee); - if (nr == __NR_getpid) + if (self->syscall_nr == __NR_getpid) change_syscall(_metadata, tracee, __NR_getppid, 0); - if (nr == __NR_gettid) + if (self->syscall_nr == __NR_gettid) change_syscall(_metadata, tracee, -1, 45000); - if (nr == __NR_openat) + if (self->syscall_nr == __NR_openat) change_syscall(_metadata, tracee, -1, -ESRCH); } -FIXTURE(TRACE_syscall) { - struct sock_fprog prog; - pid_t tracer, mytid, mypid, parent; -}; - FIXTURE_VARIANT(TRACE_syscall) { /* * All of the SECCOMP_RET_TRACE behaviors can be tested with either @@ -2044,7 +2050,7 @@ FIXTURE_SETUP(TRACE_syscall) self->tracer = setup_trace_fixture(_metadata, variant->use_ptrace ? tracer_ptrace : tracer_seccomp, - NULL, variant->use_ptrace); + self, variant->use_ptrace); ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); ASSERT_EQ(0, ret);
Some archs (like ppc) only support changing the return code during syscall exit when ptrace is used. As the syscall number might not be available anymore during syscall exit, it needs to be saved during syscall enter. Adjust the ptrace tests to do this. Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> Suggested-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> Link: https://lore.kernel.org/linux-kselftest/20200911181012.171027-1-cascardo@canonical.com/ Fixes: 58d0a862f573 ("seccomp: add tests for ptrace hole") Signed-off-by: Kees Cook <keescook@chromium.org> --- tools/testing/selftests/seccomp/seccomp_bpf.c | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-)