From cfc4f957ff7bffa6aaf7e4029ee9c8689b43c366 Mon Sep 17 00:00:00 2001 From: Roland McGrath Date: Mon, 18 Oct 2004 08:53:35 -0700 Subject: [PATCH] [PATCH] fix PTRACE_ATTACH race with real parent's wait calls There is a race between PTRACE_ATTACH and the real parent calling wait. For a moment, the task is put in PT_PTRACED but with its parent still pointing to its real_parent. In this circumstance, if the real parent calls wait without the WUNTRACED flag, he can see a stopped child status, which wait should never return without WUNTRACED when the caller is not using ptrace. Here it is not the caller that is using ptrace, but some third party. This patch avoids this race condition by adding the PT_ATTACHED flag to distinguish a real parent from a ptrace_attach parent when PT_PTRACED is set, and then having wait use this flag to confirm that things are in order and not consider the child ptraced when its ->ptrace flags are set but its parent links have not yet been switched. (ptrace_check_attach also uses it similarly to rule out a possible race with a bogus ptrace call by the real parent during ptrace_attach.) While looking into this, I noticed that every arch's sys_execve has: current->ptrace &= ~PT_DTRACE; with no locking at all. So, if an exec happens in a race with PTRACE_ATTACH, you could wind up with ->ptrace not having PT_PTRACED set because this store clobbered it. That will cause later BUG hits because the parent links indicate ptracedness but the flag is not set. The patch corrects all the places I found to use task_lock around diddling ->ptrace when it's possible to be racing with ptrace_attach. (The ptrace operation code itself doesn't have this issue because it already excludes anyone else being in ptrace_attach.) Signed-off-by: Roland McGrath Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- arch/i386/kernel/process.c | 2 ++ arch/m32r/kernel/process.c | 5 ++++- arch/parisc/hpux/fs.c | 5 ++++- arch/parisc/kernel/process.c | 5 ++++- arch/parisc/kernel/sys_parisc32.c | 5 ++++- arch/ppc/kernel/process.c | 5 ++++- arch/ppc64/kernel/process.c | 5 ++++- arch/ppc64/kernel/sys_ppc32.c | 5 ++++- arch/s390/kernel/compat_linux.c | 2 ++ arch/s390/kernel/process.c | 2 ++ arch/sh/kernel/process.c | 5 ++++- arch/sh64/kernel/process.c | 5 ++++- arch/sparc/kernel/process.c | 5 ++++- arch/sparc64/kernel/process.c | 2 ++ arch/sparc64/kernel/sys_sparc32.c | 2 ++ arch/um/kernel/exec_kern.c | 2 ++ arch/x86_64/ia32/sys_ia32.c | 5 ++++- arch/x86_64/kernel/process.c | 5 ++++- include/linux/ptrace.h | 1 + kernel/exit.c | 20 ++++++++++++++++++-- kernel/ptrace.c | 5 +++-- 21 files changed, 82 insertions(+), 16 deletions(-) diff --git a/arch/i386/kernel/process.c b/arch/i386/kernel/process.c index 0095fa1dda6f6..45df20c2e77d0 100644 --- a/arch/i386/kernel/process.c +++ b/arch/i386/kernel/process.c @@ -656,7 +656,9 @@ asmlinkage int sys_execve(struct pt_regs regs) (char __user * __user *) regs.edx, ®s); if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); /* Make sure we don't return using sysenter.. */ set_thread_flag(TIF_IRET); } diff --git a/arch/m32r/kernel/process.c b/arch/m32r/kernel/process.c index 9e7de27a8e0d7..c301e0e61173c 100644 --- a/arch/m32r/kernel/process.c +++ b/arch/m32r/kernel/process.c @@ -335,8 +335,11 @@ asmlinkage int sys_execve(char __user *ufilename, char __user * __user *uargv, c goto out; error = do_execve(filename, uargv, uenvp, ®s); - if (error == 0) + if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); + } putname(filename); out: return error; diff --git a/arch/parisc/hpux/fs.c b/arch/parisc/hpux/fs.c index 0800eb3eade6b..d7c80edf44899 100644 --- a/arch/parisc/hpux/fs.c +++ b/arch/parisc/hpux/fs.c @@ -43,8 +43,11 @@ int hpux_execve(struct pt_regs *regs) error = do_execve(filename, (char **) regs->gr[25], (char **)regs->gr[24], regs); - if (error == 0) + if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); + } putname(filename); out: diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c index d7365b958f7eb..320fca55fa1a4 100644 --- a/arch/parisc/kernel/process.c +++ b/arch/parisc/kernel/process.c @@ -363,8 +363,11 @@ asmlinkage int sys_execve(struct pt_regs *regs) goto out; error = do_execve(filename, (char **) regs->gr[25], (char **) regs->gr[24], regs); - if (error == 0) + if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); + } putname(filename); out: diff --git a/arch/parisc/kernel/sys_parisc32.c b/arch/parisc/kernel/sys_parisc32.c index e78332541b180..2b42313c1017a 100644 --- a/arch/parisc/kernel/sys_parisc32.c +++ b/arch/parisc/kernel/sys_parisc32.c @@ -80,8 +80,11 @@ asmlinkage int sys32_execve(struct pt_regs *regs) goto out; error = compat_do_execve(filename, compat_ptr(regs->gr[25]), compat_ptr(regs->gr[24]), regs); - if (error == 0) + if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); + } putname(filename); out: diff --git a/arch/ppc/kernel/process.c b/arch/ppc/kernel/process.c index d9ab6a7de95c8..94298ebecabe1 100644 --- a/arch/ppc/kernel/process.c +++ b/arch/ppc/kernel/process.c @@ -598,8 +598,11 @@ int sys_execve(unsigned long a0, unsigned long a1, unsigned long a2, preempt_enable(); error = do_execve(filename, (char __user *__user *) a1, (char __user *__user *) a2, regs); - if (error == 0) + if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); + } putname(filename); out: return error; diff --git a/arch/ppc64/kernel/process.c b/arch/ppc64/kernel/process.c index 8211337074cb5..1529ba6db632b 100644 --- a/arch/ppc64/kernel/process.c +++ b/arch/ppc64/kernel/process.c @@ -512,8 +512,11 @@ int sys_execve(unsigned long a0, unsigned long a1, unsigned long a2, error = do_execve(filename, (char __user * __user *) a1, (char __user * __user *) a2, regs); - if (error == 0) + if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); + } putname(filename); out: diff --git a/arch/ppc64/kernel/sys_ppc32.c b/arch/ppc64/kernel/sys_ppc32.c index 710b7cd9ec470..6a514b3d977c5 100644 --- a/arch/ppc64/kernel/sys_ppc32.c +++ b/arch/ppc64/kernel/sys_ppc32.c @@ -621,8 +621,11 @@ long sys32_execve(unsigned long a0, unsigned long a1, unsigned long a2, error = compat_do_execve(filename, compat_ptr(a1), compat_ptr(a2), regs); - if (error == 0) + if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); + } putname(filename); out: diff --git a/arch/s390/kernel/compat_linux.c b/arch/s390/kernel/compat_linux.c index 5c0a63aff9393..01f57183d9ba2 100644 --- a/arch/s390/kernel/compat_linux.c +++ b/arch/s390/kernel/compat_linux.c @@ -751,7 +751,9 @@ sys32_execve(struct pt_regs regs) compat_ptr(regs.gprs[4]), ®s); if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); current->thread.fp_regs.fpc=0; __asm__ __volatile__ ("sr 0,0\n\t" diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c index 5d56e77c74eee..566b34f4c3947 100644 --- a/arch/s390/kernel/process.c +++ b/arch/s390/kernel/process.c @@ -340,7 +340,9 @@ asmlinkage long sys_execve(struct pt_regs regs) error = do_execve(filename, (char __user * __user *) regs.gprs[3], (char __user * __user *) regs.gprs[4], ®s); if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); current->thread.fp_regs.fpc = 0; if (MACHINE_HAS_IEEE) asm volatile("sfpc %0,%0" : : "d" (0)); diff --git a/arch/sh/kernel/process.c b/arch/sh/kernel/process.c index c9a43c8df32fb..11019fa5ab86d 100644 --- a/arch/sh/kernel/process.c +++ b/arch/sh/kernel/process.c @@ -481,8 +481,11 @@ asmlinkage int sys_execve(char *ufilename, char **uargv, (char __user * __user *)uargv, (char __user * __user *)uenvp, ®s); - if (error == 0) + if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); + } putname(filename); out: return error; diff --git a/arch/sh64/kernel/process.c b/arch/sh64/kernel/process.c index 13cec35796ab9..68f764389781d 100644 --- a/arch/sh64/kernel/process.c +++ b/arch/sh64/kernel/process.c @@ -862,8 +862,11 @@ asmlinkage int sys_execve(char *ufilename, char **uargv, (char __user * __user *)uargv, (char __user * __user *)uenvp, pregs); - if (error == 0) + if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); + } putname(filename); out: unlock_kernel(); diff --git a/arch/sparc/kernel/process.c b/arch/sparc/kernel/process.c index 1dc918135eb33..f32eaf5b6256e 100644 --- a/arch/sparc/kernel/process.c +++ b/arch/sparc/kernel/process.c @@ -670,8 +670,11 @@ asmlinkage int sparc_execve(struct pt_regs *regs) (char __user * __user *)regs->u_regs[base + UREG_I2], regs); putname(filename); - if (error == 0) + if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); + } out: return error; } diff --git a/arch/sparc64/kernel/process.c b/arch/sparc64/kernel/process.c index f3e3c657e9cb8..56fecbdae8bc1 100644 --- a/arch/sparc64/kernel/process.c +++ b/arch/sparc64/kernel/process.c @@ -829,7 +829,9 @@ asmlinkage int sparc_execve(struct pt_regs *regs) current_thread_info()->xfsr[0] = 0; current_thread_info()->fpsaved[0] = 0; regs->tstate &= ~TSTATE_PEF; + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); } out: return error; diff --git a/arch/sparc64/kernel/sys_sparc32.c b/arch/sparc64/kernel/sys_sparc32.c index b81f15521e86b..28014dc741945 100644 --- a/arch/sparc64/kernel/sys_sparc32.c +++ b/arch/sparc64/kernel/sys_sparc32.c @@ -1274,7 +1274,9 @@ asmlinkage long sparc32_execve(struct pt_regs *regs) current_thread_info()->xfsr[0] = 0; current_thread_info()->fpsaved[0] = 0; regs->tstate &= ~TSTATE_PEF; + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); } out: return error; diff --git a/arch/um/kernel/exec_kern.c b/arch/um/kernel/exec_kern.c index a5f21283d2cd8..f6c84475bb836 100644 --- a/arch/um/kernel/exec_kern.c +++ b/arch/um/kernel/exec_kern.c @@ -43,7 +43,9 @@ static int execve1(char *file, char **argv, char **env) #endif error = do_execve(file, argv, env, ¤t->thread.regs); if (error == 0){ + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); set_cmdline(current_cmd()); } return(error); diff --git a/arch/x86_64/ia32/sys_ia32.c b/arch/x86_64/ia32/sys_ia32.c index 50465345140ad..a854fb32963ab 100644 --- a/arch/x86_64/ia32/sys_ia32.c +++ b/arch/x86_64/ia32/sys_ia32.c @@ -1135,8 +1135,11 @@ asmlinkage long sys32_execve(char __user *name, compat_uptr_t __user *argv, if (IS_ERR(filename)) return error; error = compat_do_execve(filename, argv, envp, regs); - if (error == 0) + if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); + } putname(filename); return error; } diff --git a/arch/x86_64/kernel/process.c b/arch/x86_64/kernel/process.c index 6e835be5f26a6..73dfb27a53520 100644 --- a/arch/x86_64/kernel/process.c +++ b/arch/x86_64/kernel/process.c @@ -542,8 +542,11 @@ long sys_execve(char __user *name, char __user * __user *argv, if (IS_ERR(filename)) return error; error = do_execve(filename, argv, envp, ®s); - if (error == 0) + if (error == 0) { + task_lock(current); current->ptrace &= ~PT_DTRACE; + task_unlock(current); + } putname(filename); return error; } diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h index 53132cd80429e..f182471250912 100644 --- a/include/linux/ptrace.h +++ b/include/linux/ptrace.h @@ -63,6 +63,7 @@ #define PT_TRACE_EXEC 0x00000080 #define PT_TRACE_VFORK_DONE 0x00000100 #define PT_TRACE_EXIT 0x00000200 +#define PT_ATTACHED 0x00000400 /* parent != real_parent */ #define PT_TRACE_MASK 0x000003f4 diff --git a/kernel/exit.c b/kernel/exit.c index 426d3ae722ba1..ca9a9e21c4444 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -1280,6 +1280,22 @@ static int wait_task_continued(task_t *p, int noreap, } +static inline int my_ptrace_child(struct task_struct *p) +{ + if (!(p->ptrace & PT_PTRACED)) + return 0; + if (!(p->ptrace & PT_ATTACHED)) + return 1; + /* + * This child was PTRACE_ATTACH'd. We should be seeing it only if + * we are the attacher. If we are the real parent, this is a race + * inside ptrace_attach. It is waiting for the tasklist_lock, + * which we have to switch the parent links, but has already set + * the flags in p->ptrace. + */ + return (p->parent != p->real_parent); +} + static long do_wait(pid_t pid, int options, struct siginfo __user *infop, int __user *stat_addr, struct rusage __user *ru) { @@ -1308,12 +1324,12 @@ repeat: switch (p->state) { case TASK_TRACED: - if (!(p->ptrace & PT_PTRACED)) + if (!my_ptrace_child(p)) continue; /*FALLTHROUGH*/ case TASK_STOPPED: if (!(options & WUNTRACED) && - !(p->ptrace & PT_PTRACED)) + !my_ptrace_child(p)) continue; retval = wait_task_stopped(p, ret == 2, (options & WNOWAIT), diff --git a/kernel/ptrace.c b/kernel/ptrace.c index b14b4a4677296..09ba057222c33 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -82,7 +82,8 @@ int ptrace_check_attach(struct task_struct *child, int kill) */ read_lock(&tasklist_lock); if ((child->ptrace & PT_PTRACED) && child->parent == current && - child->signal != NULL) { + (!(child->ptrace & PT_ATTACHED) || child->real_parent != current) + && child->signal != NULL) { ret = 0; spin_lock_irq(&child->sighand->siglock); if (child->state == TASK_STOPPED) { @@ -131,7 +132,7 @@ int ptrace_attach(struct task_struct *task) goto bad; /* Go */ - task->ptrace |= PT_PTRACED; + task->ptrace |= PT_PTRACED | PT_ATTACHED; if (capable(CAP_SYS_PTRACE)) task->ptrace |= PT_PTRACE_CAP; task_unlock(task); -- 2.53.0