linux-kernel - Re: [PATCH] signal: Always attempt to allocate siginfo for SIGSTOP

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 14:49:02 +0000
From:   Kees Cook <keescook@...omium.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Christian Brauner <christian@...uner.io>,
        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>
Subject: Re: [PATCH] signal: Always attempt to allocate siginfo for SIGSTOP

On Tue, Feb 5, 2019 at 2:17 PM Eric W. Biederman <ebiederm@...ssion.com> wrote:
>
>
> Since 2.5.34 the code has had the potential to not allocate siginfo
> for SIGSTOP signals.  Except for ptrace this is perfectly fine as only
> ptrace can use PTRACE_PEEK_SIGINFO and see what the contents of
> the delivered siginfo are.
>
> Users of PTRACE_PEEK_SIGINFO that care about the contents siginfo
> for SIGSTOP are rare, but they do exist.  A seccomp self test
> has cared and lldb cares.
>
> Jack Andersen <jackoalan@...il.com> writes:
>
> > 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.
>
> Everyone affected by this please note.  The kernel can still fail to
> allocate a siginfo structure.  The allocation is with GFP_KERNEL and
> is best effort only.  If memory is tight when the signal allocation
> comes in this will fail to allocate a siginfo.
>
> So I strongly recommend looking at more robust solutions for
> synchronizing with a single thread such as PTRACE_INTERRUPT.  Or if
> that does not work persuading your friendly local kernel developer to
> build the interface you need.
>
> Reported-by: Tycho Andersen <tycho@...ho.ws>
> Reported-by: Kees Cook <keescook@...omium.org>
> Reported-by: Jack Andersen <jackoalan@...il.com>
> Acked-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Reviewed-by: Christian Brauner <christian@...uner.io>
> Fixes: f149b3155744 ("signal: Never allocate siginfo for SIGKILL or SIGSTOP")
> Fixes: 6dfc88977e42 ("[PATCH] shared thread signals")
> History Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>

Cc: stable too?

In your -next tree (and the pull), can you also revert 2bd61abead58
("selftests/seccomp: Remove SIGSTOP si_pid check") too?

Thanks for getting this in!

-Kees

> ---
>
> Unless someone objects I will drop this in my tree for linux-next and
> send Linus a pull request in a couple of days.
>
>  kernel/signal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> 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;
>
>         /*
> --
> 2.17.1
>


-- 
Kees Cook