[PATCH 07/11] AppArmor: fix refcount order bug that can trigger during replacement

[Andy Whitcroft]

On Tue, Apr 13, 2010 at 12:09:36AM -0700, john.johansen at canonical.com wrote:
> From: John Johansen <john.johansen at canonical.com>
> 
> OriginalAuthor: John Johansen <john.johansen at canonical.com>
> OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
> commit: b96b66094d397cdf887abb77850e075c2d99b91b
> BugLink: http://bugs.launchpad.net/bugs/367499
> 
> AppArmor uses lazy reference counting on chains of reference to reduce
> refcounting overhead.  When loading a replacement profile races with
> a task replacing the cred's current reference, it is possible that
> cxt->profile is the reference at the head of the replacement chain,
> dropping this reference can actually trigger the loss of the last
> profile in the chain if there are no more references to it.
> 
> Signed-off-by: John Johansen <john.johansen at canonical.com>
> ---
>  security/apparmor/context.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/security/apparmor/context.c b/security/apparmor/context.c
> index caaa277..14a0e41 100644
> --- a/security/apparmor/context.c
> +++ b/security/apparmor/context.c
> @@ -91,8 +91,13 @@ static void replace_group(struct aa_task_cxt *cxt, struct aa_profile *profile)
>  		cxt->onexec = NULL;
>  		cxt->token = 0;
>  	}
> +	/* be careful switching cxt->profile, when racing replacement it
> +	 * is possible that cxt->profile->replacedby is the reference keeping
> +	 * @profile valid, so make sure to get its reference before dropping
> +	 * the reference on cxt->profile */
> +	aa_get_profile(profile);
>  	aa_put_profile(cxt->profile);
> -	cxt->profile = aa_get_profile(profile);
> +	cxt->profile = profile;
>  }
>  
>  /**
> @@ -130,8 +135,9 @@ int aa_set_current_onexec(struct aa_profile *profile)
>  		return -ENOMEM;
>  
>  	cxt = new->security;
> +	aa_get_profile(profile);
>  	aa_put_profile(cxt->onexec);
> -	cxt->onexec = aa_get_profile(profile);
> +	cxt->onexec = profile;
>  
>  	commit_creds(new);
>  	return 0;

Looks to do what it claims:

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

-apw