Patchwork [03/11] AppArmor: Take refcount on cxt->profile to ensure it remains a valid reference

login
register
mail settings
Submitter John Johansen
Date April 13, 2010, 7:09 a.m.
Message ID <1271142580-26555-4-git-send-email-john.johansen@canonical.com>
Download mbox | patch
Permalink /patch/50037/
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: ea35fcdcec2316aa57f2d9d5c9ed9002d90b9607
BugLink: http://bugs.launchpad.net/bugs/367499

A reference for a profile is not obtained from aa_newest_version(cxt->profile)
this would normally be valid as cxt->profile will hold a reference keeping
the profile valid, however cxt->profiles reference is replaced with the
new_profile reference
	aa_put_profile(cxt->profile);
	/* transfer new profile reference will be released when cxt is freed */
	cxt->profile = new_profile;

before we are done referencing profile in
        aa_audit_file(profile,

so obtain a reference to profile for the duration of the function.

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/domain.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Andy Whitcroft - April 13, 2010, 8:57 a.m.
On Tue, Apr 13, 2010 at 12:09:32AM -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: ea35fcdcec2316aa57f2d9d5c9ed9002d90b9607
> BugLink: http://bugs.launchpad.net/bugs/367499
> 
> A reference for a profile is not obtained from aa_newest_version(cxt->profile)
> this would normally be valid as cxt->profile will hold a reference keeping
> the profile valid, however cxt->profiles reference is replaced with the
> new_profile reference
> 	aa_put_profile(cxt->profile);
> 	/* transfer new profile reference will be released when cxt is freed */
> 	cxt->profile = new_profile;
> 
> before we are done referencing profile in
>         aa_audit_file(profile,
> 
> so obtain a reference to profile for the duration of the function.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  security/apparmor/domain.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 2721fcb..7af454f 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -350,7 +350,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  	cxt = bprm->cred->security;
>  	BUG_ON(!cxt);
>  
> -	profile = aa_newest_version(cxt->profile);
> +	profile = aa_get_profile(aa_newest_version(cxt->profile));
>  	/*
>  	 * get the namespace from the replacement profile as replacement
>  	 * can change the namespace
> @@ -480,6 +480,7 @@ audit:
>  	sa.base.error = aa_audit_file(profile, &sa);
>  
>  cleanup:
> +	aa_put_profile(profile);
>  	kfree(buffer);
>  
>  	return sa.base.error;

Seems reasonable:

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

-apw

Patch

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 2721fcb..7af454f 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -350,7 +350,7 @@  int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 	cxt = bprm->cred->security;
 	BUG_ON(!cxt);
 
-	profile = aa_newest_version(cxt->profile);
+	profile = aa_get_profile(aa_newest_version(cxt->profile));
 	/*
 	 * get the namespace from the replacement profile as replacement
 	 * can change the namespace
@@ -480,6 +480,7 @@  audit:
 	sa.base.error = aa_audit_file(profile, &sa);
 
 cleanup:
+	aa_put_profile(profile);
 	kfree(buffer);
 
 	return sa.base.error;