Messages in this thread | | | Date | Tue, 5 Feb 2019 12:23:02 +0100 | From | Christian Brauner <> | Subject | Re: [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>
| |