FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Ubuntu > Ubuntu Kernel Team

 
 
LinkBack Thread Tools
 
Old 09-26-2012, 03:59 PM
Tim Gardner
 
Default UBUNTU SAUCE: apparmor: fix IRQ stack overflow

From: John Johansen <john.johansen@canonical.com>

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 | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index 4a5e1b5..93faca0 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)
", __func__, profile);

if (!profile)
@@ -752,7 +754,27 @@ 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, 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 */
+ free_profile(p);
+
+ /* follow up with next profile in the chain */
+ p = next;
+ } else
+ break;
+ }

kzfree(profile);
}
--
1.7.9.5


--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 09-26-2012, 04:11 PM
Tim Gardner
 
Default UBUNTU SAUCE: apparmor: fix IRQ stack overflow

From: John Johansen <john.johansen@canonical.com>

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 | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index 4d5ce13..b5c5b8b 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)
", __func__, profile);

if (!profile)
@@ -751,7 +753,27 @@ static void free_profile(struct aa_profile *profile)
aa_free_sid(profile->sid);
aa_put_dfa(profile->xmatch);

- 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 */
+ free_profile(p);
+
+ /* follow up with next profile in the chain */
+ p = next;
+ } else
+ break;
+ }

kzfree(profile);
}
--
1.7.9.5


--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 09-26-2012, 05:34 PM
Tim Gardner
 
Default UBUNTU SAUCE: apparmor: fix IRQ stack overflow

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)
", __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);
}
--
1.7.9.5


--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 09-27-2012, 07:15 AM
John Johansen
 
Default UBUNTU SAUCE: apparmor: fix IRQ stack overflow

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@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)
", __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);
}
--
1.7.10.4


--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 09-27-2012, 07:25 AM
John Johansen
 
Default UBUNTU SAUCE: apparmor: fix IRQ stack overflow

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@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)
", __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);
}
--
1.7.10.4


--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 09-27-2012, 12:17 PM
Tim Gardner
 
Default UBUNTU SAUCE: apparmor: fix IRQ stack overflow

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@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)
", __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).


rtg
--
Tim Gardner tim.gardner@canonical.com

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 09-27-2012, 05:18 PM
John Johansen
 
Default UBUNTU SAUCE: apparmor: fix IRQ stack overflow

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@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)
", __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



--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 

Thread Tools




All times are GMT. The time now is 07:17 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org