Message-ID: <CAF52+S6Y40K-sRPf4YAb8ceAmtgBzZ9OUbyhPTOc_AcOWuQ=Ng@mail.gmail.com>
Date:	Tue, 1 Apr 2014 13:44:15 -0700
From:	Matthew Dempsky <mdempsky@...omium.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Kees Cook <keescook@...omium.org>,
	Julien Tinnes <jln@...omium.org>,
	Roland McGrath <mcgrathr@...omium.org>,
	Jan Kratochvil <jan.kratochvil@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] Fix ptrace events across pid namespaces

On Tue, Apr 1, 2014 at 11:52 AM, Oleg Nesterov <oleg@...hat.com> wrote:
> Sorry for slow response,

No worries!

>> But how can I guard against current->parent disappearing?  Do I need
>> to grab a read lock on the tasklist and rcu?
>
> rcu_read_lock() should be enough.

Noted.

> The main question is how we can ensure that we use the correct namespace,
> iow, how we can ensure that current->parent won't change after this.
>
> Suppose that debugger detaches right after we calculated pid_t we were
> going to report.
>
> Now suppose that another task from the different namespace does
> ptrace(PTRACE_SEIZE, PTRACE_O_TRACEFORK). Or PTRACE_O_TRACEVFORKDONE.
>
> In this case ptrace_event() will still report the wrong pid. I simply do
> not see how we can prevent this race without the really ugly complications.

Yeah, the 'best' alternative I can think of is to save the "struct pid
*" instead of a pid_t in the ptrace_message field, and then have
PTRACE_GETEVENTMSG handle translating it into a pid_t (similar to what
SCM_CREDENTIALS does)... but that seems very messy and intrusive for
such an unlikely situation (because we need extra bits to distinguish
the contents of ptrace_message and to free up references).

> But perhaps we do not really care? The race is very unlikely, and nothing
> really bad can happen. So perhaps your simple patch makes sense anyway,
> just the problem should be documented. Currently "nr" is always wrong if
> the tracer is from another ns, this change will obviously makes the things
> better in any case.

Yeah, I'm leaning towards just accepting that the ptrace event message
might be wrong in this case, and documenting that.  It's at least no
worse than the status quo where it's wrong in all cases.  I'll
followup with a v3 patch for this in a bit.

>> Basically, if process A is being ptraced by process B and we record an
>> event for it, I don't want process B to detach and then process C to
>> attach and be able to still view the event.
>
> Aha, so I got it right. But see above, this won't help anyway, C can
> attach before ptrace_event() sets ->ptrace_message.
>
> Perhaps this change makes sense anyway. I think it needs a separate patch
> but I won't insist.
>
>> > Probably this is related to detach/attach, but then it would be better
>> > to do this in PTRACE_ATTACH. Because ->ptrace_message can be non-zero
>> > if, say, a traced task forks.
>>
>> I think zeroing out ptrace_message at attach time works too, and I can
>> do it that way if you prefer.
>
> I do not really mind. But once again, unless we clear it in PTRACE_ATTACH,
> PTRACE_GETEVENTMSG still can return non-zero right after PTRACE_ATTACH.
> Of course we can change copy_process() to clear child->ptrace_message,
> but I don't think this would be better.

I think I'll punt on zero'ing ptrace_message for now and propose that
as a followup patch then.  I think it's a really narrow and unlikely
issue (like the detach/attach race above), and I don't want to
distract from the more pressing matter of getting debugging working.
:)

> Matthew, I just noticed we discuss this off-list, I added lkml.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists