Patchwork [02/11] AppArmor: Fix refcount bug when exec fails

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

The error case for ptrace permission on exec missed putting the new_profile,
fix this ommission and consolidate the error cases to a single point.

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/domain.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)
Stefan Bader - April 13, 2010, 8:08 a.m.
Looks ok, though jumping forth and back feels a bit "unclean"

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: af11e86b3b91f0aaf5155e73d0d3f196124b25e2
> BugLink: http://bugs.launchpad.net/bugs/562063
> 
> The error case for ptrace permission on exec missed putting the new_profile,
> fix this ommission and consolidate the error cases to a single point.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  security/apparmor/domain.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index cd8ec99..2721fcb 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -424,10 +424,8 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  	if (!new_profile)
>  		goto audit;
>  
> -	if (profile == new_profile) {
> -		aa_put_profile(new_profile);
> -		goto audit;
> -	}
> +	if (profile == new_profile)
> +		goto abort;
>  
>  	if (bprm->unsafe & LSM_UNSAFE_SHARE) {
>  		/* FIXME: currently don't mediate shared state */
> @@ -438,7 +436,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  		sa.base.error = aa_may_change_ptraced_domain(current,
>  							     new_profile);
>  		if (sa.base.error)
> -			goto audit;
> +			goto abort;
>  	}
>  
>  	/* Determine if secure exec is needed.
> @@ -485,6 +483,10 @@ cleanup:
>  	kfree(buffer);
>  
>  	return sa.base.error;
> +
> +abort:
> +	aa_put_profile(new_profile);
> +	goto audit;
>  }
>  
>  /**
Andy Whitcroft - April 13, 2010, 8:52 a.m.
On Tue, Apr 13, 2010 at 12:09:31AM -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: af11e86b3b91f0aaf5155e73d0d3f196124b25e2
> BugLink: http://bugs.launchpad.net/bugs/562063
> 
> The error case for ptrace permission on exec missed putting the new_profile,
> fix this ommission and consolidate the error cases to a single point.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  security/apparmor/domain.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index cd8ec99..2721fcb 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -424,10 +424,8 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  	if (!new_profile)
>  		goto audit;
>  
> -	if (profile == new_profile) {
> -		aa_put_profile(new_profile);
> -		goto audit;
> -	}
> +	if (profile == new_profile)
> +		goto abort;
>  
>  	if (bprm->unsafe & LSM_UNSAFE_SHARE) {
>  		/* FIXME: currently don't mediate shared state */
> @@ -438,7 +436,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  		sa.base.error = aa_may_change_ptraced_domain(current,
>  							     new_profile);
>  		if (sa.base.error)
> -			goto audit;
> +			goto abort;
>  	}
>  
>  	/* Determine if secure exec is needed.
> @@ -485,6 +483,10 @@ cleanup:
>  	kfree(buffer);
>  
>  	return sa.base.error;
> +
> +abort:
> +	aa_put_profile(new_profile);
> +	goto audit;
>  }

This is a little ugly, but I can see why you want to consolidate them.

>  
>  /**

This routine is generally vile and jumptastic already, so I guess its
not making it any worse.  For a patch this late I would prefer something
vile to a big cleanup.  On balance:

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

-apw

Patch

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index cd8ec99..2721fcb 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -424,10 +424,8 @@  int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 	if (!new_profile)
 		goto audit;
 
-	if (profile == new_profile) {
-		aa_put_profile(new_profile);
-		goto audit;
-	}
+	if (profile == new_profile)
+		goto abort;
 
 	if (bprm->unsafe & LSM_UNSAFE_SHARE) {
 		/* FIXME: currently don't mediate shared state */
@@ -438,7 +436,7 @@  int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 		sa.base.error = aa_may_change_ptraced_domain(current,
 							     new_profile);
 		if (sa.base.error)
-			goto audit;
+			goto abort;
 	}
 
 	/* Determine if secure exec is needed.
@@ -485,6 +483,10 @@  cleanup:
 	kfree(buffer);
 
 	return sa.base.error;
+
+abort:
+	aa_put_profile(new_profile);
+	goto audit;
 }
 
 /**