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

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(-)

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);

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

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

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);

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

Applied