Q: proc: hold cred_guard_mutex in check_mem_permission()

Q: proc: hold cred_guard_mutex in check_mem_permission()

[Oleg Nesterov]

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?

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.

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.

Oleg.

--- x/fs/proc/base.c
+++ x/fs/proc/base.c
@@ -194,38 +194,31 @@ static int proc_root_link(struct inode *
 	return result;
 }
 
-static struct mm_struct *__check_mem_permission(struct task_struct *task)
+static int __check_mem_permission(struct task_struct *task)
 {
-	struct mm_struct *mm;
-
-	mm = get_task_mm(task);
-	if (!mm)
-		return ERR_PTR(-EINVAL);
-
 	/*
 	 * A task can always look at itself, in case it chooses
 	 * to use system calls instead of load instructions.
 	 */
 	if (task == current)
-		return mm;
+		return 0;
 
 	/*
 	 * If current is actively ptrace'ing, and would also be
 	 * permitted to freshly attach with ptrace now, permit it.
 	 */
-	if (task_is_stopped_or_traced(task)) {
+	if (task_is_traced(task)) {
 		int match;
 		rcu_read_lock();
 		match = (ptrace_parent(task) == current);
 		rcu_read_unlock();
 		if (match && ptrace_may_access(task, PTRACE_MODE_ATTACH))
-			return mm;
+			return 0;
 	}
 
 	/*
 	 * No one else is allowed.
 	 */
-	mmput(mm);
 	return ERR_PTR(-EPERM);
 }
 
@@ -238,18 +231,11 @@ static struct mm_struct *check_mem_permi
 	struct mm_struct *mm;
 	int err;
 
-	/*
-	 * Avoid racing if task exec's as we might get a new mm but validate
-	 * against old credentials.
-	 */
-	err = mutex_lock_killable(&task->signal->cred_guard_mutex);
+	err = __check_mem_permission(task);
 	if (err)
 		return ERR_PTR(err);
 
-	mm = __check_mem_permission(task);
-	mutex_unlock(&task->signal->cred_guard_mutex);
-
-	return mm;
+	return get_task_mm(task) ?: ERR_PTR(-EINVAL);
 }
 
 struct mm_struct *mm_for_maps(struct task_struct *task)

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

[Stephen Wilson]

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

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

[Oleg Nesterov]

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.

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

[Stephen Wilson]

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

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

[Oleg Nesterov]

On 09/29, Stephen Wilson wrote:
>
> On Thu, Sep 29, 2011 at 01:48:27PM +0200, Oleg Nesterov wrote:
>
> > So I think we should simply remove ->cred_guard_mutex.
>
> Yes, I think that is right,

Great,

> together with removing the
> ptrace_may_access() check

Agreed, but this needs a separate patch.

Plus rcu_read_lock + ptrace_parent() should die, we simply need

	task->ptrace && task->parent == current

And task_is_stopped_or_traced() should be task_is_traced(), starting
from 3.0 TASK_STOPPED && ptrace is not possible.

I'll send the patches tomorrow.

Thanks,

Oleg.