bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: Kees Cook <keescook@chromium.org>
Cc: Andrea Righi <andrea.righi@canonical.com>,
	Shuah Khan <shuah@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Andy Lutomirski <luto@amacapital.net>,
	Will Drewry <wad@chromium.org>,
	linux-kselftest@vger.kernel.org, bpf@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: selftests: seccomp_bpf failure on 5.15
Date: Thu, 28 Oct 2021 17:06:53 -0500	[thread overview]
Message-ID: <8735okls76.fsf@disp2133> (raw)
In-Reply-To: <202110281136.5CE65399A7@keescook> (Kees Cook's message of "Thu, 28 Oct 2021 11:47:48 -0700")

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?

Eric

  reply	other threads:[~2021-10-28 22:07 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-28 16:21 selftests: seccomp_bpf failure on 5.15 Andrea Righi
2021-10-28 16:56 ` Kees Cook
2021-10-28 17:26   ` Eric W. Biederman
2021-10-28 18:47     ` Kees Cook
2021-10-28 22:06       ` Eric W. Biederman [this message]
2021-10-29 14:58         ` Kees Cook
2021-10-29 15:17           ` Eric W. Biederman
2021-11-02 18:22           ` Eric W. Biederman
2021-11-03 16:14             ` Kees Cook
2021-11-03 18:35               ` Eric W. Biederman
2021-10-29 15:09       ` [PATCH] signal: Add SA_IMMUTABLE to ensure forced siganls do not get changed Eric W. Biederman
2021-10-31 17:40         ` Andrea Righi
2021-11-01 22:28           ` Eric W. Biederman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8735okls76.fsf@disp2133 \
    --to=ebiederm@xmission.com \
    --cc=andrea.righi@canonical.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=keescook@chromium.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=shuah@kernel.org \
    --cc=wad@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).