From: Kees Cook <keescook@chromium.org>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Kyle Huey <me@kylehuey.com>,
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>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@vger.kernel.org>,
bpf@vger.kernel.org, open list <linux-kernel@vger.kernel.org>,
linux-hardening@vger.kernel.org,
Linus Torvalds <torvalds@linux-foundation.org>,
Robert O'Callahan <rocallahan@gmail.com>
Subject: Re: [REGRESSION] 5.16rc1: SA_IMMUTABLE breaks debuggers
Date: Wed, 17 Nov 2021 13:54:14 -0800 [thread overview]
Message-ID: <202111171341.41053845C3@keescook> (raw)
In-Reply-To: <87k0h6334w.fsf@email.froward.int.ebiederm.org>
On Wed, Nov 17, 2021 at 03:04:31PM -0600, Eric W. Biederman wrote:
> Kyle Huey <me@kylehuey.com> writes:
>
> > On Wed, Nov 17, 2021 at 11:05 AM Kyle Huey <me@kylehuey.com> wrote:
> >>
> >> On Wed, Nov 17, 2021 at 10:51 AM Kees Cook <keescook@chromium.org> wrote:
> >> >
> >> > On Wed, Nov 17, 2021 at 10:47:13AM -0800, Kyle Huey wrote:
> >> > > rr, a userspace record and replay debugger[0], is completely broken on
> >> > > 5.16rc1. I bisected this to 00b06da29cf9dc633cdba87acd3f57f4df3fd5c7.
> >> > >
> >> > > That patch makes two changes, it blocks sigaction from changing signal
> >> > > handlers once the kernel has decided to force the program to take a
> >> > > signal and it also stops notifying ptracers of the signal in the same
> >> > > circumstances. The latter behavior is just wrong. There's no reason
> >> > > that ptrace should not be able to observe and even change
> >> > > (non-SIGKILL) forced signals. It should be reverted.
> >> > >
> >> > > This behavior change is also observable in gdb. If you take a program
> >> > > that sets SIGSYS to SIG_IGN and then raises a SIGSYS via
> >> > > SECCOMP_RET_TRAP and run it under gdb on a good kernel gdb will stop
> >> > > when the SIGSYS is raised, let you inspect program state, etc. After
> >> > > the SA_IMMUTABLE change gdb won't stop until the program has already
> >> > > died of SIGSYS.
> >> >
> >> > Ah, hm, this was trying to fix the case where a program trips
> >> > SECCOMP_RET_KILL (which is a "fatal SIGSYS"), and had been unobservable
> >> > before. I guess the fix was too broad...
> >>
> >> Perhaps I don't understand precisely what you mean by this, but gdb's
> >> behavior for a program that is SECCOMP_RET_KILLed was not changed by
> >> this patch (the SIGSYS is not observed until after program exit before
> >> or after this change).
The SA_IMMUTABLE change was to deal with failures seen in the seccomp
test suite after the recent fatal signal refactoring. Mainly that a
process that should have effectively performed do_exit() was suddenly
visible to the tracer.
> > Ah, maybe that behavior changed in 5.15 (my "before" here is a 5.14
> > kernel). I would argue that the debugger seeing the SIGSYS for
> > SECCOMP_RET_KILL is desirable though ...
>
> This is definitely worth discussing, and probably in need of fixing (aka
> something in rr seems to have broken).
>
> We definitely need protection against the race with sigaction.
>
> The fundamental question becomes does it make sense and is it safe
> to allow a debugger to stop at, and possibly change these signals.
I have no problem with a debugger getting notified about a fatal
(SECCOMP_RET_KILL*-originated) SIGSYS. But whatever happens, the kernel
needs to make sure the process does not continue. (i.e. signal can't be
changed/removed/etc.)
> Stopping at something SA_IMMUTABLE as long as the signal is allowed to
> continue and kill the process when PTRACE_CONT happens seems harmless.
>
> Allowing the debugger to change the signal, or change it's handling
> I don't know.
Right -- I'm fine with a visibility change (the seccomp test suite is
just checking for various expected state machine changes across the
various signal/death cases: as long as it _dies_, that's what we want.
If a extra notification appears before it dies, that's okay, it just
needs the test suite to change).
> [...]
> Kees I am back to asking the question I had before I figured out
> SA_IMMUTABLE. Are there security concerns with debuggers intercepting
> SECCOMP_RET_KILL.
I see no problem with allowing a tracer to observe the signal, but the
signalled process must have no way to continue running. If we end up in
such a state, then a seccomp process with access to clone() and
ptrace() can escape the seccomp sandbox. This is why seccomp had been
using the big do_exit() hammer -- I really want to absolutely never have
a bug manifest with a bypassed SECCOMP_RET_KILL: having a completely
unavoidable "dying" state is needed.
--
Kees Cook
next prev parent reply other threads:[~2021-11-17 21:54 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-17 18:47 [REGRESSION] 5.16rc1: SA_IMMUTABLE breaks debuggers Kyle Huey
2021-11-17 18:51 ` Kees Cook
2021-11-17 19:05 ` Kyle Huey
2021-11-17 19:09 ` Kyle Huey
2021-11-17 21:04 ` Eric W. Biederman
2021-11-17 21:54 ` Kees Cook [this message]
2021-11-17 23:24 ` Linus Torvalds
2021-11-18 0:05 ` Kees Cook
2021-11-18 0:15 ` Linus Torvalds
2021-11-18 0:37 ` Kyle Huey
2021-11-18 1:11 ` Linus Torvalds
2021-11-18 1:20 ` Kyle Huey
2021-11-18 1:32 ` Kees Cook
2021-11-18 16:10 ` Eric W. Biederman
2021-11-19 16:07 ` Kyle Huey
2021-11-19 16:35 ` Kees Cook
2021-11-19 16:58 ` Kyle Huey
2021-11-18 21:58 ` [PATCH 0/2] SA_IMMUTABLE fixes Eric W. Biederman
2021-11-18 21:58 ` Eric W. Biederman
2021-11-18 22:04 ` [PATCH 1/2] signal: Don't always set SA_IMMUTABLE for forced signals Eric W. Biederman
2021-11-18 22:04 ` Eric W. Biederman
2021-11-18 23:52 ` Kees Cook
2021-11-18 23:52 ` Kees Cook
2021-11-18 23:54 ` Kees Cook
2021-11-18 23:54 ` Kees Cook
2021-11-19 15:08 ` Eric W. Biederman
2021-11-19 15:08 ` Eric W. Biederman
2021-11-19 1:13 ` Kyle Huey
2021-11-19 1:13 ` Kyle Huey
2021-11-19 15:03 ` Eric W. Biederman
2021-11-19 15:03 ` Eric W. Biederman
2021-11-18 22:05 ` [PATCH 2/2] signal: Replace force_fatal_sig with force_exit_sig when in doubt Eric W. Biederman
2021-11-18 22:05 ` Eric W. Biederman
2021-11-18 23:53 ` Kees Cook
2021-11-18 23:53 ` Kees Cook
2021-11-19 1:12 ` [PATCH 0/2] SA_IMMUTABLE fixes Kyle Huey
2021-11-19 1:12 ` Kyle Huey
2021-11-19 15:41 ` [GIT PULL] SA_IMMUTABLE fixes for v5.16-rc2 Eric W. Biederman
2021-11-19 15:41 ` Eric W. Biederman
2021-11-19 19:46 ` pr-tracker-bot
2021-11-19 19:46 ` pr-tracker-bot
2021-11-17 22:29 ` [REGRESSION] 5.16rc1: SA_IMMUTABLE breaks debuggers Kyle Huey
2021-11-18 5:43 ` Thorsten Leemhuis
2021-11-20 6:13 ` Thorsten Leemhuis
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=202111171341.41053845C3@keescook \
--to=keescook@chromium.org \
--cc=andrea.righi@canonical.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=ebiederm@xmission.com \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=me@kylehuey.com \
--cc=rocallahan@gmail.com \
--cc=shuah@kernel.org \
--cc=torvalds@linux-foundation.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.