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

[Kees Cook]

On Tue, Feb 5, 2019 at 2:17 PM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxx>
> Reported-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Reported-by: Jack Andersen <jackoalan@xxxxxxxxx>
> Acked-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Christian Brauner <christian@xxxxxxxxxx>
> 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@xxxxxxxxxxxx>

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