On Thu, Oct 28, 2021 at 12:26:26PM -0500, Eric W. Biederman wrote:
> Kees Cook <keescook@xxxxxxxxxxxx> 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);
--
Kees Cook
