lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 26 Nov 2008 15:00:27 +0100 From: Ingo Molnar <mingo@...e.hu> To: eranian@...glemail.com Cc: linux-kernel@...r.kernel.org, akpm@...ux-foundation.org, x86@...nel.org, andi@...stfloor.org, eranian@...il.com, sfr@...b.auug.org.au, Roland McGrath <roland@...hat.com>, Oleg Nesterov <oleg@...hat.com> Subject: Re: [patch 20/24] perfmon: system calls interface * eranian@...glemail.com <eranian@...glemail.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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists