On Thu, Oct 28, 2021 at 12:26:26PM -0500, Eric W. Biederman wrote:
Kees Cook keescook@chromium.org writes:
On Thu, Oct 28, 2021 at 06:21:12PM +0200, Andrea Righi wrote:
The following sub-tests are failing in seccomp_bpf selftest:
18:56:54 DEBUG| [stdout] # selftests: seccomp: seccomp_bpf ... 18:56:57 DEBUG| [stdout] # # RUN TRACE_syscall.ptrace.kill_after ... 18:56:57 DEBUG| [stdout] # # seccomp_bpf.c:2023:kill_after:Expected entry ? PTRACE_EVENTMSG_SYSCALL_ENTRY : PTRACE_EVENTMSG_SYSCALL_EXIT (1) == msg (0) 18:56:57 DEBUG| [stdout] # # seccomp_bpf.c:2023:kill_after:Expected entry ? PTRACE_EVENTMSG_SYSCALL_ENTRY : PTRACE_EVENTMSG_SYSCALL_EXIT (2) == msg (1) 18:56:57 DEBUG| [stdout] # # seccomp_bpf.c:2023:kill_after:Expected entry ? PTRACE_EVENTMSG_SYSCALL_ENTRY : PTRACE_EVENTMSG_SYSCALL_EXIT (1) == msg (2) 18:56:57 DEBUG| [stdout] # # kill_after: Test exited normally instead of by signal (code: 12) 18:56:57 DEBUG| [stdout] # # FAIL TRACE_syscall.ptrace.kill_after ... 18:56:57 DEBUG| [stdout] # # RUN TRACE_syscall.seccomp.kill_after ... 18:56:57 DEBUG| [stdout] # # seccomp_bpf.c:1547:kill_after:Expected !ptrace_syscall (1) == IS_SECCOMP_EVENT(status) (0) 18:56:57 DEBUG| [stdout] # # kill_after: Test exited normally instead of by signal (code: 0) 18:56:57 DEBUG| [stdout] # # FAIL TRACE_syscall.seccomp.kill_after 18:56:57 DEBUG| [stdout] # not ok 80 TRACE_syscall.seccomp.kill_after ... 18:56:57 DEBUG| [stdout] # # FAILED: 85 / 87 tests passed. 18:56:57 DEBUG| [stdout] # # Totals: pass:85 fail:2 xfail:0 xpass:0 skip:0 error:0 18:56:57 DEBUG| [stdout] not ok 1 selftests: seccomp: seccomp_bpf # exit=1
I did some bisecting and found that the failures started to happen with:
307d522f5eb8 ("signal/seccomp: Refactor seccomp signal and coredump generation")
Not sure if the test needs to be fixed after this commit, or if the commit is actually introducing an issue. I'll investigate more, unless someone knows already what's going on.
Ah thanks for noticing; I will investigate...
I just did a quick read through of the test and while I don't understand everything having a failure seems very weird.
I don't understand the comment: /* Tracer will redirect getpid to getppid, and we should die. */
As I think what happens is it the bpf programs loads the signal number. Tests to see if the signal number if GETPPID and allows that system call and causes any other system call to be terminated.
The test suite runs a series of seccomp filter vs syscalls under tracing, either with ptrace or with seccomp SECCOMP_RET_TRACE, to validate the expected behavioral states. It seems that what's happened is that the SIGSYS has suddenly become non-killing:
# RUN TRACE_syscall.ptrace.kill_after ... # seccomp_bpf.c:1555:kill_after:Expected WSTOPSIG(status) & 0x80 (0) == 0x80 (128) # seccomp_bpf.c:1556:kill_after:WSTOPSIG: 31 # kill_after: Test exited normally instead of by signal (code: 12) # FAIL TRACE_syscall.ptrace.kill_after
i.e. the ptracer no longer sees a dead tracee, which would pass through here:
if (WIFSIGNALED(status) || WIFEXITED(status)) /* Child is dead. Time to go. */ return;
So the above saw a SIG_TRAP|SIGSYS rather than a killing SIGSYS. i.e. instead of WIFSIGNALED(stauts) being true, it instead catches a PTRACE_EVENT_STOP for SIGSYS, which should be impossible (the process should be getting killed).
Which being single threaded would seem to cause the kernel to execute the changed code.
How there kernel at that point is having the process exit with anything except SIGSYS I am not immediately seeing.
I've run out of time at the moment to debug further, but I've appended my changes to the test, and a brute-force change to kernel/seccomp.c to restore original behavior (though I haven't tested if coredumping works still). I'll return to this in a few hours...
The logic is the same as that for SECCOMP_RET_TRAP is there a test for that, that is also failing?
How do you run that test anyway?
cd tools/testing/selftests/seccomp make seccomp_bpf scp seccomp_bpf target: ssh target ./seccomp_bpf
diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 4d8f44a17727..b6c8c8f8bd69 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1269,10 +1269,12 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd, syscall_rollback(current, current_pt_regs()); /* Trigger a coredump with SIGSYS */ force_sig_seccomp(this_syscall, data, true); - } else { - do_exit(SIGSYS); + do_group_exit(SIGSYS); } - return -1; /* skip the syscall go directly to signal handling */ + if (action == SECCOMP_RET_KILL_THREAD) + do_exit(SIGSYS); + else + do_group_exit(SIGSYS); }
unreachable(); diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 1d64891e6492..8f8c1df885d6 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -1487,7 +1487,7 @@ TEST_F(precedence, log_is_fifth_in_any_order) #define PTRACE_EVENT_SECCOMP 7 #endif
-#define IS_SECCOMP_EVENT(status) ((status >> 16) == PTRACE_EVENT_SECCOMP) +#define PTRACE_EVENT_MASK(status) ((status) >> 16) bool tracer_running; void tracer_stop(int sig) { @@ -1536,17 +1536,34 @@ void start_tracer(struct __test_metadata *_metadata, int fd, pid_t tracee, /* Run until we're shut down. Must assert to stop execution. */ while (tracer_running) { int status; + bool run_callback = true;
if (wait(&status) != tracee) continue; + if (WIFSIGNALED(status) || WIFEXITED(status)) /* Child is dead. Time to go. */ return;
- /* Check if this is a seccomp event. */ - ASSERT_EQ(!ptrace_syscall, IS_SECCOMP_EVENT(status)); + /* Check if we got an expected event. */ + ASSERT_EQ(WIFCONTINUED(status), false); + ASSERT_EQ(WIFSTOPPED(status), true); + ASSERT_EQ(WSTOPSIG(status) & SIGTRAP, SIGTRAP) { + TH_LOG("WSTOPSIG: %d", WSTOPSIG(status)); + } + if (ptrace_syscall) { + EXPECT_EQ(WSTOPSIG(status) & 0x80, 0x80) { + TH_LOG("WSTOPSIG: %d", WSTOPSIG(status)); + run_callback = false; + }; + } else { + EXPECT_EQ(PTRACE_EVENT_MASK(status), PTRACE_EVENT_SECCOMP) { + run_callback = false; + }; + }
- tracer_func(_metadata, tracee, status, args); + if (run_callback) + tracer_func(_metadata, tracee, status, args);
ret = ptrace(ptrace_syscall ? PTRACE_SYSCALL : PTRACE_CONT, tracee, NULL, 0);