| Message ID | 1271142580-26555-3-git-send-email-john.johansen@canonical.com |
|---|---|
| State | Accepted |
| Delegated to: | Andy Whitcroft |
| Headers | show |
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; > } > > /**
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
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; } /**