Re: [PATCH 7/7] ptrace: s/tracehook_tracer_task()/ptrace_parent()/

[John Johansen <john.johansen@canonical.com>]

On 06/20/2011 01:16 PM, Oleg Nesterov wrote:
> On 06/17, Tejun Heo wrote:
>>
>> tracehook.h is on the way out.  Rename tracehook_tracer_task() to
>> ptrace_parent() and move it from tracehook.h to ptrace.h.
> 
> I am a bit surpised you decided to keep this helper. Can't we simply
> kill it?
> 
> OK, we will see. I guess this change is mostly needed to remove yet
> another function from tracehook.h.
> 
>> @@ -216,7 +216,7 @@ static struct mm_struct *__check_mem_permission(struct task_struct *task)
>>  	if (task_is_stopped_or_traced(task)) {
>>  		int match;
>>  		rcu_read_lock();
>> -		match = (tracehook_tracer_task(task) == current);
>> +		match = (ptrace_parent(task) == current);
>>  		rcu_read_unlock();
>>  		if (match && ptrace_may_access(task, PTRACE_MODE_ATTACH))
> 
> All we need
> 
> 	if (task_is_traced(task) && task->parent == current) {
> 		if (ptrace_may_access()
> 			return mm;
> 	}
> 
> Of course I do not blame this patch, my only point is that this helper
> only adds more confusion imho.
> 
> 
> 
> 
>> @@ -67,7 +67,7 @@ static int may_change_ptraced_domain(struct task_struct *task,
>>  	int error = 0;
>>
>>  	rcu_read_lock();
>> -	tracer = tracehook_tracer_task(task);
>> +	tracer = ptrace_parent(task);
>>  	if (tracer) {
>>  		/* released below */
>>  		cred = get_task_cred(tracer);
> 
> Hmm. And then this task_struct is used after we dropped rcu_read_lock().
> 
> John, is this correct?
> 
nope this use is wrong.  The following patch should fix this

===

AppArmor: Fix reference to rcu protected pointer outside of rcu_read_lock

The pointer returned from tracehook_tracer_task() is only valid inside
the rcu_read_lock.  However the tracer pointer obtained is being passed
to aa_may_ptrace outside of the rcu_read_lock critical section.

Mover the aa_may_ptrace test into the rcu_read_lock critical section, to
fix this.

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/domain.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index c825c6e..78adc43 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -73,7 +73,6 @@ static int may_change_ptraced_domain(struct task_struct *task,
 		cred = get_task_cred(tracer);
 		tracerp = aa_cred_profile(cred);
 	}
-	rcu_read_unlock();
 
 	/* not ptraced */
 	if (!tracer || unconfined(tracerp))
@@ -82,6 +81,7 @@ static int may_change_ptraced_domain(struct task_struct *task,
 	error = aa_may_ptrace(tracer, tracerp, to_profile, PTRACE_MODE_ATTACH);
 
 out:
+	rcu_read_unlock();
 	if (cred)
 		put_cred(cred);
 
-- 
1.7.4.1