Patchwork [1/5] UBUNTU: SAUCE: AppArmor: Fix oops after profile removal

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

Comments

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

SRU Justicication: this bug can cause a null pointer dereference kernel
oops. This will occur any time children profiles are attached to running
processes. This can occur when change_hat, children profiles or profile
learning is used.
---
 ubuntu/apparmor/policy.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Stefan Bader - Nov. 11, 2009, 1:14 p.m.
So the problem arises as aa_profile_newest() would return NULL when being
called with replacedby being an error_ptr. And aa_profile_newest() is often
called as producer of the argument to aa_filtered_profile() which tries to
access profile->flags without checking for a NULL pointer.
When using a generic profile (I just assume it is) instead,
is there danger of accidentally dropping it in free_aa_profile()?

        if (profile->replacedby && !PTR_ERR(profile->replacedby))
                aa_put_profile(profile->replacedby);

Stefan

John Johansen wrote:
> BugLink: http://bugs.launchpad.net/bugs/475619
> 
> SRU Justicication: this bug can cause a null pointer dereference kernel
> oops. This will occur any time children profiles are attached to running
> processes. This can occur when change_hat, children profiles or profile
> learning is used.
> ---
>  ubuntu/apparmor/policy.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/ubuntu/apparmor/policy.c b/ubuntu/apparmor/policy.c
> index 390bbf6..a772801 100644
> --- a/ubuntu/apparmor/policy.c
> +++ b/ubuntu/apparmor/policy.c
> @@ -322,7 +322,7 @@ void __aa_remove_profile(struct aa_profile *profile,
>  	if (replacement)
>  		profile->replacedby = aa_get_profile(replacement);
>  	else
> -		profile->replacedby = ERR_PTR(-EINVAL);
> +		profile->replacedby = aa_get_profile(profile->ns->unconfined);
>  	list_del_init(&profile->base.list);
>  	if (!(profile->flags & PFLAG_NO_LIST_REF))
>  		aa_put_profile(profile);
John Johansen - Nov. 11, 2009, 4:11 p.m.
Stefan Bader wrote:
> So the problem arises as aa_profile_newest() would return NULL when being
> called with replacedby being an error_ptr. And aa_profile_newest() is often
> called as producer of the argument to aa_filtered_profile() which tries to
> access profile->flags without checking for a NULL pointer.
> When using a generic profile (I just assume it is) instead,
> is there danger of accidentally dropping it in free_aa_profile()?
> 
>         if (profile->replacedby && !PTR_ERR(profile->replacedby))
>                 aa_put_profile(profile->replacedby);
> 
No, I could have dropped that in the patch as well as the PTR_ERR stuff in newest profile.  There is always a valid profile present most of the time the namespace->unconfined profile.  It used to be that NULL or ERR_PTR was used but that didn't allow tracking of which namespace a task was in.

I went for the absolute minimum patch for karmic and cleaned up the PTR_ERR in the upstream version, I can post that instead if you would like.

john
Stefan Bader - Nov. 11, 2009, 4:17 p.m.
John Johansen wrote:
> Stefan Bader wrote:
>> So the problem arises as aa_profile_newest() would return NULL when being
>> called with replacedby being an error_ptr. And aa_profile_newest() is often
>> called as producer of the argument to aa_filtered_profile() which tries to
>> access profile->flags without checking for a NULL pointer.
>> When using a generic profile (I just assume it is) instead,
>> is there danger of accidentally dropping it in free_aa_profile()?
>>
>>         if (profile->replacedby && !PTR_ERR(profile->replacedby))
>>                 aa_put_profile(profile->replacedby);
>>
> No, I could have dropped that in the patch as well as the PTR_ERR stuff in newest profile.  There is always a valid profile present most of the time the namespace->unconfined profile.  It used to be that NULL or ERR_PTR was used but that didn't allow tracking of which namespace a task was in.
> 
> I went for the absolute minimum patch for karmic and cleaned up the PTR_ERR in the upstream version, I can post that instead if you would like.
> 
No, not necessary. I just wanted to make sure you see no danger in freeing a profile which has te
unconfined profile in its replacedby value and the unconfined profile gets inappropriately freed.

Stefan
Stefan Bader - Nov. 11, 2009, 4:24 p.m.
Ok, after making sure the reset of the code is happy with it.

John Johansen wrote:
> BugLink: http://bugs.launchpad.net/bugs/475619
> 
> SRU Justicication: this bug can cause a null pointer dereference kernel
> oops. This will occur any time children profiles are attached to running
> processes. This can occur when change_hat, children profiles or profile
> learning is used.

Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>

> ---
>  ubuntu/apparmor/policy.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/ubuntu/apparmor/policy.c b/ubuntu/apparmor/policy.c
> index 390bbf6..a772801 100644
> --- a/ubuntu/apparmor/policy.c
> +++ b/ubuntu/apparmor/policy.c
> @@ -322,7 +322,7 @@ void __aa_remove_profile(struct aa_profile *profile,
>  	if (replacement)
>  		profile->replacedby = aa_get_profile(replacement);
>  	else
> -		profile->replacedby = ERR_PTR(-EINVAL);
> +		profile->replacedby = aa_get_profile(profile->ns->unconfined);
>  	list_del_init(&profile->base.list);
>  	if (!(profile->flags & PFLAG_NO_LIST_REF))
>  		aa_put_profile(profile);
Andy Whitcroft - Nov. 12, 2009, 1:06 p.m.
On Tue, Nov 10, 2009 at 10:29:09AM -0800, John Johansen wrote:
> BugLink: http://bugs.launchpad.net/bugs/475619
> 
> SRU Justicication: this bug can cause a null pointer dereference kernel
> oops. This will occur any time children profiles are attached to running
> processes. This can occur when change_hat, children profiles or profile
> learning is used.
> ---
>  ubuntu/apparmor/policy.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/ubuntu/apparmor/policy.c b/ubuntu/apparmor/policy.c
> index 390bbf6..a772801 100644
> --- a/ubuntu/apparmor/policy.c
> +++ b/ubuntu/apparmor/policy.c
> @@ -322,7 +322,7 @@ void __aa_remove_profile(struct aa_profile *profile,
>  	if (replacement)
>  		profile->replacedby = aa_get_profile(replacement);
>  	else
> -		profile->replacedby = ERR_PTR(-EINVAL);
> +		profile->replacedby = aa_get_profile(profile->ns->unconfined);
>  	list_del_init(&profile->base.list);
>  	if (!(profile->flags & PFLAG_NO_LIST_REF))
>  		aa_put_profile(profile);
> -- 

Based on JJ's replies to smb I think this one is ok.

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

-apw
Stefan Bader - Nov. 12, 2009, 1:37 p.m.
Applied

Patch

diff --git a/ubuntu/apparmor/policy.c b/ubuntu/apparmor/policy.c
index 390bbf6..a772801 100644
--- a/ubuntu/apparmor/policy.c
+++ b/ubuntu/apparmor/policy.c
@@ -322,7 +322,7 @@  void __aa_remove_profile(struct aa_profile *profile,
 	if (replacement)
 		profile->replacedby = aa_get_profile(replacement);
 	else
-		profile->replacedby = ERR_PTR(-EINVAL);
+		profile->replacedby = aa_get_profile(profile->ns->unconfined);
 	list_del_init(&profile->base.list);
 	if (!(profile->flags & PFLAG_NO_LIST_REF))
 		aa_put_profile(profile);