| Message ID | AM8PR10MB4708AFBD838138A84CE89EF8E4359@AM8PR10MB4708.EURPRD10.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv8] exec: Fix dead-lock in de_thread with ptrace_attach | expand |
On Thu, 10 Jun 2021 09:31:42 +0200 Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > This introduces signal->unsafe_execve_in_progress, > which is used to fix the case when at least one of the > sibling threads is traced, and therefore the trace > process may dead-lock in ptrace_attach, but de_thread > will need to wait for the tracer to continue execution. Deadlocks are serious. Is this exploitable by unprivileged userspace? > Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de> Was a -stable backport considered?
On 6/10/21 11:36 PM, Andrew Morton wrote: > On Thu, 10 Jun 2021 09:31:42 +0200 Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > >> This introduces signal->unsafe_execve_in_progress, >> which is used to fix the case when at least one of the >> sibling threads is traced, and therefore the trace >> process may dead-lock in ptrace_attach, but de_thread >> will need to wait for the tracer to continue execution. > > Deadlocks are serious. Is this exploitable by unprivileged userspace? > Yes, in theory: You need a program doing things like the second test case in vmaccess.c and if the parent (also unprivileged) does simply not call PTRACE_ATTACH and not waitpid. This is an unprivileged process. Now if A kernel process would try the PTRACE_ATTACH from the test case it would freeze until the parent process calls waitpid, which it would not do in this scenario. >> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de> > > Was a -stable backport considered? > > Yes, maybe after some time. Thanks Bernd.
On 6/10/21 9:31 AM, Bernd Edlinger wrote: > This introduces signal->unsafe_execve_in_progress, > which is used to fix the case when at least one of the > sibling threads is traced, and therefore the trace > process may dead-lock in ptrace_attach, but de_thread > will need to wait for the tracer to continue execution. > > The solution is to detect this situation and allow > ptrace_attach to continue, while de_thread() is still > waiting for traced zombies to be eventually released. > When the current thread changed the ptrace status from > non-traced to traced, we can simply abort the whole > execve and restart it by returning -ERESTARTSYS. > This needs to be done before changing the thread leader, > because the PTRACE_EVENT_EXEC needs to know the old > thread pid. > > Although it is technically after the point of no return, > we just have to reset bprm->point_of_no_return here, > since at this time only the other threads have received > a fatal signal, not the current thread. > > From the user's point of view the whole execve was > simply delayed until after the ptrace_attach. > > Other threads die quickly since the cred_guard_mutex > is released, but a deadly signal is already pending. > In case the mutex_lock_killable misses the signal, > ->unsafe_execve_in_progress makes sure they release > the mutex immediately and return with -ERESTARTNOINTR. > > This means there is no API change, unlike the previous > version of this patch which was discussed here: > > https://lore.kernel.org/lkml/b6537ae6-31b1-5c50-f32b-8b8332ace882@hotmail.de/ > > See tools/testing/selftests/ptrace/vmaccess.c > for a test case that gets fixed by this change. > > Note that since the test case was originally designed to > test the ptrace_attach returning an error in this situation, > the test expectation needed to be adjusted, to allow the > API to succeed at the first attempt. > > Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de> > --- > fs/exec.c | 45 ++++++++++++++++++++++++++----- > fs/proc/base.c | 6 +++++ > include/linux/sched/signal.h | 13 +++++++++ > kernel/ptrace.c | 9 +++++++ > kernel/seccomp.c | 12 ++++++--- > tools/testing/selftests/ptrace/vmaccess.c | 25 +++++++++++------ > 6 files changed, 92 insertions(+), 18 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 8344fba..ac3fec1 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1040,6 +1040,8 @@ static int de_thread(struct task_struct *tsk) > struct signal_struct *sig = tsk->signal; > struct sighand_struct *oldsighand = tsk->sighand; > spinlock_t *lock = &oldsighand->siglock; > + unsigned int prev_ptrace = tsk->ptrace; > + struct task_struct *t = tsk; > > if (thread_group_empty(tsk)) > goto no_thread_group; > @@ -1057,20 +1059,40 @@ static int de_thread(struct task_struct *tsk) > return -EAGAIN; > } > > + while_each_thread(tsk, t) { > + if (unlikely(t->ptrace) && t != tsk->group_leader) > + sig->unsafe_execve_in_progress = true; > + } > + > sig->group_exit_task = tsk; > sig->notify_count = zap_other_threads(tsk); > if (!thread_group_leader(tsk)) > sig->notify_count--; > + spin_unlock_irq(lock); > > - while (sig->notify_count) { > - __set_current_state(TASK_KILLABLE); > - spin_unlock_irq(lock); > + if (unlikely(sig->unsafe_execve_in_progress)) > + mutex_unlock(&sig->cred_guard_mutex); > + > + for (;;) { > + set_current_state(TASK_KILLABLE); > + if (!sig->notify_count) > + break; > schedule(); > if (__fatal_signal_pending(tsk)) > goto killed; > - spin_lock_irq(lock); > } > - spin_unlock_irq(lock); > + __set_current_state(TASK_RUNNING); Oh, sorry, I think I'll need to keep this spin_lock here, because otherwise the assignment sig->group_exit_task = NULL below will race with kernel/exit.c (__exit_signal): if (sig->notify_count > 0 && !--sig->notify_count) wake_up_process(sig->group_exit_task); which runs under spin_lock(&sighand->siglock) and tasklist_lock write-locked. Will send an updaten the patch later today. Bernd. > + > + if (unlikely(sig->unsafe_execve_in_progress)) { > + if (mutex_lock_killable(&sig->cred_guard_mutex)) > + goto killed; > + sig->unsafe_execve_in_progress = false; > + if (!prev_ptrace && tsk->ptrace) { > + sig->group_exit_task = NULL; > + sig->notify_count = 0; > + return -ERESTARTSYS; > + } > + } > > /* > * At this point all other threads have exited, all we have to > @@ -1255,8 +1277,11 @@ int begin_new_exec(struct linux_binprm * bprm) > * Make this the only thread in the thread group. > */ > retval = de_thread(me); > - if (retval) > + if (retval) { > + if (retval == -ERESTARTSYS) > + bprm->point_of_no_return = false; > goto out; > + } > > /* > * Cancel any io_uring activity across execve > @@ -1466,6 +1491,11 @@ static int prepare_bprm_creds(struct linux_binprm *bprm) > if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex)) > return -ERESTARTNOINTR; > > + if (unlikely(current->signal->unsafe_execve_in_progress)) { > + mutex_unlock(¤t->signal->cred_guard_mutex); > + return -ERESTARTNOINTR; > + } > + > bprm->cred = prepare_exec_creds(); > if (likely(bprm->cred)) > return 0; > @@ -1482,7 +1512,8 @@ static void free_bprm(struct linux_binprm *bprm) > } > free_arg_pages(bprm); > if (bprm->cred) { > - mutex_unlock(¤t->signal->cred_guard_mutex); > + if (!current->signal->unsafe_execve_in_progress) > + mutex_unlock(¤t->signal->cred_guard_mutex); > abort_creds(bprm->cred); > } > if (bprm->file) { > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 3851bfc..3b2a55c 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2739,6 +2739,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, > if (rv < 0) > goto out_free; > > + if (unlikely(current->signal->unsafe_execve_in_progress)) { > + mutex_unlock(¤t->signal->cred_guard_mutex); > + rv = -ERESTARTNOINTR; > + goto out_free; > + } > + > rv = security_setprocattr(PROC_I(inode)->op.lsm, > file->f_path.dentry->d_name.name, page, > count); > diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h > index 3f6a0fc..220a083 100644 > --- a/include/linux/sched/signal.h > +++ b/include/linux/sched/signal.h > @@ -214,6 +214,17 @@ struct signal_struct { > #endif > > /* > + * Set while execve is executing but is *not* holding > + * cred_guard_mutex to avoid possible dead-locks. > + * The cred_guard_mutex is released *after* de_thread() has > + * called zap_other_threads(), therefore a fatal signal is > + * guaranteed to be already pending in the unlikely event, that > + * current->signal->unsafe_execve_in_progress happens to be > + * true after the cred_guard_mutex was acquired. > + */ > + bool unsafe_execve_in_progress; > + > + /* > * Thread is the potential origin of an oom condition; kill first on > * oom > */ > @@ -227,6 +238,8 @@ struct signal_struct { > struct mutex cred_guard_mutex; /* guard against foreign influences on > * credential calculations > * (notably. ptrace) > + * Held while execve runs, except when > + * a sibling thread is being traced. > * Deprecated do not use in new code. > * Use exec_update_lock instead. > */ > diff --git a/kernel/ptrace.c b/kernel/ptrace.c > index 61db50f..0cbc1eb 100644 > --- a/kernel/ptrace.c > +++ b/kernel/ptrace.c > @@ -468,6 +468,14 @@ static int ptrace_traceme(void) > { > int ret = -EPERM; > > + if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex)) > + return -ERESTARTNOINTR; > + > + if (unlikely(current->signal->unsafe_execve_in_progress)) { > + mutex_unlock(¤t->signal->cred_guard_mutex); > + return -ERESTARTNOINTR; > + } > + > write_lock_irq(&tasklist_lock); > /* Are we already being traced? */ > if (!current->ptrace) { > @@ -483,6 +491,7 @@ static int ptrace_traceme(void) > } > } > write_unlock_irq(&tasklist_lock); > + mutex_unlock(¤t->signal->cred_guard_mutex); > > return ret; > } > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index 1d60fc2..b1389ee 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -1824,9 +1824,15 @@ static long seccomp_set_mode_filter(unsigned int flags, > * Make sure we cannot change seccomp or nnp state via TSYNC > * while another thread is in the middle of calling exec. > */ > - if (flags & SECCOMP_FILTER_FLAG_TSYNC && > - mutex_lock_killable(¤t->signal->cred_guard_mutex)) > - goto out_put_fd; > + if (flags & SECCOMP_FILTER_FLAG_TSYNC) { > + if (mutex_lock_killable(¤t->signal->cred_guard_mutex)) > + goto out_put_fd; > + > + if (unlikely(current->signal->unsafe_execve_in_progress)) { > + mutex_unlock(¤t->signal->cred_guard_mutex); > + goto out_put_fd; > + } > + } > > spin_lock_irq(¤t->sighand->siglock); > > diff --git a/tools/testing/selftests/ptrace/vmaccess.c b/tools/testing/selftests/ptrace/vmaccess.c > index 4db327b..c7c2242 100644 > --- a/tools/testing/selftests/ptrace/vmaccess.c > +++ b/tools/testing/selftests/ptrace/vmaccess.c > @@ -39,8 +39,15 @@ static void *thread(void *arg) > f = open(mm, O_RDONLY); > ASSERT_GE(f, 0); > close(f); > - f = kill(pid, SIGCONT); > - ASSERT_EQ(f, 0); > + f = waitpid(-1, NULL, 0); > + ASSERT_NE(f, -1); > + ASSERT_NE(f, 0); > + ASSERT_NE(f, pid); > + f = waitpid(-1, NULL, 0); > + ASSERT_EQ(f, pid); > + f = waitpid(-1, NULL, 0); > + ASSERT_EQ(f, -1); > + ASSERT_EQ(errno, ECHILD); > } > > TEST(attach) > @@ -57,22 +64,24 @@ static void *thread(void *arg) > > sleep(1); > k = ptrace(PTRACE_ATTACH, pid, 0L, 0L); > - ASSERT_EQ(errno, EAGAIN); > - ASSERT_EQ(k, -1); > + ASSERT_EQ(k, 0); > k = waitpid(-1, &s, WNOHANG); > ASSERT_NE(k, -1); > ASSERT_NE(k, 0); > ASSERT_NE(k, pid); > ASSERT_EQ(WIFEXITED(s), 1); > ASSERT_EQ(WEXITSTATUS(s), 0); > - sleep(1); > - k = ptrace(PTRACE_ATTACH, pid, 0L, 0L); > - ASSERT_EQ(k, 0); > k = waitpid(-1, &s, 0); > ASSERT_EQ(k, pid); > ASSERT_EQ(WIFSTOPPED(s), 1); > ASSERT_EQ(WSTOPSIG(s), SIGSTOP); > - k = ptrace(PTRACE_DETACH, pid, 0L, 0L); > + k = ptrace(PTRACE_CONT, pid, 0L, 0L); > + ASSERT_EQ(k, 0); > + k = waitpid(-1, &s, 0); > + ASSERT_EQ(k, pid); > + ASSERT_EQ(WIFSTOPPED(s), 1); > + ASSERT_EQ(WSTOPSIG(s), SIGTRAP); > + k = ptrace(PTRACE_CONT, pid, 0L, 0L); > ASSERT_EQ(k, 0); > k = waitpid(-1, &s, 0); > ASSERT_EQ(k, pid); >
On Thu, 10 Jun 2021 09:31:42 +0200 Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > This introduces signal->unsafe_execve_in_progress, > which is used to fix the case when at least one of the > sibling threads is traced, and therefore the trace > process may dead-lock in ptrace_attach, but de_thread > will need to wait for the tracer to continue execution. > > The solution is to detect this situation and allow > ptrace_attach to continue, while de_thread() is still > waiting for traced zombies to be eventually released. > When the current thread changed the ptrace status from > non-traced to traced, we can simply abort the whole > execve and restart it by returning -ERESTARTSYS. > This needs to be done before changing the thread leader, > because the PTRACE_EVENT_EXEC needs to know the old > thread pid. > > Although it is technically after the point of no return, > we just have to reset bprm->point_of_no_return here, > since at this time only the other threads have received > a fatal signal, not the current thread. > > >From the user's point of view the whole execve was > simply delayed until after the ptrace_attach. > > Other threads die quickly since the cred_guard_mutex > is released, but a deadly signal is already pending. > In case the mutex_lock_killable misses the signal, > ->unsafe_execve_in_progress makes sure they release > the mutex immediately and return with -ERESTARTNOINTR. > > This means there is no API change, unlike the previous > version of this patch which was discussed here: > > https://lore.kernel.org/lkml/b6537ae6-31b1-5c50-f32b-8b8332ace882@hotmail.de/ > > See tools/testing/selftests/ptrace/vmaccess.c > for a test case that gets fixed by this change. > > Note that since the test case was originally designed to > test the ptrace_attach returning an error in this situation, > the test expectation needed to be adjusted, to allow the > API to succeed at the first attempt. > Here's the diff from v8. It's conventional to tell reviewers what changed when sending out a new version. What changed in this version? --- a/fs/exec.c~exec-fix-dead-lock-in-de_thread-with-ptrace_attach-v9 +++ a/fs/exec.c @@ -1056,29 +1056,31 @@ static int de_thread(struct task_struct return -EAGAIN; } - while_each_thread(tsk, t) { - if (unlikely(t->ptrace) && t != tsk->group_leader) - sig->unsafe_execve_in_progress = true; - } - sig->group_exit_task = tsk; sig->notify_count = zap_other_threads(tsk); if (!thread_group_leader(tsk)) sig->notify_count--; - spin_unlock_irq(lock); - if (unlikely(sig->unsafe_execve_in_progress)) + while_each_thread(tsk, t) { + if (unlikely(t->ptrace) && t != tsk->group_leader) + sig->unsafe_execve_in_progress = true; + } + + if (unlikely(sig->unsafe_execve_in_progress)) { + spin_unlock_irq(lock); mutex_unlock(&sig->cred_guard_mutex); + spin_lock_irq(lock); + } - for (;;) { - set_current_state(TASK_KILLABLE); - if (!sig->notify_count) - break; + while (sig->notify_count) { + __set_current_state(TASK_KILLABLE); + spin_unlock_irq(lock); schedule(); if (__fatal_signal_pending(tsk)) goto killed; + spin_lock_irq(lock); } - __set_current_state(TASK_RUNNING); + spin_unlock_irq(lock); if (unlikely(sig->unsafe_execve_in_progress)) { if (mutex_lock_killable(&sig->cred_guard_mutex))
Andrew Morton <akpm@linux-foundation.org> writes: > On Thu, 10 Jun 2021 09:31:42 +0200 Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > >> This introduces signal->unsafe_execve_in_progress, >> which is used to fix the case when at least one of the >> sibling threads is traced, and therefore the trace >> process may dead-lock in ptrace_attach, but de_thread >> will need to wait for the tracer to continue execution. > > Deadlocks are serious. Is this exploitable by unprivileged userspace? The processes are killable so I don't think this is the serious in the way you mean. In fact Linus has already said that it is not a deadlock. Eric
diff --git a/fs/exec.c b/fs/exec.c index 8344fba..ac3fec1 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1040,6 +1040,8 @@ static int de_thread(struct task_struct *tsk) struct signal_struct *sig = tsk->signal; struct sighand_struct *oldsighand = tsk->sighand; spinlock_t *lock = &oldsighand->siglock; + unsigned int prev_ptrace = tsk->ptrace; + struct task_struct *t = tsk; if (thread_group_empty(tsk)) goto no_thread_group; @@ -1057,20 +1059,40 @@ static int de_thread(struct task_struct *tsk) return -EAGAIN; } + while_each_thread(tsk, t) { + if (unlikely(t->ptrace) && t != tsk->group_leader) + sig->unsafe_execve_in_progress = true; + } + sig->group_exit_task = tsk; sig->notify_count = zap_other_threads(tsk); if (!thread_group_leader(tsk)) sig->notify_count--; + spin_unlock_irq(lock); - while (sig->notify_count) { - __set_current_state(TASK_KILLABLE); - spin_unlock_irq(lock); + if (unlikely(sig->unsafe_execve_in_progress)) + mutex_unlock(&sig->cred_guard_mutex); + + for (;;) { + set_current_state(TASK_KILLABLE); + if (!sig->notify_count) + break; schedule(); if (__fatal_signal_pending(tsk)) goto killed; - spin_lock_irq(lock); } - spin_unlock_irq(lock); + __set_current_state(TASK_RUNNING); + + if (unlikely(sig->unsafe_execve_in_progress)) { + if (mutex_lock_killable(&sig->cred_guard_mutex)) + goto killed; + sig->unsafe_execve_in_progress = false; + if (!prev_ptrace && tsk->ptrace) { + sig->group_exit_task = NULL; + sig->notify_count = 0; + return -ERESTARTSYS; + } + } /* * At this point all other threads have exited, all we have to @@ -1255,8 +1277,11 @@ int begin_new_exec(struct linux_binprm * bprm) * Make this the only thread in the thread group. */ retval = de_thread(me); - if (retval) + if (retval) { + if (retval == -ERESTARTSYS) + bprm->point_of_no_return = false; goto out; + } /* * Cancel any io_uring activity across execve @@ -1466,6 +1491,11 @@ static int prepare_bprm_creds(struct linux_binprm *bprm) if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex)) return -ERESTARTNOINTR; + if (unlikely(current->signal->unsafe_execve_in_progress)) { + mutex_unlock(¤t->signal->cred_guard_mutex); + return -ERESTARTNOINTR; + } + bprm->cred = prepare_exec_creds(); if (likely(bprm->cred)) return 0; @@ -1482,7 +1512,8 @@ static void free_bprm(struct linux_binprm *bprm) } free_arg_pages(bprm); if (bprm->cred) { - mutex_unlock(¤t->signal->cred_guard_mutex); + if (!current->signal->unsafe_execve_in_progress) + mutex_unlock(¤t->signal->cred_guard_mutex); abort_creds(bprm->cred); } if (bprm->file) { diff --git a/fs/proc/base.c b/fs/proc/base.c index 3851bfc..3b2a55c 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2739,6 +2739,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, if (rv < 0) goto out_free; + if (unlikely(current->signal->unsafe_execve_in_progress)) { + mutex_unlock(¤t->signal->cred_guard_mutex); + rv = -ERESTARTNOINTR; + goto out_free; + } + rv = security_setprocattr(PROC_I(inode)->op.lsm, file->f_path.dentry->d_name.name, page, count); diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 3f6a0fc..220a083 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -214,6 +214,17 @@ struct signal_struct { #endif /* + * Set while execve is executing but is *not* holding + * cred_guard_mutex to avoid possible dead-locks. + * The cred_guard_mutex is released *after* de_thread() has + * called zap_other_threads(), therefore a fatal signal is + * guaranteed to be already pending in the unlikely event, that + * current->signal->unsafe_execve_in_progress happens to be + * true after the cred_guard_mutex was acquired. + */ + bool unsafe_execve_in_progress; + + /* * Thread is the potential origin of an oom condition; kill first on * oom */ @@ -227,6 +238,8 @@ struct signal_struct { struct mutex cred_guard_mutex; /* guard against foreign influences on * credential calculations * (notably. ptrace) + * Held while execve runs, except when + * a sibling thread is being traced. * Deprecated do not use in new code. * Use exec_update_lock instead. */ diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 61db50f..0cbc1eb 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -468,6 +468,14 @@ static int ptrace_traceme(void) { int ret = -EPERM; + if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex)) + return -ERESTARTNOINTR; + + if (unlikely(current->signal->unsafe_execve_in_progress)) { + mutex_unlock(¤t->signal->cred_guard_mutex); + return -ERESTARTNOINTR; + } + write_lock_irq(&tasklist_lock); /* Are we already being traced? */ if (!current->ptrace) { @@ -483,6 +491,7 @@ static int ptrace_traceme(void) } } write_unlock_irq(&tasklist_lock); + mutex_unlock(¤t->signal->cred_guard_mutex); return ret; } diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 1d60fc2..b1389ee 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1824,9 +1824,15 @@ static long seccomp_set_mode_filter(unsigned int flags, * Make sure we cannot change seccomp or nnp state via TSYNC * while another thread is in the middle of calling exec. */ - if (flags & SECCOMP_FILTER_FLAG_TSYNC && - mutex_lock_killable(¤t->signal->cred_guard_mutex)) - goto out_put_fd; + if (flags & SECCOMP_FILTER_FLAG_TSYNC) { + if (mutex_lock_killable(¤t->signal->cred_guard_mutex)) + goto out_put_fd; + + if (unlikely(current->signal->unsafe_execve_in_progress)) { + mutex_unlock(¤t->signal->cred_guard_mutex); + goto out_put_fd; + } + } spin_lock_irq(¤t->sighand->siglock); diff --git a/tools/testing/selftests/ptrace/vmaccess.c b/tools/testing/selftests/ptrace/vmaccess.c index 4db327b..c7c2242 100644 --- a/tools/testing/selftests/ptrace/vmaccess.c +++ b/tools/testing/selftests/ptrace/vmaccess.c @@ -39,8 +39,15 @@ static void *thread(void *arg) f = open(mm, O_RDONLY); ASSERT_GE(f, 0); close(f); - f = kill(pid, SIGCONT); - ASSERT_EQ(f, 0); + f = waitpid(-1, NULL, 0); + ASSERT_NE(f, -1); + ASSERT_NE(f, 0); + ASSERT_NE(f, pid); + f = waitpid(-1, NULL, 0); + ASSERT_EQ(f, pid); + f = waitpid(-1, NULL, 0); + ASSERT_EQ(f, -1); + ASSERT_EQ(errno, ECHILD); } TEST(attach) @@ -57,22 +64,24 @@ static void *thread(void *arg) sleep(1); k = ptrace(PTRACE_ATTACH, pid, 0L, 0L); - ASSERT_EQ(errno, EAGAIN); - ASSERT_EQ(k, -1); + ASSERT_EQ(k, 0); k = waitpid(-1, &s, WNOHANG); ASSERT_NE(k, -1); ASSERT_NE(k, 0); ASSERT_NE(k, pid); ASSERT_EQ(WIFEXITED(s), 1); ASSERT_EQ(WEXITSTATUS(s), 0); - sleep(1); - k = ptrace(PTRACE_ATTACH, pid, 0L, 0L); - ASSERT_EQ(k, 0); k = waitpid(-1, &s, 0); ASSERT_EQ(k, pid); ASSERT_EQ(WIFSTOPPED(s), 1); ASSERT_EQ(WSTOPSIG(s), SIGSTOP); - k = ptrace(PTRACE_DETACH, pid, 0L, 0L); + k = ptrace(PTRACE_CONT, pid, 0L, 0L); + ASSERT_EQ(k, 0); + k = waitpid(-1, &s, 0); + ASSERT_EQ(k, pid); + ASSERT_EQ(WIFSTOPPED(s), 1); + ASSERT_EQ(WSTOPSIG(s), SIGTRAP); + k = ptrace(PTRACE_CONT, pid, 0L, 0L); ASSERT_EQ(k, 0); k = waitpid(-1, &s, 0); ASSERT_EQ(k, pid);
This introduces signal->unsafe_execve_in_progress, which is used to fix the case when at least one of the sibling threads is traced, and therefore the trace process may dead-lock in ptrace_attach, but de_thread will need to wait for the tracer to continue execution. The solution is to detect this situation and allow ptrace_attach to continue, while de_thread() is still waiting for traced zombies to be eventually released. When the current thread changed the ptrace status from non-traced to traced, we can simply abort the whole execve and restart it by returning -ERESTARTSYS. This needs to be done before changing the thread leader, because the PTRACE_EVENT_EXEC needs to know the old thread pid. Although it is technically after the point of no return, we just have to reset bprm->point_of_no_return here, since at this time only the other threads have received a fatal signal, not the current thread. From the user's point of view the whole execve was simply delayed until after the ptrace_attach. Other threads die quickly since the cred_guard_mutex is released, but a deadly signal is already pending. In case the mutex_lock_killable misses the signal, ->unsafe_execve_in_progress makes sure they release the mutex immediately and return with -ERESTARTNOINTR. This means there is no API change, unlike the previous version of this patch which was discussed here: https://lore.kernel.org/lkml/b6537ae6-31b1-5c50-f32b-8b8332ace882@hotmail.de/ See tools/testing/selftests/ptrace/vmaccess.c for a test case that gets fixed by this change. Note that since the test case was originally designed to test the ptrace_attach returning an error in this situation, the test expectation needed to be adjusted, to allow the API to succeed at the first attempt. Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de> --- fs/exec.c | 45 ++++++++++++++++++++++++++----- fs/proc/base.c | 6 +++++ include/linux/sched/signal.h | 13 +++++++++ kernel/ptrace.c | 9 +++++++ kernel/seccomp.c | 12 ++++++--- tools/testing/selftests/ptrace/vmaccess.c | 25 +++++++++++------ 6 files changed, 92 insertions(+), 18 deletions(-)