lkml.org 
[lkml]   [2019]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] signal: always allocate siginfo for SI_TKILL
On Mon, Feb 04, 2019 at 08:41:15PM -0600, Eric W. Biederman wrote:
> 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.

Yep. That looks good! I take it you'll be sending this out then. :)
Thanks Eric!

Reviewed-by: Christian Brauner <christian@brauner.io>

\
 
 \ /
  Last update: 2019-02-05 12:25    [W:0.084 / U:0.588 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site