Messages in this thread | | | Date | Wed, 26 Nov 2008 15:00:27 +0100 | From | Ingo Molnar <> | Subject | Re: [patch 20/24] perfmon: system calls interface |
| |
* eranian@googlemail.com <eranian@googlemail.com> wrote:
> +static int pfm_task_incompatible(struct pfm_context *ctx, > + struct task_struct *task) > +{ > + /* > + * cannot attach to a kernel thread > + */ > + if (!task->mm) { > + PFM_DBG("cannot attach to kernel thread [%d]", task->pid); > + return -EPERM; > + } > + > + /* > + * cannot attach to a zombie task > + */ > + if (task->exit_state == EXIT_ZOMBIE || task->exit_state == EXIT_DEAD) { > + PFM_DBG("cannot attach to zombie/dead task [%d]", task->pid); > + return -EBUSY; > + } > + return 0; > +}
The ptrace coupling code seems broken.
It is used in the following context:
+ /* + * returns 0 if cannot attach + */ + ret1 = ptrace_may_access(p, PTRACE_MODE_ATTACH); + if (ret1) + ret = ptrace_check_attach(p, 0); + + PFM_DBG("may_attach=%d check_attach=%d", ret1, ret); + + if (ret || !ret1) + goto error; + + ret = pfm_task_incompatible(ctx, p); + if (ret) + goto error;
firstly, this code is critical to security, but the variable naming and the control flow is shaped in a dangerous and error-prone way: two opaque 'ret' and 'ret1' names, which have _inverted_ logical meaning: for 'ret' a nonzero value means an error, for 'ret1' a zero value means error.
This code _must_ be rewritten cleanly via a single 'err' variable. There's absolutely no need to nest ret and ret1 here. If we may not access via ptrace then we should error out pronto and not complicate the flow.
Secondly, get rid of those PFM_DBG() calls there, they are not needed in a production kernel and just obscure review.
Thirdly, the check for ->exit_state in pfm_task_incompatible() is not needed: we've just passed ptrace_check_attach() so we know we just transitioned the task to task->state == TASK_TRACED.
If you _ever_ see a task exit TASK_TRACED and go zombie or dead from there without this code allowing it that means the whole state machine with ptrace is borked up by perfmon. For example i dont see where the perfmon-control task parents itself as the exclusive debugger (parent) of the debuggee-task.
Without that being implemented properly, a parallel ptrace / perfmon scenario (triggerable by unprivileged userspace) can go amok, crash the kernel and likely open up various rootholes as well. The kludge in pfm_task_incompatible() shows that this was probably seen in the field and hacked around.
Ingo
| |