[PATCH][Quantal] UBUNTU SAUCE: apparmor: fix IRQ stack overflow

[John Johansen]

On 09/27/2012 05:17 AM, Tim Gardner wrote:
> On 09/27/2012 01:15 AM, John Johansen wrote:
>> This is a minimal patch to fix bug#1056078. The patch in the works for
>> upstream is larger and reworks the entire replacedby logic to prevent the
>> replacedby chains from happening, which will improve resource usage, but
>> it is not available yet. This can be reverted when it is.
>>
>> BugLink: http://bugs.launchpad.net/bugs/1056078
>>
>> Profile replacement can cause long chains 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_profile is nested (looks like recursion of free_profile). That is
>> free_profile indirectly calls free_profile on the next profile in the
>> chain via aa_put_profile.
>>
>> Break this nesting 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 at canonical.com>
>> ---
>>   security/apparmor/policy.c |   28 +++++++++++++++++++++++++++-
>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
>> index 27c8161..3f01fb7 100644
>> --- a/security/apparmor/policy.c
>> +++ b/security/apparmor/policy.c
>> @@ -724,6 +724,8 @@ fail:
>>    */
>>   static void free_profile(struct aa_profile *profile)
>>   {
>> +    struct aa_profile *p;
>> +
>>       AA_DEBUG("%s(%p)\n", __func__, profile);
>>
>>       if (!profile)
>> @@ -752,7 +754,31 @@ static void free_profile(struct aa_profile *profile)
>>       aa_put_dfa(profile->xmatch);
>>       aa_put_dfa(profile->policy.dfa);
>>
>> -    aa_put_profile(profile->replacedby);
>> +    /* put the profile reference for replacedby, but not via
>> +     * put_profile(kref_put).
>> +     * replacedby can form a long chain that can result in cascading
>> +     * frees that blows the stack because kref_put makes a nested fn
>> +     * call (it looks like recursion, with free_profile calling
>> +     * free_profile) for each profile in the chain lp#1056078. The
>> +     * long chain creation should be addressed by changes to profile
>> +     * replacement.
>> +     * This just addresses recursion of free_profile causing the
>> +     * stack to blow, when a chain is encountered.
>> +     */
>> +    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 */
>> +            free_profile(p);
>> +
>> +            /* follow up with next profile in the chain */
>> +            p = next;
>> +        } else
>> +            break;
>> +    }
>>
>>       kzfree(profile);
>>   }
>>
> 
> John - other then a bit of commit log difference, isn't this patch identical to the one already applied to Quantal/Precise/Oneiric ? I also did a backport to Lucid which I'd appreciate you having a look at (its applied to Lucid master-next).
> 
yes,
sorry I saw the Lucid, Oneiric and Precise ones got by, but didn't see it on Quantal.  Looks like when I fetched I only reset my master and not master-next.

The Lucid backport looks good

thanks Tim