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