From b79b7ba93df14a1fc0b8d4d6d78a0e097de03bbd Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Wed, 17 Jun 2009 16:27:31 -0700 Subject: ptrace: ptrace_attach: check PF_KTHREAD + exit_state instead of ->mm - Add PF_KTHREAD check to prevent attaching to the kernel thread with a borrowed ->mm. With or without this change we can race with daemonize() which can set PF_KTHREAD or clear ->mm after ptrace_attach() does the check, but this doesn't matter because reparent_to_kthreadd() does ptrace_unlink(). - Kill "!task->mm" check. We don't really care about ->mm != NULL, and the task can call exit_mm() right after we drop task_lock(). What we need is to make sure we can't attach after exit_notify(), check task->exit_state != 0 instead. Also, move the "already traced" check down for cosmetic reasons. Signed-off-by: Oleg Nesterov Cc: Chris Wright Acked-by: Roland McGrath Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- kernel/ptrace.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) (limited to 'kernel/ptrace.c') diff --git a/kernel/ptrace.c b/kernel/ptrace.c index f6d8b8cb5e34..9439bd3331a6 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -172,6 +172,8 @@ int ptrace_attach(struct task_struct *task) audit_ptrace(task); retval = -EPERM; + if (unlikely(task->flags & PF_KTHREAD)) + goto out; if (same_thread_group(task, current)) goto out; @@ -182,8 +184,6 @@ int ptrace_attach(struct task_struct *task) retval = mutex_lock_interruptible(&task->cred_guard_mutex); if (retval < 0) goto out; - - retval = -EPERM; repeat: /* * Nasty, nasty. @@ -203,23 +203,24 @@ repeat: goto repeat; } - if (!task->mm) - goto bad; - /* the same process cannot be attached many times */ - if (task->ptrace & PT_PTRACED) - goto bad; retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH); if (retval) goto bad; - /* Go */ + retval = -EPERM; + if (unlikely(task->exit_state)) + goto bad; + if (task->ptrace & PT_PTRACED) + goto bad; + task->ptrace |= PT_PTRACED; if (capable(CAP_SYS_PTRACE)) task->ptrace |= PT_PTRACE_CAP; __ptrace_link(task, current); - send_sig_info(SIGSTOP, SEND_SIG_FORCED, task); + + retval = 0; bad: write_unlock_irqrestore(&tasklist_lock, flags); task_unlock(task); -- cgit v1.2.2 From f2f0b00ad61d53adfecb8bdf8f3cf8f05f6ed548 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Wed, 17 Jun 2009 16:27:32 -0700 Subject: ptrace: cleanup check/set of PT_PTRACED during attach ptrace_attach() and ptrace_traceme() are the last functions which look as if the untraced task can have task->ptrace != 0, this must not be possible. Change the code to just check ->ptrace != 0 and s/|=/=/ to set PT_PTRACED. Also, a couple of trivial whitespace cleanups in ptrace_attach(). And move ptrace_traceme() up near ptrace_attach() to keep them close to each other. Signed-off-by: Oleg Nesterov Cc: Chris Wright Acked-by: Roland McGrath Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- kernel/ptrace.c | 101 ++++++++++++++++++++++++++++---------------------------- 1 file changed, 51 insertions(+), 50 deletions(-) (limited to 'kernel/ptrace.c') diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 9439bd3331a6..12e21a949db1 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -177,12 +177,13 @@ int ptrace_attach(struct task_struct *task) if (same_thread_group(task, current)) goto out; - /* Protect the target's credential calculations against our + /* + * Protect exec's credential calculations against our interference; * interference; SUID, SGID and LSM creds get determined differently * under ptrace. */ retval = mutex_lock_interruptible(&task->cred_guard_mutex); - if (retval < 0) + if (retval < 0) goto out; repeat: /* @@ -210,10 +211,10 @@ repeat: retval = -EPERM; if (unlikely(task->exit_state)) goto bad; - if (task->ptrace & PT_PTRACED) + if (task->ptrace) goto bad; - task->ptrace |= PT_PTRACED; + task->ptrace = PT_PTRACED; if (capable(CAP_SYS_PTRACE)) task->ptrace |= PT_PTRACE_CAP; @@ -229,6 +230,52 @@ out: return retval; } +/** + * ptrace_traceme -- helper for PTRACE_TRACEME + * + * Performs checks and sets PT_PTRACED. + * Should be used by all ptrace implementations for PTRACE_TRACEME. + */ +int ptrace_traceme(void) +{ + int ret = -EPERM; + + /* + * Are we already being traced? + */ +repeat: + task_lock(current); + if (!current->ptrace) { + /* + * See ptrace_attach() comments about the locking here. + */ + unsigned long flags; + if (!write_trylock_irqsave(&tasklist_lock, flags)) { + task_unlock(current); + do { + cpu_relax(); + } while (!write_can_lock(&tasklist_lock)); + goto repeat; + } + + ret = security_ptrace_traceme(current->parent); + + /* + * Check PF_EXITING to ensure ->real_parent has not passed + * exit_ptrace(). Otherwise we don't report the error but + * pretend ->real_parent untraces us right after return. + */ + if (!ret && !(current->real_parent->flags & PF_EXITING)) { + current->ptrace = PT_PTRACED; + __ptrace_link(current, current->real_parent); + } + + write_unlock_irqrestore(&tasklist_lock, flags); + } + task_unlock(current); + return ret; +} + /* * Called with irqs disabled, returns true if childs should reap themselves. */ @@ -567,52 +614,6 @@ int ptrace_request(struct task_struct *child, long request, return ret; } -/** - * ptrace_traceme -- helper for PTRACE_TRACEME - * - * Performs checks and sets PT_PTRACED. - * Should be used by all ptrace implementations for PTRACE_TRACEME. - */ -int ptrace_traceme(void) -{ - int ret = -EPERM; - - /* - * Are we already being traced? - */ -repeat: - task_lock(current); - if (!(current->ptrace & PT_PTRACED)) { - /* - * See ptrace_attach() comments about the locking here. - */ - unsigned long flags; - if (!write_trylock_irqsave(&tasklist_lock, flags)) { - task_unlock(current); - do { - cpu_relax(); - } while (!write_can_lock(&tasklist_lock)); - goto repeat; - } - - ret = security_ptrace_traceme(current->parent); - - /* - * Check PF_EXITING to ensure ->real_parent has not passed - * exit_ptrace(). Otherwise we don't report the error but - * pretend ->real_parent untraces us right after return. - */ - if (!ret && !(current->real_parent->flags & PF_EXITING)) { - current->ptrace |= PT_PTRACED; - __ptrace_link(current, current->real_parent); - } - - write_unlock_irqrestore(&tasklist_lock, flags); - } - task_unlock(current); - return ret; -} - /** * ptrace_get_task_struct -- grab a task struct reference for ptrace * @pid: process id to grab a task_struct reference of -- cgit v1.2.2 From 4b105cbbaf7c06e01c27391957dc3c446328d087 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Wed, 17 Jun 2009 16:27:33 -0700 Subject: ptrace: do not use task_lock() for attach Remove the "Nasty, nasty" lock dance in ptrace_attach()/ptrace_traceme() - from now task_lock() has nothing to do with ptrace at all. With the recent changes nobody uses task_lock() to serialize with ptrace, but in fact it was never needed and it was never used consistently. However ptrace_attach() calls __ptrace_may_access() and needs task_lock() to pin task->mm for get_dumpable(). But we can call __ptrace_may_access() before we take tasklist_lock, ->cred_exec_mutex protects us against do_execve() path which can change creds and MMF_DUMP* flags. (ugly, but we can't use ptrace_may_access() because it hides the error code, so we have to take task_lock() and use __ptrace_may_access()). NOTE: this change assumes that LSM hooks, security_ptrace_may_access() and security_ptrace_traceme(), can be called without task_lock() held. Signed-off-by: Oleg Nesterov Cc: Chris Wright Acked-by: Roland McGrath Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- kernel/ptrace.c | 59 +++++++++++++-------------------------------------------- 1 file changed, 13 insertions(+), 46 deletions(-) (limited to 'kernel/ptrace.c') diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 12e21a949db1..38fdfea1a15a 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -167,7 +167,6 @@ bool ptrace_may_access(struct task_struct *task, unsigned int mode) int ptrace_attach(struct task_struct *task) { int retval; - unsigned long flags; audit_ptrace(task); @@ -185,34 +184,19 @@ int ptrace_attach(struct task_struct *task) retval = mutex_lock_interruptible(&task->cred_guard_mutex); if (retval < 0) goto out; -repeat: - /* - * Nasty, nasty. - * - * We want to hold both the task-lock and the - * tasklist_lock for writing at the same time. - * But that's against the rules (tasklist_lock - * is taken for reading by interrupts on other - * cpu's that may have task_lock). - */ - task_lock(task); - if (!write_trylock_irqsave(&tasklist_lock, flags)) { - task_unlock(task); - do { - cpu_relax(); - } while (!write_can_lock(&tasklist_lock)); - goto repeat; - } + task_lock(task); retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH); + task_unlock(task); if (retval) - goto bad; + goto unlock_creds; + write_lock_irq(&tasklist_lock); retval = -EPERM; if (unlikely(task->exit_state)) - goto bad; + goto unlock_tasklist; if (task->ptrace) - goto bad; + goto unlock_tasklist; task->ptrace = PT_PTRACED; if (capable(CAP_SYS_PTRACE)) @@ -222,9 +206,9 @@ repeat: send_sig_info(SIGSTOP, SEND_SIG_FORCED, task); retval = 0; -bad: - write_unlock_irqrestore(&tasklist_lock, flags); - task_unlock(task); +unlock_tasklist: + write_unlock_irq(&tasklist_lock); +unlock_creds: mutex_unlock(&task->cred_guard_mutex); out: return retval; @@ -240,26 +224,10 @@ int ptrace_traceme(void) { int ret = -EPERM; - /* - * Are we already being traced? - */ -repeat: - task_lock(current); + write_lock_irq(&tasklist_lock); + /* Are we already being traced? */ if (!current->ptrace) { - /* - * See ptrace_attach() comments about the locking here. - */ - unsigned long flags; - if (!write_trylock_irqsave(&tasklist_lock, flags)) { - task_unlock(current); - do { - cpu_relax(); - } while (!write_can_lock(&tasklist_lock)); - goto repeat; - } - ret = security_ptrace_traceme(current->parent); - /* * Check PF_EXITING to ensure ->real_parent has not passed * exit_ptrace(). Otherwise we don't report the error but @@ -269,10 +237,9 @@ repeat: current->ptrace = PT_PTRACED; __ptrace_link(current, current->real_parent); } - - write_unlock_irqrestore(&tasklist_lock, flags); } - task_unlock(current); + write_unlock_irq(&tasklist_lock); + return ret; } -- cgit v1.2.2 From 8053bdd5ce15dcf043d41a4dd6cac4a5567effdc Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Wed, 17 Jun 2009 16:27:34 -0700 Subject: ptrace_get_task_struct: s/tasklist/rcu/, make it static - Use rcu_read_lock() instead of tasklist_lock to find/get the task in ptrace_get_task_struct(). - Make it static, it has no callers outside of ptrace.c. - The comment doesn't match the reality, this helper does not do any checks. Beacuse it is really trivial and static I removed the whole comment. Signed-off-by: Oleg Nesterov Acked-by: Roland McGrath Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- kernel/ptrace.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) (limited to 'kernel/ptrace.c') diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 38fdfea1a15a..a64fe75a48ba 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -581,26 +581,16 @@ int ptrace_request(struct task_struct *child, long request, return ret; } -/** - * ptrace_get_task_struct -- grab a task struct reference for ptrace - * @pid: process id to grab a task_struct reference of - * - * This function is a helper for ptrace implementations. It checks - * permissions and then grabs a task struct for use of the actual - * ptrace implementation. - * - * Returns the task_struct for @pid or an ERR_PTR() on failure. - */ -struct task_struct *ptrace_get_task_struct(pid_t pid) +static struct task_struct *ptrace_get_task_struct(pid_t pid) { struct task_struct *child; - read_lock(&tasklist_lock); + rcu_read_lock(); child = find_task_by_vpid(pid); if (child) get_task_struct(child); + rcu_read_unlock(); - read_unlock(&tasklist_lock); if (!child) return ERR_PTR(-ESRCH); return child; -- cgit v1.2.2 From e49612544c695117644af48bb4625264a3d2918f Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Wed, 17 Jun 2009 16:27:36 -0700 Subject: ptrace: don't take tasklist to get/set ->last_siginfo Change ptrace_getsiginfo/ptrace_setsiginfo to use lock_task_sighand() without tasklist_lock. Perhaps it makes sense to make a single helper with "bool rw" argument. Signed-off-by: Oleg Nesterov Acked-by: Roland McGrath Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- kernel/ptrace.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) (limited to 'kernel/ptrace.c') diff --git a/kernel/ptrace.c b/kernel/ptrace.c index a64fe75a48ba..61c78b2c07ba 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -424,37 +424,33 @@ static int ptrace_setoptions(struct task_struct *child, long data) static int ptrace_getsiginfo(struct task_struct *child, siginfo_t *info) { + unsigned long flags; int error = -ESRCH; - read_lock(&tasklist_lock); - if (likely(child->sighand != NULL)) { + if (lock_task_sighand(child, &flags)) { error = -EINVAL; - spin_lock_irq(&child->sighand->siglock); if (likely(child->last_siginfo != NULL)) { *info = *child->last_siginfo; error = 0; } - spin_unlock_irq(&child->sighand->siglock); + unlock_task_sighand(child, &flags); } - read_unlock(&tasklist_lock); return error; } static int ptrace_setsiginfo(struct task_struct *child, const siginfo_t *info) { + unsigned long flags; int error = -ESRCH; - read_lock(&tasklist_lock); - if (likely(child->sighand != NULL)) { + if (lock_task_sighand(child, &flags)) { error = -EINVAL; - spin_lock_irq(&child->sighand->siglock); if (likely(child->last_siginfo != NULL)) { *child->last_siginfo = *info; error = 0; } - spin_unlock_irq(&child->sighand->siglock); + unlock_task_sighand(child, &flags); } - read_unlock(&tasklist_lock); return error; } -- cgit v1.2.2 From 793285fcafce4719a05e0c99fa74b188157fe7fe Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Sun, 5 Jul 2009 12:08:26 -0700 Subject: cred_guard_mutex: do not return -EINTR to user-space do_execve() and ptrace_attach() return -EINTR if mutex_lock_interruptible(->cred_guard_mutex) fails. This is not right, change the code to return ERESTARTNOINTR. Perhaps we should also change proc_pid_attr_write(). Signed-off-by: Oleg Nesterov Cc: David Howells Acked-by: Roland McGrath Cc: James Morris Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- kernel/ptrace.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'kernel/ptrace.c') diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 61c78b2c07ba..082c320e4dbf 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -181,8 +181,8 @@ int ptrace_attach(struct task_struct *task) * interference; SUID, SGID and LSM creds get determined differently * under ptrace. */ - retval = mutex_lock_interruptible(&task->cred_guard_mutex); - if (retval < 0) + retval = -ERESTARTNOINTR; + if (mutex_lock_interruptible(&task->cred_guard_mutex)) goto out; task_lock(task); -- cgit v1.2.2