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

login
register
mail settings
Submitter John Johansen
Date April 13, 2010, 7:09 a.m.
Message ID <1271142580-26555-8-git-send-email-john.johansen@canonical.com>
Download mbox | patch
Permalink /patch/50041/
State Accepted
Delegated to: Andy Whitcroft
Headers show

Comments

John Johansen - April 13, 2010, 7:09 a.m.
From: John Johansen <john.johansen@canonical.com>

OriginalAuthor: John Johansen <john.johansen@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@canonical.com>
---
 security/apparmor/context.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)
Andy Whitcroft - April 13, 2010, 9:14 a.m.
On Tue, Apr 13, 2010 at 12:09:36AM -0700, john.johansen@canonical.com wrote:
> From: John Johansen <john.johansen@canonical.com>
> 
> OriginalAuthor: John Johansen <john.johansen@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@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@canonical.com>

-apw

Patch

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;