From: Kyle Huey <email@example.com> To: Linus Torvalds <firstname.lastname@example.org> Cc: Kees Cook <email@example.com>, "Eric W. Biederman" <firstname.lastname@example.org>, Andrea Righi <email@example.com>, Shuah Khan <firstname.lastname@example.org>, Alexei Starovoitov <email@example.com>, Andy Lutomirski <firstname.lastname@example.org>, Will Drewry <email@example.com>, "open list:KERNEL SELFTEST FRAMEWORK" <firstname.lastname@example.org>, bpf <email@example.com>, open list <firstname.lastname@example.org>, email@example.com, "Robert O'Callahan" <firstname.lastname@example.org> Subject: Re: [REGRESSION] 5.16rc1: SA_IMMUTABLE breaks debuggers Date: Wed, 17 Nov 2021 17:20:33 -0800 [thread overview] Message-ID: <CAP045AoqssLTKOqse1t1DG1HgK9h+goG8C3sqgOyOV3Wwq+LDA@mail.gmail.com> (raw) In-Reply-To: <CAHk-=wiCRbSvUi_TnQkokLeM==_+Tow0GsQXnV3UYwhsxirPwg@mail.gmail.com> On Wed, Nov 17, 2021 at 5:11 PM Linus Torvalds <email@example.com> wrote: > > On Wed, Nov 17, 2021 at 4:37 PM Kyle Huey <firstname.lastname@example.org> wrote: > > > > This fixes most of the issues with rr, but it still changes the ptrace > > behavior for the double-SIGSEGV case > > Hmm. I think that's because of how "force_sigsgv()" works. > > I absolutely detest that function. > > So we have signal_setup_done() doing that > > if (failed) > force_sigsegv(ksig->sig); > > and then force_sigsegv() has that completely insane > > if (sig == SIGSEGV) > force_fatal_sig(SIGSEGV); > else > force_sig(SIGSEGV); > > behavior. > > And I think I know the _reason_ for that complete insanity: when > SIGSEGV takes a SIGSEGV, and there is a handler, we need to stop > trying to send more SIGSEGV's. Right, in our test we setup a SIGSEGV handler on an alt stack that doesn't actually exist, and then overflow the regular stack, and that would loop forever trying to setup SIGSEGV handlers if it weren't for force_sigsegv and the sigdfl=true stuff. > But it does mean that with my change, that second SIGSEGV now ends up > being that SA_IMMUTABLE kind, so yeah, it broke the debugger test - > where catching the second SIGSEGV is actually somewhat sensible (ok, > not really, but at least understandable) > > End result: I think we want not a boolean, but a three-way choice for > that force_sig_info_to_task() thing: with the following clarifications, yes > - unconditionally fatal (for things that just want to force an exit > and used to do do_exit()) no matter what the ptracer wants > - ignore valid and unblocked handler (for that SIGSEGV recursion > case, aka force "sigdfl") but following the usual ptrace rules > - catching signal ok > > So my one-liner isn't sufficient. It wants some kind of nasty enum. > > At least the enum can be entirely internal to kernel/signal.c, I > think. No need to expose this all to anything else. > > Linus Yeah that's one way to solve the problem. I think you're right that fundamentally the problem here is that what SECCOMP_RET_KILL wants is not really a signal. To the extent that it wants a signal, what it really wants is SIGKILL, and the problem here is the code trying to act like SIGKILL but call it SIGSYS. I assume the ship for fixing that sailed years ago though. - Kyle
next prev parent reply other threads:[~2021-11-18 1:21 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-11-17 18:47 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 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 [this message] 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 22:04 ` [PATCH 1/2] signal: Don't always set SA_IMMUTABLE for forced signals Eric W. Biederman 2021-11-18 23:52 ` Kees Cook 2021-11-18 23:54 ` Kees Cook 2021-11-19 15:08 ` Eric W. Biederman 2021-11-19 1:13 ` Kyle Huey 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 23:53 ` Kees Cook 2021-11-19 1:12 ` [PATCH 0/2] SA_IMMUTABLE fixes Kyle Huey 2021-11-19 15:41 ` [GIT PULL] SA_IMMUTABLE fixes for v5.16-rc2 Eric W. Biederman 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=CAP045AoqssLTKOqse1t1DG1HgK9h+goG8C3sqgOyOV3Wwq+LDA@mail.gmail.com \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [REGRESSION] 5.16rc1: SA_IMMUTABLE breaks debuggers' \ /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
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).