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