Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   Ubuntu Kernel Team (http://www.linux-archive.org/ubuntu-kernel-team/)
-   -   Ack: UBUNTU SAUCE: apparmor: fix IRQ stack overflow (http://www.linux-archive.org/ubuntu-kernel-team/707570-ack-ubuntu-sauce-apparmor-fix-irq-stack-overflow.html)

Brad Figg 09-26-2012 04:12 PM

Ack: UBUNTU SAUCE: apparmor: fix IRQ stack overflow
 
On 09/26/2012 08:59 AM, Tim Gardner wrote:
> 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);
> }
>


--
Brad Figg brad.figg@canonical.com http://www.canonical.com

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

Colin Ian King 09-26-2012 04:14 PM

Ack: UBUNTU SAUCE: apparmor: fix IRQ stack overflow
 
On 26/09/12 16:59, Tim Gardner wrote:

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);
}


Acked-by: Colin Ian King <colin.king@canonical.com>

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

Colin Ian King 09-26-2012 04:16 PM

Ack: UBUNTU SAUCE: apparmor: fix IRQ stack overflow
 
On 26/09/12 17:11, Tim Gardner wrote:

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);
}


Acked-by: Colin Ian King <colin.king@canonical.com>


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

Brad Figg 09-26-2012 04:28 PM

Ack: UBUNTU SAUCE: apparmor: fix IRQ stack overflow
 
On 09/26/2012 09:11 AM, Tim Gardner wrote:
> 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);
> }
>


--
Brad Figg brad.figg@canonical.com http://www.canonical.com

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

Herton Ronaldo Krzesinski 09-26-2012 07:48 PM

Ack: UBUNTU SAUCE: apparmor: fix IRQ stack overflow
 
--
[]'s
Herton

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


All times are GMT. The time now is 09:18 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.