Kees Cook keescook@chromium.org writes:
On Thu, Oct 28, 2021 at 05:06:53PM -0500, Eric W. Biederman wrote:
Kees Cook keescook@chromium.org writes:
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).
Oh. This is being ptraced as part of the test?
Yes. The signal started being delivered. As far as that goes that sounds correct.
Ptrace is allowed to intercept even fatal signals. Everything except SIGKILL.
Is this a condition we don't want even ptrace to be able to catch?
I think we can arrange it so that even ptrace can't intercept this signal. I need to sit this problem on the back burner for a few minutes. It is an angle I had not considered.
Is it a problem that the debugger can see the signal if the process does not?
Right, I'm trying to understand that too. However, my neighbor just lost power. :|
What I was in the middle of checking was what ptrace "sees" going through a fatal SIGSYS; my initial debugging attempts were weird.
If we don't allow ptrace to see these signals, then it makes it possible for complete_signal to short circuit deliver them and ignore ptrace later on. Which seems nice, and allows for not needing to change sigaction at all in the future.
I don't know if it is strictly necessary. It is not like people using debuggers have complained yet.
I just posted a patch that solves this by setting an extra flag called SA_IMMUTABLE and disabling sigaction and ptrace when the flag is set.
I think that is a simple patch that sorts this out.
Eric