| Message ID | 1257877753-9448-5-git-send-email-john.johansen@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
Sounds and looks sensible. Also implications sound SRU worthy. John Johansen wrote: > BugLink: http://bugs.launchpad.net/bugs/479115 > > SRU Justification: Failure to put the cred causes a memory leak that is > larger than the cred struct, as it leaks everything it references. This > happens for every unconfined processes that does an exec, change_hat or > change_profile and passes through this function. > > AppArmor when doing ptrace check for domain changes, fails to drop > the ref count on the task creds when it is unconfined. > > Signed-off-by: John Johansen <john.johansen@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > ubuntu/apparmor/domain.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c > index 128e527..fe89ddc 100644 > --- a/ubuntu/apparmor/domain.c > +++ b/ubuntu/apparmor/domain.c > @@ -65,9 +65,10 @@ static int aa_may_change_ptraced_domain(struct task_struct *task, > rcu_read_unlock(); > > if (!tracerp) > - return error; > + goto out; > > error = aa_may_ptrace(tracer, tracerp, to_profile, PTRACE_MODE_ATTACH); > +out: > put_cred(cred); > > return error;
On Tue, Nov 10, 2009 at 10:29:12AM -0800, John Johansen wrote: > BugLink: http://bugs.launchpad.net/bugs/479115 > > SRU Justification: Failure to put the cred causes a memory leak that is > larger than the cred struct, as it leaks everything it references. This > happens for every unconfined processes that does an exec, change_hat or > change_profile and passes through this function. > > AppArmor when doing ptrace check for domain changes, fails to drop > the ref count on the task creds when it is unconfined. > > Signed-off-by: John Johansen <john.johansen@canonical.com> > --- > ubuntu/apparmor/domain.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c > index 128e527..fe89ddc 100644 > --- a/ubuntu/apparmor/domain.c > +++ b/ubuntu/apparmor/domain.c > @@ -65,9 +65,10 @@ static int aa_may_change_ptraced_domain(struct task_struct *task, > rcu_read_unlock(); > > if (!tracerp) > - return error; > + goto out; > > error = aa_may_ptrace(tracer, tracerp, to_profile, PTRACE_MODE_ATTACH); > +out: > put_cred(cred); > > return error; Ok, the change itself looks fine if we have taken the cred reference we should return it. I just want to confirm that aa_get_task_policy() cannot return a valid tracerp but a null cred? Else the put_cred() might oops. A quick look at that seems to say its ok. Assuming that is true: Acked-by: Andy Whitcroft <apw@canonical.com> -apw
Applied
Andy Whitcroft wrote: > On Tue, Nov 10, 2009 at 10:29:12AM -0800, John Johansen wrote: >> BugLink: http://bugs.launchpad.net/bugs/479115 >> >> SRU Justification: Failure to put the cred causes a memory leak that is >> larger than the cred struct, as it leaks everything it references. This >> happens for every unconfined processes that does an exec, change_hat or >> change_profile and passes through this function. >> >> AppArmor when doing ptrace check for domain changes, fails to drop >> the ref count on the task creds when it is unconfined. >> >> Signed-off-by: John Johansen <john.johansen@canonical.com> >> --- >> ubuntu/apparmor/domain.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c >> index 128e527..fe89ddc 100644 >> --- a/ubuntu/apparmor/domain.c >> +++ b/ubuntu/apparmor/domain.c >> @@ -65,9 +65,10 @@ static int aa_may_change_ptraced_domain(struct task_struct *task, >> rcu_read_unlock(); >> >> if (!tracerp) >> - return error; >> + goto out; >> >> error = aa_may_ptrace(tracer, tracerp, to_profile, PTRACE_MODE_ATTACH); >> +out: >> put_cred(cred); >> >> return error; > > Ok, the change itself looks fine if we have taken the cred reference we > should return it. > > I just want to confirm that aa_get_task_policy() cannot return a valid > tracerp but a null cred? Else the put_cred() might oops. A quick look > at that seems to say its ok. Assuming that is true: > Well you have already, seen Patch 5 so know that it might oops. The reason for the split is I was trying to treat the as distinct bugs, and keep the fixes distinct and minimal. john
diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c index 128e527..fe89ddc 100644 --- a/ubuntu/apparmor/domain.c +++ b/ubuntu/apparmor/domain.c @@ -65,9 +65,10 @@ static int aa_may_change_ptraced_domain(struct task_struct *task, rcu_read_unlock(); if (!tracerp) - return error; + goto out; error = aa_may_ptrace(tracer, tracerp, to_profile, PTRACE_MODE_ATTACH); +out: put_cred(cred); return error;
BugLink: http://bugs.launchpad.net/bugs/479115 SRU Justification: Failure to put the cred causes a memory leak that is larger than the cred struct, as it leaks everything it references. This happens for every unconfined processes that does an exec, change_hat or change_profile and passes through this function. AppArmor when doing ptrace check for domain changes, fails to drop the ref count on the task creds when it is unconfined. Signed-off-by: John Johansen <john.johansen@canonical.com> --- ubuntu/apparmor/domain.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)