Message-ID: <87o87ctmvw.fsf@disp2133>
Date:   Mon, 25 Oct 2021 23:45:23 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andy Lutomirski <luto@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH 13/20] signal: Implement force_fatal_sig

Linus Torvalds <torvalds@...ux-foundation.org> writes:

> On Mon, Oct 25, 2021 at 3:41 PM Andy Lutomirski <luto@...nel.org> wrote:
>>
>> I'm rather nervous about all this, and I'm also nervous about the
>> existing code.  A quick skim is finding plenty of code paths that assume
>> force_sigsegv (or a do_exit that this series touches) are genuinely
>> unrecoverable.
>
> I was going to say "what are you talking about", because clearly Eric
> kept it all fatal.
>
> But then looked at that patch a bit more before I claimed you were wrong.
>
> And yeah, Eric's force_fatal_sig() is completely broken.
>
> It claims to force a fatal signal, but doesn't actually do that at
> all, and is completely misnamed.
>
> It just uses "force_sig_info_to_task()", which still allows user space
> to catch signals - so it's not "fatal" in the least. It only punches
> through SIG_IGN and blocked signals.
>
> So yeah, that's broken.
>
> I do still think that that could the behavior we possibly want for
> that "can't write updated vm86 state back" situation, but for
> something that is called "fatal", it really needs to be fatal.

Once the code gets as far as force_sig_info_to_task the only
bit that is really missing is to make the signals fatal is:

diff --git a/kernel/signal.c b/kernel/signal.c
index 6a5e1802b9a2..fde043f1e59d 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1048,7 +1048,6 @@ static void complete_signal(int sig, struct task_struct *p, enum pid_type type)
                /*
                 * This signal will be fatal to the whole group.
                 */
-               if (!sig_kernel_coredump(sig)) {
                        /*
                         * Start a group exit and wake everybody up.
                         * This way we don't have other threads
@@ -1065,7 +1064,6 @@ static void complete_signal(int sig, struct task_struct *p, enum pid_type type)
                                signal_wake_up(t, 1);
                        } while_each_thread(p, t);
                        return;
-               }
        }
 
        /*

AKA the only real bit missing is the interaction with the coredump code.

Now we can't just delete sig_kernel_coredump a replacement has to be
written.   And the easiest replacement depends on my other set of
changes that are already in linux-next to make coredumps per
signal_struct instead of per mm.

Which means that in a release or two force_fatal_sig will reliably do
what the name says.




So the question is: Should I name force_fatal_sig to something else in
the meantime?  What should I name it?




I do intend to fix that bit in complete_signal, as well as updating the
code in force_siginfo_to_task so that it doesn't need to change the
blocked state or the signal handler.

These special cases have been annoying me for years and now Andy has
found how they are actually hurting us.  So I do intend to fix that code
as quickly as being careful and code review allows.  Which I think means
one additional development cycle after this one.

Eric

Powered by blists - more mailing lists