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

[Oleg Nesterov <oleg@redhat.com>]

On 09/29, Stephen Wilson wrote:
>
> 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?

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.

> > 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,

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.

> > 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?

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?

So I think we should simply remove ->cred_guard_mutex.

Oleg.