Re: siginfo pid not populated from ptrace?

[ebiederm@xmission.com (Eric W. Biederman)]

Kees Cook <keescook@chromium.org> writes:

> On Thu, Dec 6, 2018 at 1:11 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> Tycho Andersen <tycho@tycho.ws> writes:
>>
>> > On Thu, Dec 06, 2018 at 10:48:39AM -0800, Linus Torvalds wrote:
>> >> On Thu, Dec 6, 2018 at 6:40 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>> >> >
>> >> > We have in the past had ptrace users that weren't just about debugging
>> >> > so I don't know that it is fair to just dismiss it as debugging
>> >> > infrastructure.
>> >>
>> >> Absolutely.
>> >>
>> >> Some uses are more than just debug. People occasionally use ptrace
>> >> because it's the only way to do what they want, so you'll find people
>> >> who do it for sandboxing, for example. It's not necessarily designed
>> >> for that, or particularly fast or well-suited for it, but I've
>> >> definitely seen it used that way.
>> >>
>> >> So I don't think the behavioral test breakage like this is necessarily
>> >> a huge deal, and until some "real use" actually shows that it cares it
>> >> might be something we dismiss as "just test", but it very much has the
>> >> potential to hit real uses.
>> >>
>> >> The fact that a behavioral test broke is definitely interesting.
>> >>
>> >> And maybe some of the siginfo allocations could depend on whether the
>> >> signal is actually ever caught or not.
>> >>
>> >> For example, a terminal signal (or one that is ignored) might not need
>> >> siginfo. But if the process is ptraced, maybe that terminal signal
>> >> isn't actually terminal? So we might have situations where we want to
>> >> simply check "is the signal target being ptraced"..
>> >
>> > Yes, something like this, I suppose? It works for me.
>>
>> The challenge is that we could be delivering this to a zombie signal
>> group leader.  At which point we won't deliver it to the target task.
>>
>> Sigh it is probably time that I dig in and figure out how to avoid that
>> case which we need to fix anyway because we can get the permission
>> checks wrong for multi-threaded processes that call setuid and friends.
>>
>> Once that is sorted your small change will at least be safe.
>>
>> Eric
>>
>> > From 3bcaadd56ebb532ab4d481556fcc0826d65efc43 Mon Sep 17 00:00:00 2001
>> > From: Tycho Andersen <tycho@tycho.ws>
>> > Date: Thu, 6 Dec 2018 12:15:22 -0700
>> > Subject: [PATCH] signal: allocate siginfo when a traced task gets SIGSTOP
>> >
>> > Tracers can view SIGSTOP:
>> >
>> > https://lore.kernel.org/lkml/87zhtthkuy.fsf@xmission.com/T/#u
>> >
>> > so let's allocate a siginfo for SIGSTOP when a task is traced.
>> >
>> > Signed-off-by: Tycho Andersen <tycho@tycho.ws>
>> > ---
>> >  kernel/signal.c | 9 ++++++---
>> >  1 file changed, 6 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/kernel/signal.c b/kernel/signal.c
>> > index 9a32bc2088c9..ab4ba00119f4 100644
>> > --- a/kernel/signal.c
>> > +++ b/kernel/signal.c
>> > @@ -1056,11 +1056,14 @@ static int __send_signal(int sig, struct kernel_siginfo *info, struct task_struc
>> >               goto ret;
>> >
>> >       result = TRACE_SIGNAL_DELIVERED;
>> > +
>> >       /*
>> > -      * Skip useless siginfo allocation for SIGKILL SIGSTOP,
>> > -      * and kernel threads.
>> > +      * Skip useless siginfo allocation for SIGKILL and kernel threads.
>> > +      * SIGSTOP is visible to tracers, so only skip allocation when the task
>> > +      * is not traced.
>> >        */
>> > -     if (sig_kernel_only(sig) || (t->flags & PF_KTHREAD))
>> > +     if ((sig == SIGKILL) || (!task_is_traced(t) && sig == SIGSTOP) ||
>> > +         (t->flags & PF_KTHREAD))
>> >               goto out_set;
>> >
>> >       /*
>
> What should we do for v4.20? I need to have the selftests actually
> passing. :)

For v4.20 we need to do one of two things.
1) Present a plausible case that someone will could care about,
   we document it in the commit we can perform my earlier partial revert.

2) Remove the sanity check seccomp_bpf.c

I really just want to ensure we have clear reasoning here.

Eric