| Message ID | 1257877753-9448-6-git-send-email-john.johansen@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
As tracer is NULL in the exist case, ca_get_task_policy() has not been called, so a direct exit looks ok. John Johansen wrote: > BugLink: http://bugs.launchpad.net/bugs/480112 > > SRU Justification: This bug can cause confined process to oops at address 0. > This can occur when executing a process if the LSM_UNSAFE_PTRACE | > LSM_UNSAFE_PTRACE_CAP flags are set. The likely hood of if/how often this > will occur depends on if ptrace is being used. > > As reported by Tetsuo Handa on kernel-team mailing list: > > In aa_may_change_ptraced_domain, if (!tracer) cred == NULL, and > put_cred(cred) will oops. This will only happen on exec if the task > is marked as LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP, so should > only happen to ptraced tasks that are confined. > > Fix this by returning directly from aa_may_change_ptrace_domain if > there is now tracer. > > Signed-off-by: John Johansen <john.johansen@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > ubuntu/apparmor/domain.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c > index fe89ddc..12e45c6 100644 > --- a/ubuntu/apparmor/domain.c > +++ b/ubuntu/apparmor/domain.c > @@ -64,6 +64,10 @@ static int aa_may_change_ptraced_domain(struct task_struct *task, > cred = aa_get_task_policy(tracer, &tracerp); > rcu_read_unlock(); > > + /* not ptraced */ > + if (!tracer) > + return 0; > + > if (!tracerp) > goto out; >
On Tue, Nov 10, 2009 at 10:29:13AM -0800, John Johansen wrote: > BugLink: http://bugs.launchpad.net/bugs/480112 > > SRU Justification: This bug can cause confined process to oops at address 0. > This can occur when executing a process if the LSM_UNSAFE_PTRACE | > LSM_UNSAFE_PTRACE_CAP flags are set. The likely hood of if/how often this > will occur depends on if ptrace is being used. > > As reported by Tetsuo Handa on kernel-team mailing list: > > In aa_may_change_ptraced_domain, if (!tracer) cred == NULL, and > put_cred(cred) will oops. This will only happen on exec if the task > is marked as LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP, so should > only happen to ptraced tasks that are confined. > > Fix this by returning directly from aa_may_change_ptrace_domain if > there is now tracer. > > Signed-off-by: John Johansen <john.johansen@canonical.com> > --- > ubuntu/apparmor/domain.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c > index fe89ddc..12e45c6 100644 > --- a/ubuntu/apparmor/domain.c > +++ b/ubuntu/apparmor/domain.c > @@ -64,6 +64,10 @@ static int aa_may_change_ptraced_domain(struct task_struct *task, > cred = aa_get_task_policy(tracer, &tracerp); > rcu_read_unlock(); > > + /* not ptraced */ > + if (!tracer) > + return 0; > + > if (!tracerp) > goto out; > > -- > 1.6.3.3 Hrm, now perhaps this is fixing the concern I raised in the previous patch. Perhaps it would be safer to simply make the put_cred() incantation instead? if (cred) put_cred(cred); -apw
On Thu, Nov 12, 2009 at 11:28:10AM +0000, Andy Whitcroft wrote: > Hrm, now perhaps this is fixing the concern I raised in the previous > patch. Perhaps it would be safer to simply make the put_cred() > incantation instead? > > if (cred) > put_cred(cred); I guess I _am_ happy that the current form would address the concern and so in that sense: Acked-by: Andy Whitcroft <apw@canonical.com> I would suggest considering a more bullet proof approach as above for your mainline efforts as its clearer that cred can only be released if non-null, and its safer against further change in the function. -apw
Hello. John Johansen wrote: > As reported by Tetsuo Handa on kernel-team mailing list: Oops. I used wrong sender address and therefore above report didn't reach to kernel-team mailing list. Just a comment for AppArmor for Karmic and earlier. --- security/apparmor/path.c --- > char *sysctl_pathname(struct ctl_table *table, char *buffer, int buflen) > { > if (buflen < 1) > return NULL; > buffer += --buflen; > *buffer = '\0'; > > while (table) { > int namelen = strlen(table->procname); Eric W. Biederman is going to remove table->ctl_name field. Thus, future versions will be safe to use table->procname without checking for NULL. But, for past versions, some out-of-tree kernel module might create a table with table->procname == NULL. Maybe AppArmor for Karmic and earlier should prepare for NULL because parse_table()'s loop condition allows NULL procname. > > if (buflen < namelen + 1) > return NULL; > buflen -= namelen + 1; > buffer -= namelen; > memcpy(buffer, table->procname, namelen); > *--buffer = '/'; > table = table->parent; > } > if (buflen < 4) > return NULL; > buffer -= 4; > memcpy(buffer, "/sys", 4); > > return buffer; > }
Applied
Andy Whitcroft wrote: > On Thu, Nov 12, 2009 at 11:28:10AM +0000, Andy Whitcroft wrote: > >> Hrm, now perhaps this is fixing the concern I raised in the previous >> patch. Perhaps it would be safer to simply make the put_cred() >> incantation instead? >> >> if (cred) >> put_cred(cred); > > I guess I _am_ happy that the current form would address the concern and > so in that sense: > > Acked-by: Andy Whitcroft <apw@canonical.com> > > I would suggest considering a more bullet proof approach as above for > your mainline efforts as its clearer that cred can only be released if > non-null, and its safer against further change in the function. > Yep, what you suggest would be better.
Tetsuo Handa wrote: > Hello. > > John Johansen wrote: >> As reported by Tetsuo Handa on kernel-team mailing list: > Oops. I used wrong sender address and therefore above report didn't reach to > kernel-team mailing list. > > > > Just a comment for AppArmor for Karmic and earlier. > > --- security/apparmor/path.c --- >> char *sysctl_pathname(struct ctl_table *table, char *buffer, int buflen) >> { >> if (buflen < 1) >> return NULL; >> buffer += --buflen; >> *buffer = '\0'; >> >> while (table) { >> int namelen = strlen(table->procname); > > Eric W. Biederman is going to remove table->ctl_name field. Thus, future > versions will be safe to use table->procname without checking for NULL. > > But, for past versions, some out-of-tree kernel module might create a table > with table->procname == NULL. Maybe AppArmor for Karmic and earlier should > prepare for NULL because parse_table()'s loop condition allows NULL procname. > Thanks for pointing this out Tetsuo
diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c index fe89ddc..12e45c6 100644 --- a/ubuntu/apparmor/domain.c +++ b/ubuntu/apparmor/domain.c @@ -64,6 +64,10 @@ static int aa_may_change_ptraced_domain(struct task_struct *task, cred = aa_get_task_policy(tracer, &tracerp); rcu_read_unlock(); + /* not ptraced */ + if (!tracer) + return 0; + if (!tracerp) goto out;
BugLink: http://bugs.launchpad.net/bugs/480112 SRU Justification: This bug can cause confined process to oops at address 0. This can occur when executing a process if the LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP flags are set. The likely hood of if/how often this will occur depends on if ptrace is being used. As reported by Tetsuo Handa on kernel-team mailing list: In aa_may_change_ptraced_domain, if (!tracer) cred == NULL, and put_cred(cred) will oops. This will only happen on exec if the task is marked as LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP, so should only happen to ptraced tasks that are confined. Fix this by returning directly from aa_may_change_ptrace_domain if there is now tracer. Signed-off-by: John Johansen <john.johansen@canonical.com> --- ubuntu/apparmor/domain.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)