Patchwork [4/5] UBUNTU: SAUCE: AppArmor: Fix refcounting bug causing leak of creds

login
register
mail settings
Submitter John Johansen
Date Nov. 10, 2009, 6:29 p.m.
Message ID <1257877753-9448-5-git-send-email-john.johansen@canonical.com>
Download mbox | patch
Permalink /patch/38070/
State Accepted
Headers show

Comments

John Johansen - Nov. 10, 2009, 6:29 p.m.
BugLink: http://bugs.launchpad.net/bugs/479115

SRU Justification: Failure to put the cred causes a memory leak that is
larger than the cred struct, as it leaks everything it references. This
happens for every unconfined processes that does an exec, change_hat or
change_profile and passes through this function.

AppArmor when doing ptrace check for domain changes, fails to drop
the ref count on the task creds when it is unconfined.

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 ubuntu/apparmor/domain.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Stefan Bader - Nov. 11, 2009, 1:24 p.m.
Sounds and looks sensible. Also implications sound SRU worthy.

John Johansen wrote:
> BugLink: http://bugs.launchpad.net/bugs/479115
> 
> SRU Justification: Failure to put the cred causes a memory leak that is
> larger than the cred struct, as it leaks everything it references. This
> happens for every unconfined processes that does an exec, change_hat or
> change_profile and passes through this function.
> 
> AppArmor when doing ptrace check for domain changes, fails to drop
> the ref count on the task creds when it is unconfined.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>

Acked-by: Stefan Bader <stefan.bader@canonical.com>

> ---
>  ubuntu/apparmor/domain.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c
> index 128e527..fe89ddc 100644
> --- a/ubuntu/apparmor/domain.c
> +++ b/ubuntu/apparmor/domain.c
> @@ -65,9 +65,10 @@ static int aa_may_change_ptraced_domain(struct task_struct *task,
>  	rcu_read_unlock();
>  
>  	if (!tracerp)
> -		return error;
> +		goto out;
>  
>  	error = aa_may_ptrace(tracer, tracerp, to_profile, PTRACE_MODE_ATTACH);
> +out:
>  	put_cred(cred);
>  
>  	return error;
Andy Whitcroft - Nov. 12, 2009, 11:25 a.m.
On Tue, Nov 10, 2009 at 10:29:12AM -0800, John Johansen wrote:
> BugLink: http://bugs.launchpad.net/bugs/479115
> 
> SRU Justification: Failure to put the cred causes a memory leak that is
> larger than the cred struct, as it leaks everything it references. This
> happens for every unconfined processes that does an exec, change_hat or
> change_profile and passes through this function.
> 
> AppArmor when doing ptrace check for domain changes, fails to drop
> the ref count on the task creds when it is unconfined.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  ubuntu/apparmor/domain.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c
> index 128e527..fe89ddc 100644
> --- a/ubuntu/apparmor/domain.c
> +++ b/ubuntu/apparmor/domain.c
> @@ -65,9 +65,10 @@ static int aa_may_change_ptraced_domain(struct task_struct *task,
>  	rcu_read_unlock();
>  
>  	if (!tracerp)
> -		return error;
> +		goto out;
>  
>  	error = aa_may_ptrace(tracer, tracerp, to_profile, PTRACE_MODE_ATTACH);
> +out:
>  	put_cred(cred);
>  
>  	return error;

Ok, the change itself looks fine if we have taken the cred reference we
should return it.

I just want to confirm that aa_get_task_policy() cannot return a valid
tracerp but a null cred?  Else the put_cred() might oops.  A quick look
at that seems to say its ok.  Assuming that is true:

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw
Stefan Bader - Nov. 12, 2009, 1:38 p.m.
Applied
John Johansen - Nov. 12, 2009, 4:24 p.m.
Andy Whitcroft wrote:
> On Tue, Nov 10, 2009 at 10:29:12AM -0800, John Johansen wrote:
>> BugLink: http://bugs.launchpad.net/bugs/479115
>>
>> SRU Justification: Failure to put the cred causes a memory leak that is
>> larger than the cred struct, as it leaks everything it references. This
>> happens for every unconfined processes that does an exec, change_hat or
>> change_profile and passes through this function.
>>
>> AppArmor when doing ptrace check for domain changes, fails to drop
>> the ref count on the task creds when it is unconfined.
>>
>> Signed-off-by: John Johansen <john.johansen@canonical.com>
>> ---
>>  ubuntu/apparmor/domain.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c
>> index 128e527..fe89ddc 100644
>> --- a/ubuntu/apparmor/domain.c
>> +++ b/ubuntu/apparmor/domain.c
>> @@ -65,9 +65,10 @@ static int aa_may_change_ptraced_domain(struct task_struct *task,
>>  	rcu_read_unlock();
>>  
>>  	if (!tracerp)
>> -		return error;
>> +		goto out;
>>  
>>  	error = aa_may_ptrace(tracer, tracerp, to_profile, PTRACE_MODE_ATTACH);
>> +out:
>>  	put_cred(cred);
>>  
>>  	return error;
> 
> Ok, the change itself looks fine if we have taken the cred reference we
> should return it.
> 
> I just want to confirm that aa_get_task_policy() cannot return a valid
> tracerp but a null cred?  Else the put_cred() might oops.  A quick look
> at that seems to say its ok.  Assuming that is true:
> 
Well you have already, seen Patch 5 so know that it might oops.  The reason
for the split is I was trying to treat the as distinct bugs, and keep the
fixes distinct and minimal.

john

Patch

diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c
index 128e527..fe89ddc 100644
--- a/ubuntu/apparmor/domain.c
+++ b/ubuntu/apparmor/domain.c
@@ -65,9 +65,10 @@  static int aa_may_change_ptraced_domain(struct task_struct *task,
 	rcu_read_unlock();
 
 	if (!tracerp)
-		return error;
+		goto out;
 
 	error = aa_may_ptrace(tracer, tracerp, to_profile, PTRACE_MODE_ATTACH);
+out:
 	put_cred(cred);
 
 	return error;