Re: [PATCH] signal: always allocate siginfo for SI_TKILL

[ebiederm@xmission.com (Eric W. Biederman)]

Kees Cook <keescook@chromium.org> writes:

> On Sun, Feb 3, 2019 at 12:39 AM Christian Brauner <christian@brauner.io> wrote:
>>
>> On Sat, Feb 02, 2019 at 09:49:38PM -1000, Jack Andersen wrote:
>> > The patch titled
>> > `signal: Never allocate siginfo for SIGKILL or SIGSTOP`
>> > created a regression for users of PTRACE_GETSIGINFO needing to
>> > discern signals that were raised via the tgkill syscall.
>> >
>> > A notable user of this tgkill+ptrace combination is lldb while
>> > debugging a multithreaded program. Without the ability to detect a
>> > SIGSTOP originating from tgkill, lldb does not have a way to
>> > synchronize on a per-thread basis and falls back to SIGSTOP-ing the
>> > entire process.
>> >
>> > This patch allocates the siginfo as it did previously whenever the
>> > SI_TKILL code is present.
>> >
>> > Signed-off-by: Jack Andersen <jackoalan@gmail.com>
>>
>> The  commit you're trying to fix has been discussed before wrt to
>> seccomp tests:
>>
>> commit 2bd61abead58c82714a1f6fa6beb0fd0df6a6d13
>> Author: Kees Cook <keescook@chromium.org>
>> Date:   Thu Dec 6 15:50:38 2018 -0800
>>
>>     selftests/seccomp: Remove SIGSTOP si_pid check
>>
>>     Commit f149b3155744 ("signal: Never allocate siginfo for SIGKILL or SIGSTOP")
>>     means that the seccomp selftest cannot check si_pid under SIGSTOP anymore.
>>     Since it's believed[1] there are no other userspace things depending on the
>>     old behavior, this removes the behavioral check in the selftest, since it's
>>     more a "extra" sanity check (which turns out, maybe, not to have been
>>     useful to test).
>>
>>     [1] https://lkml.kernel.org/r/CAGXu5jJaZAOzP1qFz66tYrtbuywqb+UN2SOA1VLHpCCOiYvYeg@mail.gmail.com
>>
>>     Reported-by: Tycho Andersen <tycho@tycho.ws>
>>     Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
>>     Signed-off-by: Kees Cook <keescook@chromium.org>
>>     Signed-off-by: Shuah Khan <shuah@kernel.org>
>>
>> Ccing Kees on this. Seems that this commit might be worth given that
>> there's some parts of userspace relying on it and not just internal
>> kernel tests.
>
> Yup, so this is the "real" userspace example that Eric was looking for.

Yes it is.

> Eric, how does the proposed fix look? I'd also like to revert my
> seccomp selftest change too, since it clearly found a real-world use.
> :)

I think the simpler change to just do:

diff --git a/kernel/signal.c b/kernel/signal.c
index e1d7ad8e6ab1..45298b3a8ffc 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1057,10 +1057,10 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
 
        result = TRACE_SIGNAL_DELIVERED;
        /*
-        * Skip useless siginfo allocation for SIGKILL SIGSTOP,
+        * Skip useless siginfo allocation for SIGKILL,
         * and kernel threads.
         */
-       if (sig_kernel_only(sig) || (t->flags & PF_KTHREAD))
+       if ((sig == SIGKILL) || (t->flags & PF_KTHREAD))
                goto out_set;
 
        /*

is the better fix.  As Christian points out that fixes possible
issues with SIGSTOP.

For the ldd use case I do want to suggest that using PTRACE_INTERRUPT
may be a better solution.

Using a signal to stop an individual thread has the problem that
someone else can send that SIGSTOP and siginfo will not be allocated.
Further for the second time a signal is sent we can not implement a
siginfo allocation for historic unix signals because that would cause
the signal to be delivered multiple times instead of just once.

AKA there are unfixable races with using SIGSTOP to stop thread for
debugging.

Eric