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

[Christian Brauner <christian@brauner.io>]

On Sun, Feb 03, 2019 at 10:33:55AM -0800, Kees Cook wrote:
> 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.
> 
> Eric, how does the proposed fix look? I'd also like to revert my

I think Eric's proposal in [1] is better. Iiuc, the proposed fix here
doesn't restore the behavior for seccomp. I'd rather have you or Jack
pick up Eric's fix with a Suggested-by for Eric in there and then
restore the old behavior. I'd feel comfortable acking this.
Once we have that you can revert the seccomp fix you sent. Later we can
think about Eric's proposal to make siginfo_t allocation more robust.

Christian

[1]: https://lore.kernel.org/lkml/87zhtthkuy.fsf@xmission.com/