Re: Q: proc: hold cred_guard_mutex in check_mem_permission()

Stephen Wilson
2011-09-30T01:05:48Z

On Thu, Sep 29, 2011 at 01:48:27PM +0200, Oleg Nesterov wrote:
> > > Could you please explain? How can we race with exec?
> >
> > My understanding of the race is this:
> >
> > sequence during execve():
> >
> > 1) cred_guard_mutex is taken in prepare_bprm_creds()
> > 2) new mm is installed via exec_mmap()
> > 3) new creds are pushed via install_exec_creds() which releases
> > cred_guard_mutex
> >
> > so if we get_task_mm() and ptrace_may_access() between 2 and 3 we
> > obtain a reference to the new mm validated against old creds.
> >
> > Perhaps (the fairly old) commit 704b836c helps?
>
> Yes, and that is why I sent 704b836c ;)
>
> But, check_mem_permission() can't race with exec, that was my point.
> The task is stopped (it is TASK_TRACED), it can't run unless SIGKILL'ed.
> It can not change its mm/creds.

Ah, I see. I misunderstood your question. So, yes, the race as it
stands is fictional AFAIKT -- I did not recognize that the
ptrace_may_access() was effectively redundant when I first authored the
patch.

> > Checking ptrace_parent() against current was introduced in 0d094efeb,
>
> not actually, this is very old check, probably since 2.4.0 at least.
> That patch only renames the helper we use.
>
> > but the
> > commit message gives no clue as to why the check was added.
>
> Only debugger can read/write the task's memory. May be we can relax
> this, perhaps we can only check ptrace_may_access(PTRACE_MODE_ATTACH).
>
> But currently we require the caller should trace the target.

OK.

> > If we are the tracer then that ptrace_may_access() check is redundant and the
> > whole race thing is a non-issue, right?
>
> Yes.
>
> > But perhaps the correct move is to
> > relax the restriction that current be the tracer.
>
> Yes, I thought about this too. And in this case we do need the mutex.
> Although I don't think this really makes sense, I _think_ this all was
> created for debuggers. And, without ptrace_parent(), why do we need
> task_is_stopped_or_traced() check?

Thinking about this some more, I agree that this should remain only for
debuggers -- /proc/pid/mem is just a nicer interface to ptrace peek and
poke, nothing more.

> So I think we should simply remove ->cred_guard_mutex.

Yes, I think that is right, together with removing the
ptrace_may_access() check and updating that comment in
check_mem_permission(). I can put a patch together this weekend if
needed.

Take care,

--
steve