[Precise,SRU] UBUNTU SAUCE: apparmor: fix IRQ stack overflow

From: John Johansen <john.johansen@canonical.com>

BugLink: http://bugs.launchpad.net/bugs/1056078

Profile replacement can cause a long chain of profiles to build up
that get freed in a cascading chain of free_profile calls. Because
free_profile is being called via aa_put_profile (and hence kref_put)
each profile free is done via what amounts to recursion. That is
free_profile indirectly calls free_profile on the next profile in the
chain via aa_put_profile.

Break this recursion by directly walking the chain, and as long as
a profile is being freed because it has no more references continue
on to the next profile. This results in at most 2 levels of free_profile
being called.

Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 security/apparmor/policy.c |   24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

On 09/26/2012 08:59 AM, Tim Gardner wrote:
> From: John Johansen <john.johansen@canonical.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/1056078
> 
> Profile replacement can cause a long chain of profiles to build up
> that get freed in a cascading chain of free_profile calls. Because
> free_profile is being called via aa_put_profile (and hence kref_put)
> each profile free is done via what amounts to recursion. That is
> free_profile indirectly calls free_profile on the next profile in the
> chain via aa_put_profile.
> 
> Break this recursion by directly walking the chain, and as long as
> a profile is being freed because it has no more references continue
> on to the next profile. This results in at most 2 levels of free_profile
> being called.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
>  security/apparmor/policy.c |   24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
> index 4a5e1b5..93faca0 100644
> --- a/security/apparmor/policy.c
> +++ b/security/apparmor/policy.c
> @@ -724,6 +724,8 @@ fail:
>   */
>  static void free_profile(struct aa_profile *profile)
>  {
> +	struct aa_profile *p;
> +
>  	AA_DEBUG("%s(%p)\n", __func__, profile);
>  
>  	if (!profile)
> @@ -752,7 +754,27 @@ static void free_profile(struct aa_profile *profile)
>  	aa_put_dfa(profile->xmatch);
>  	aa_put_dfa(profile->policy.dfa);
>  
> -	aa_put_profile(profile->replacedby);
> +	/* put the profile reference, but not via put_profile/kref_put
> +	 * replacedby can form a long chain that can result in cascading
> +	 * frees that blows the stack lp#1056078. The long chain creation
> +	 * should be addressed in profile replacement.
> +	 * This just addresses recursion of free_profile causing the
> +	 * stack to blow.
> +	 */
> +	for (p = profile->replacedby; p; ) {
> +		if (atomic_dec_and_test(&p->base.count.refcount)) {
> +			/* no more refs on p, grab its replacedby */
> +			struct aa_profile *next = p->replacedby;
> +			/* break the chain */
> +			p->replacedby = NULL;
> +			/* now free p, chain is broken */
> +			free_profile(p);
> +
> +			/* follow up with next profile in the chain */
> +			p = next;
> +		} else
> +			break;
> +	}
>  
>  	kzfree(profile);
>  }
>

On 26/09/12 16:59, Tim Gardner wrote:
> From: John Johansen <john.johansen@canonical.com>
>
> BugLink: http://bugs.launchpad.net/bugs/1056078
>
> Profile replacement can cause a long chain of profiles to build up
> that get freed in a cascading chain of free_profile calls. Because
> free_profile is being called via aa_put_profile (and hence kref_put)
> each profile free is done via what amounts to recursion. That is
> free_profile indirectly calls free_profile on the next profile in the
> chain via aa_put_profile.
>
> Break this recursion by directly walking the chain, and as long as
> a profile is being freed because it has no more references continue
> on to the next profile. This results in at most 2 levels of free_profile
> being called.
>
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
>   security/apparmor/policy.c |   24 +++++++++++++++++++++++-
>   1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
> index 4a5e1b5..93faca0 100644
> --- a/security/apparmor/policy.c
> +++ b/security/apparmor/policy.c
> @@ -724,6 +724,8 @@ fail:
>    */
>   static void free_profile(struct aa_profile *profile)
>   {
> +	struct aa_profile *p;
> +
>   	AA_DEBUG("%s(%p)\n", __func__, profile);
>
>   	if (!profile)
> @@ -752,7 +754,27 @@ static void free_profile(struct aa_profile *profile)
>   	aa_put_dfa(profile->xmatch);
>   	aa_put_dfa(profile->policy.dfa);
>
> -	aa_put_profile(profile->replacedby);
> +	/* put the profile reference, but not via put_profile/kref_put
> +	 * replacedby can form a long chain that can result in cascading
> +	 * frees that blows the stack lp#1056078. The long chain creation
> +	 * should be addressed in profile replacement.
> +	 * This just addresses recursion of free_profile causing the
> +	 * stack to blow.
> +	 */
> +	for (p = profile->replacedby; p; ) {
> +		if (atomic_dec_and_test(&p->base.count.refcount)) {
> +			/* no more refs on p, grab its replacedby */
> +			struct aa_profile *next = p->replacedby;
> +			/* break the chain */
> +			p->replacedby = NULL;
> +			/* now free p, chain is broken */
> +			free_profile(p);
> +
> +			/* follow up with next profile in the chain */
> +			p = next;
> +		} else
> +			break;
> +	}
>
>   	kzfree(profile);
>   }
>
Acked-by: Colin Ian King <colin.king@canonical.com>