linux-kernel - Re: [PATCH] signal: always allocate siginfo for SI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 5 Feb 2019 12:23:02 +0100
From:   Christian Brauner <christian@...uner.io>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Kees Cook <keescook@...omium.org>,
        Jack Andersen <jackoalan@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Oleg Nesterov <oleg@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
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@...omium.org> writes:
> 
> > On Sun, Feb 3, 2019 at 12:39 AM Christian Brauner <christian@...uner.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@...il.com>
> >>
> >> The  commit you're trying to fix has been discussed before wrt to
> >> seccomp tests:
> >>
> >> commit 2bd61abead58c82714a1f6fa6beb0fd0df6a6d13
> >> Author: Kees Cook <keescook@...omium.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@...ho.ws>
> >>     Suggested-by: Eric W. Biederman <ebiederm@...ssion.com>
> >>     Signed-off-by: Kees Cook <keescook@...omium.org>
> >>     Signed-off-by: Shuah Khan <shuah@...nel.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@...uner.io>