On 10/11, Roland McGrath wrote:
>
> > Issues with ptrace_check_attach(),
> >
> > - it does utrace_control(UTRACE_STOP).
> >
> > This is wrong, ptrace_check_attach() must be "passive",
> > while utrace_control(UTRACE_STOP) can actualy stop the
> > tracee.
>
> This is not inherently problematic on its own. I'd say it's OK if you
> have to actually be the tracer task to get that far, and of course if
> you call utrace_control(UTRACE_RESUME) to reverse it in the failure
> case. The UTRACE_STOP itself does not have any effect that the tracee
> can observe except as scheduling flutter. (I'm not really counting it
> cooperating with another process that does "ps" and sees "T"--we can
> live with that too.) If the actual tracer making stupid use of ptrace
> calls that will wind up failing with ESRCH has the side effect of
> lousing up the scheduling of the tracee it's failing to otherwise
> molest, so be it.
Not sure I understand... This is behaviour change, no?
Suppose a tracee is running, but for some reason it is somewhere
inside start_callback(). utrace_control(UTRACE_STOP) can change ->flags
and provoke report->action = UTRACE_STOP. In this case the tracee will
stop.
IOW. The tracee is running, say, it does "for (;;) ;". If the tracer
does
for (;;)
ptrace(PTRACE_CONT);
it can stop the tracee.
As I said, I think this is minor. But I never know which behaviour
changes are important.
> It's a security issue if you can do UTRACE_STOP on a task that you would
> not be allowed to ptrace, even for an instant.
I don't think we have security issues, ptrace_check_attach() is only
called when we already ptrace the task. It is a bit racy, we should
check child->parent after ptrace_lookup_engine(), but this is trivial.
(and in fact this race doesn't exist because of lock_kernl(), but
sys_ptrace()->lock_kernel() must die).
> > - even if it doesn't, this is wrong when we have multiple
> > tracers. ptrace_check_attach() should not succeed if the
> > tracee was stopped by another engine.
>
> Yes. UTRACE_STOP state is necessary for ptrace to consider the tracee
> to be "in ptrace stop", but it's not sufficient. Ensuring UTRACE_STOP
> is the mechanism for the underlying material effect that "in ptrace
> stop" means, i.e. in (or on the way into) TASK_TRACED until SIGKILL.
> Since we use UTRACE_STOP in report returns to implement ptrace stops,
> this is normally already the case in almost all the situations where
> ptrace_check_attach should succeed. The only reason we have to use
> utrace_control to ensure that's the case is for a tracee in TASK_STOPPED
> (job control stop), which we have to convert into being a guaranteed
> ptrace stop SIGCONT won't wake (i.e. UTRACE_STOP, i.e. TASK_TRACED).
>
> So, we use the UTRACE_STOP interlock for that purpose. But it's not
> what we should use for "are we in a ptrace stop?" bookkeeping, which
> previously was implied directly by task_is_traced(). What might make
> sense as the indicator of "in ptrace stop" is "ctx->stop_code != 0" or
> "ctx->resume == UTRACE_STOP".
We already have the indicator - get_stop_event(). Except it doesn't work
for jctl stops. ptrace_check_attach() needs something like:
if (!ptrace_event_pending()) {
do not change engine->flags,
do not set utrace->report,
just s/TASK_STOPPED/TASK_TRACED/ or fail
}
> > - it ignores "int kill".
>
> That's for PTRACE_KILL. This is a story similar to your question about
> PTRACE_DETACH,SIGKILL. That is, it's actually pretty useless but we
> still don't want to break some arcane assumptions.
Yes, yes, I see.
> > Can we ignore these problems in V1 ? Or should we fix something?
>
> It doesn't seem like it should be very hard to get this right from the
> start. I don't think we can submit something upstream that has any
> known regressions even in stupid cases, except the explicit intended
> "improvements" that we will document.
PTRACE_KILL is trivial. Other problems are not completely trivial,
afaics, but I didn't really try to think.
OK. We should fix this, I'll try to do this later. Right now we have
more important work.
Oleg.
