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

[Stephen Wilson <wilsons@start.ca>]

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