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


--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

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

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

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

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

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


--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

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

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

Applied

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team