Message ID | 1348680841-64116-1-git-send-email-tim.gardner@canonical.com |
---|---|
State | New |
Headers | show |
On 26/09/12 18:34, Tim Gardner wrote: > BugLink: http://bugs.launchpad.net/bugs/1056078 > > Profile replacement can cause a long chain of profiles to build up > that get freed in a cascading chain of free_profile calls. Because > free_profile is being called via aa_put_profile (and hence kref_put) > each profile free is done via what amounts to recursion. That is > free_profile indirectly calls free_profile on the next profile in the > chain via aa_put_profile. > > Break this recursion by directly walking the chain, and as long as > a profile is being freed because it has no more references continue > on to the next profile. This results in at most 2 levels of free_profile > being called. > > Signed-off-by: John Johansen <john.johansen@canonical.com> > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > security/apparmor/policy.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c > index e1db319..415b8a9 100644 > --- a/security/apparmor/policy.c > +++ b/security/apparmor/policy.c > @@ -659,6 +659,8 @@ fail: > */ > static void aa_free_profile(struct aa_profile *profile) > { > + struct aa_profile *p; > + > AA_DEBUG("%s(%p)\n", __func__, profile); > > if (!profile) > @@ -685,8 +687,28 @@ static void aa_free_profile(struct aa_profile *profile) > aa_free_sid(profile->sid); > aa_put_dfa(profile->xmatch); > > - if (profile->replacedby) > - aa_put_profile(profile->replacedby); > + /* put the profile reference, but not via put_profile/kref_put > + * replacedby can form a long chain that can result in cascading > + * frees that blows the stack lp#1056078. The long chain creation > + * should be addressed in profile replacement. > + * This just addresses recursion of free_profile causing the > + * stack to blow. > + */ > + for (p = profile->replacedby; p; ) { > + if (atomic_dec_and_test(&p->base.count.refcount)) { > + /* no more refs on p, grab its replacedby */ > + struct aa_profile *next = p->replacedby; > + /* break the chain */ > + p->replacedby = NULL; > + /* now free p, chain is broken */ > + aa_put_profile(p); > + > + /* follow up with next profile in the chain */ > + p = next; > + } else > + break; > + } > + > > kzfree(profile); > } > Acked-by: Colin Ian King <colin.king@canonical.com>
diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index e1db319..415b8a9 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -659,6 +659,8 @@ fail: */ static void aa_free_profile(struct aa_profile *profile) { + struct aa_profile *p; + AA_DEBUG("%s(%p)\n", __func__, profile); if (!profile) @@ -685,8 +687,28 @@ static void aa_free_profile(struct aa_profile *profile) aa_free_sid(profile->sid); aa_put_dfa(profile->xmatch); - if (profile->replacedby) - aa_put_profile(profile->replacedby); + /* put the profile reference, but not via put_profile/kref_put + * replacedby can form a long chain that can result in cascading + * frees that blows the stack lp#1056078. The long chain creation + * should be addressed in profile replacement. + * This just addresses recursion of free_profile causing the + * stack to blow. + */ + for (p = profile->replacedby; p; ) { + if (atomic_dec_and_test(&p->base.count.refcount)) { + /* no more refs on p, grab its replacedby */ + struct aa_profile *next = p->replacedby; + /* break the chain */ + p->replacedby = NULL; + /* now free p, chain is broken */ + aa_put_profile(p); + + /* follow up with next profile in the chain */ + p = next; + } else + break; + } + kzfree(profile); }