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

[Stephen Wilson <wilsons@start.ca>]

On Wed, Sep 28, 2011 at 10:20:20PM +0200, Oleg Nesterov wrote:
> Another change we probably need to backport, 18f661bc
> "proc: hold cred_guard_mutex in check_mem_permission()".
> 
> From the changelog:
> 
>     Avoid a potential race when task exec's and we get a new ->mm but
>     check against the old credentials in ptrace_may_access().
> 
> 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? 


> This task is either current, or it is TASK_TRACED and we are the
> tracer. In the latter case nobody can resume it except SIGKILL.
> And the killed task obviously can't exec.
> 
> Afaics, all we need is: we should read task->mm after the
> task_is_traced() check, we do not need the mutex.

Checking ptrace_parent() against current was introduced in 0d094efeb,  but the
commit message gives no clue as to why the check was added.  It does seem to
go against the comment "would also be permitted to freshly attach with
ptrace".  Not sure what to think ATM..

> IOW, what do you think about the patch below? I have no idea how
> can I test it (and it wasn't even applied/compiled).
>
> Also, I'd appreciate if you can explain the PTRACE_MODE_ATTACH
> check. Again, we are already the tracer.

If we are the tracer then that ptrace_may_access() check is redundant and the
whole race thing is a non-issue, right?  But perhaps the correct move is to
relax the restriction that current be the tracer.  I might be overlooking
something though.


Thanks,


-- 
steve