This introduces signal->exec_bprm, 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 problem happens when a tracer tries to ptrace_attach to a multi-threaded process, that does an execve in one of the threads at the same time, without doing that in a forked sub-process. That means: There is a race condition, when one or more of the threads are already ptraced, but the thread that invoked the execve is not yet traced. Now in this case the execve locks the cred_guard_mutex and waits for de_thread to complete. But that waits for the traced sibling threads to exit, and those have to wait for the tracer to receive the exit signal, but the tracer cannot call wait right now, because it is waiting for the ptrace call to complete, and this never does not happen. The traced process and the tracer are now in a deadlock situation, and can only be killed by a fatal signal.

The solution is to detect this situation and allow ptrace_attach to continue by temporarily releasing the cred_guard_mutex, while de_thread() is still waiting for traced zombies to be eventually released by the tracer. In the case of the thread group leader we only have to wait for the thread to become a zombie, which may also need co-operation from the tracer due to PTRACE_O_TRACEEXIT.

When a tracer wants to ptrace_attach a task that already is in execve, we simply retry the ptrace_may_access check while temporarily installing the new credentials and dumpability which are about to be used after execve completes. If the ptrace_attach happens on a thread that is a sibling-thread of the thread doing execve, it is sufficient to check against the old credentials, as this thread will be waited for, before the new credentials are installed.

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, the non-zero current->signal->exec_bprm 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 | 69 ++++++++++++++++------- fs/proc/base.c | 6 ++ include/linux/cred.h | 1 + include/linux/sched/signal.h | 18 ++++++ kernel/cred.c | 28 +++++++-- kernel/ptrace.c | 32 +++++++++++ kernel/seccomp.c | 12 +++- tools/testing/selftests/ptrace/vmaccess.c | 23 +++++--- 8 files changed, 155 insertions(+), 34 deletions(-)

v10: Changes to previous version, make the PTRACE_ATTACH return -EAGAIN, instead of execve return -ERESTARTSYS. Added some lessions learned to the description.

v11: Check old and new credentials in PTRACE_ATTACH again without changing the API.

Note: I got actually one response from an automatic checker to the v11 patch,

https://lore.kernel.org/lkml/202107121344.wu68hEPF-lkp@intel.com/

which is complaining about:

...

...

...

kernel/ptrace.c:425:26: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected struct cred const *old_cred @@ got struct cred const [noderef] __rcu *real_cred @@

417 struct linux_binprm *bprm = task->signal->exec_bprm; 418 const struct cred *old_cred; 419 struct mm_struct *old_mm; 420 421 retval = down_write_killable(&task->signal->exec_update_lock); 422 if (retval) 423 goto unlock_creds; 424 task_lock(task);

425 old_cred = task->real_cred;

v12: Essentially identical to v11.

- Fixed a minor merge conflict in linux v5.17, and fixed the above mentioned nit by adding __rcu to the declaration.

- re-tested the patch with all linux versions from v5.11 to v6.6

v10 was an alternative approach which did imply an API change. But I would prefer to avoid such an API change.

The difficult part is getting the right dumpability flags assigned before de_thread starts, hope you like this version. If not, the v10 is of course also acceptable.

v13: Fixed duplicated Return section in function header of is_dumpability_changed which was reported by the kernel test robot

v14: rebased to v6.7, refreshed and retested. And added a more detailed description of the actual bug.

Thanks Bernd.

diff --git a/fs/exec.c b/fs/exec.c index 6d9ed2d765ef..f2cf7c58fe16 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1039,11 +1039,13 @@ static int exec_mmap(struct mm_struct *mm) return 0; }

-static int de_thread(struct task_struct *tsk) +static int de_thread(struct task_struct *tsk, struct linux_binprm *bprm) { struct signal_struct *sig = tsk->signal; struct sighand_struct *oldsighand = tsk->sighand; spinlock_t *lock = &oldsighand->siglock; + struct task_struct *t = tsk; + bool unsafe_execve_in_progress = false;

if (thread_group_empty(tsk)) goto no_thread_group; @@ -1066,6 +1068,19 @@ static int de_thread(struct task_struct *tsk) if (!thread_group_leader(tsk)) sig->notify_count--;

+ while_each_thread(tsk, t) { + if (unlikely(t->ptrace) + && (t != tsk->group_leader || !t->exit_state)) + unsafe_execve_in_progress = true; + } + + if (unlikely(unsafe_execve_in_progress)) { + spin_unlock_irq(lock); + sig->exec_bprm = bprm; + mutex_unlock(&sig->cred_guard_mutex); + spin_lock_irq(lock); + } + while (sig->notify_count) { __set_current_state(TASK_KILLABLE); spin_unlock_irq(lock); @@ -1156,6 +1171,11 @@ static int de_thread(struct task_struct *tsk) release_task(leader); }

+ if (unlikely(unsafe_execve_in_progress)) { + mutex_lock(&sig->cred_guard_mutex); + sig->exec_bprm = NULL; + } + sig->group_exec_task = NULL; sig->notify_count = 0;

@@ -1167,6 +1187,11 @@ static int de_thread(struct task_struct *tsk) return 0;

killed: + if (unlikely(unsafe_execve_in_progress)) { + mutex_lock(&sig->cred_guard_mutex); + sig->exec_bprm = NULL; + } + /* protects against exit_notify() and __exit_signal() */ read_lock(&tasklist_lock); sig->group_exec_task = NULL; @@ -1251,6 +1276,24 @@ int begin_new_exec(struct linux_binprm * bprm) if (retval) return retval;

+ /* If the binary is not readable then enforce mm->dumpable=0 */ + would_dump(bprm, bprm->file); + if (bprm->have_execfd) + would_dump(bprm, bprm->executable); + + /* + * Figure out dumpability. Note that this checking only of current + * is wrong, but userspace depends on it. This should be testing + * bprm->secureexec instead. + */ + if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP || + is_dumpability_changed(current_cred(), bprm->cred) || + !(uid_eq(current_euid(), current_uid()) && + gid_eq(current_egid(), current_gid()))) + set_dumpable(bprm->mm, suid_dumpable); + else + set_dumpable(bprm->mm, SUID_DUMP_USER); + /* * Ensure all future errors are fatal. */ @@ -1259,7 +1302,7 @@ int begin_new_exec(struct linux_binprm * bprm) /* * Make this the only thread in the thread group. */ - retval = de_thread(me); + retval = de_thread(me, bprm); if (retval) goto out;

@@ -1282,11 +1325,6 @@ int begin_new_exec(struct linux_binprm * bprm) if (retval) goto out;

- /* If the binary is not readable then enforce mm->dumpable=0 */ - would_dump(bprm, bprm->file); - if (bprm->have_execfd) - would_dump(bprm, bprm->executable); - /* * Release all of the old mmap stuff */ @@ -1348,18 +1386,6 @@ int begin_new_exec(struct linux_binprm * bprm)

me->sas_ss_sp = me->sas_ss_size = 0;

- /* - * Figure out dumpability. Note that this checking only of current - * is wrong, but userspace depends on it. This should be testing - * bprm->secureexec instead. - */ - if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP || - !(uid_eq(current_euid(), current_uid()) && - gid_eq(current_egid(), current_gid()))) - set_dumpable(current->mm, suid_dumpable); - else - set_dumpable(current->mm, SUID_DUMP_USER); - perf_event_exec(); __set_task_comm(me, kbasename(bprm->filename), true);

@@ -1478,6 +1504,11 @@ static int prepare_bprm_creds(struct linux_binprm *bprm) if (mutex_lock_interruptible(&current->signal->cred_guard_mutex)) return -ERESTARTNOINTR;

+ if (unlikely(current->signal->exec_bprm)) { + mutex_unlock(&current->signal->cred_guard_mutex); + return -ERESTARTNOINTR; + } + bprm->cred = prepare_exec_creds(); if (likely(bprm->cred)) return 0; diff --git a/fs/proc/base.c b/fs/proc/base.c index dd31e3b6bf77..99ff3420138b 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2784,6 +2784,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->exec_bprm)) { + mutex_unlock(&current->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/cred.h b/include/linux/cred.h index 2976f534a7a3..a1a1ac38f749 100644 --- a/include/linux/cred.h +++ b/include/linux/cred.h @@ -153,6 +153,7 @@ extern const struct cred *get_task_cred(struct task_struct *); extern struct cred *cred_alloc_blank(void); extern struct cred *prepare_creds(void); extern struct cred *prepare_exec_creds(void); +extern bool is_dumpability_changed(const struct cred *, const struct cred *); extern int commit_creds(struct cred *); extern void abort_creds(struct cred *); extern const struct cred *override_creds(const struct cred *); diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 3499c1a8b929..85d8f8f2f44f 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -234,9 +234,27 @@ struct signal_struct { struct mm_struct *oom_mm; /* recorded mm when the thread group got * killed by the oom killer */

+ struct linux_binprm *exec_bprm; /* Used to check ptrace_may_access + * against new credentials while + * de_thread is waiting for other + * traced threads to terminate. + * Set while de_thread is executing. + * 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->exec_bprm happens + * to be non-zero after the + * cred_guard_mutex was acquired. + */ + 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/cred.c b/kernel/cred.c index c033a201c808..72aadde3f10c 100644 --- a/kernel/cred.c +++ b/kernel/cred.c @@ -375,6 +375,28 @@ static bool cred_cap_issubset(const struct cred *set, const struct cred *subset) return false; }

+/** + * is_dumpability_changed - Will changing creds from old to new + * affect the dumpability in commit_creds? + * + * Return: false - dumpability will not be changed in commit_creds. + * true - dumpability will be changed to non-dumpable. + * + * @old: The old credentials + * @new: The new credentials + */ +bool is_dumpability_changed(const struct cred *old, const struct cred *new) +{ + if (!uid_eq(old->euid, new->euid) || + !gid_eq(old->egid, new->egid) || + !uid_eq(old->fsuid, new->fsuid) || + !gid_eq(old->fsgid, new->fsgid) || + !cred_cap_issubset(old, new)) + return true; + + return false; +} + /** * commit_creds - Install new credentials upon the current task * @new: The credentials to be assigned @@ -403,11 +425,7 @@ int commit_creds(struct cred *new) get_cred(new); /* we will require a ref for the subj creds too */

/* dumpability changes */ - if (!uid_eq(old->euid, new->euid) || - !gid_eq(old->egid, new->egid) || - !uid_eq(old->fsuid, new->fsuid) || - !gid_eq(old->fsgid, new->fsgid) || - !cred_cap_issubset(old, new)) { + if (is_dumpability_changed(old, new)) { if (task->mm) set_dumpable(task->mm, suid_dumpable); task->pdeath_signal = 0; diff --git a/kernel/ptrace.c b/kernel/ptrace.c index d8b5e13a2229..578bc02eea27 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -20,6 +20,7 @@ #include <linux/pagemap.h> #include <linux/ptrace.h> #include <linux/security.h> +#include <linux/binfmts.h> #include <linux/signal.h> #include <linux/uio.h> #include <linux/audit.h> @@ -435,6 +436,28 @@ static int ptrace_attach(struct task_struct *task, long request, if (retval) goto unlock_creds;

+ if (unlikely(task->in_execve)) { + struct linux_binprm *bprm = task->signal->exec_bprm; + const struct cred __rcu *old_cred; + struct mm_struct *old_mm; + + retval = down_write_killable(&task->signal->exec_update_lock); + if (retval) + goto unlock_creds; + task_lock(task); + old_cred = task->real_cred; + old_mm = task->mm; + rcu_assign_pointer(task->real_cred, bprm->cred); + task->mm = bprm->mm; + retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS); + rcu_assign_pointer(task->real_cred, old_cred); + task->mm = old_mm; + task_unlock(task); + up_write(&task->signal->exec_update_lock); + if (retval) + goto unlock_creds; + } + write_lock_irq(&tasklist_lock); retval = -EPERM; if (unlikely(task->exit_state)) @@ -508,6 +531,14 @@ static int ptrace_traceme(void) { int ret = -EPERM;

+ if (mutex_lock_interruptible(&current->signal->cred_guard_mutex)) + return -ERESTARTNOINTR; + + if (unlikely(current->signal->exec_bprm)) { + mutex_unlock(&current->signal->cred_guard_mutex); + return -ERESTARTNOINTR; + } + write_lock_irq(&tasklist_lock); /* Are we already being traced? */ if (!current->ptrace) { @@ -523,6 +554,7 @@ static int ptrace_traceme(void) } } write_unlock_irq(&tasklist_lock); + mutex_unlock(&current->signal->cred_guard_mutex);

return ret; } diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 255999ba9190..b29bbfa0b044 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1955,9 +1955,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(&current->signal->cred_guard_mutex)) - goto out_put_fd; + if (flags & SECCOMP_FILTER_FLAG_TSYNC) { + if (mutex_lock_killable(&current->signal->cred_guard_mutex)) + goto out_put_fd; + + if (unlikely(current->signal->exec_bprm)) { + mutex_unlock(&current->signal->cred_guard_mutex); + goto out_put_fd; + } + }

spin_lock_irq(&current->sighand->siglock);

diff --git a/tools/testing/selftests/ptrace/vmaccess.c b/tools/testing/selftests/ptrace/vmaccess.c index 4db327b44586..3b7d81fb99bb 100644 --- a/tools/testing/selftests/ptrace/vmaccess.c +++ b/tools/testing/selftests/ptrace/vmaccess.c @@ -39,8 +39,15 @@ TEST(vmaccess) 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 @@ TEST(attach)

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); + 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); 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);

On 1/15/24 20:37, Matthew Wilcox wrote:

On Mon, Jan 15, 2024 at 08:22:19PM +0100, Bernd Edlinger wrote:

...

This introduces signal->exec_bprm, 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.

Not entirely sure why I've been added to the cc; this doesn't seem like it's even remotely within my realm of expertise.

Ah, okay, never mind. A couple new email addresses were found this time when I used ./scripts/get_maintainer.pl

...

+++ b/include/linux/cred.h @@ -153,6 +153,7 @@ extern const struct cred *get_task_cred(struct task_struct *); extern struct cred *cred_alloc_blank(void); extern struct cred *prepare_creds(void); extern struct cred *prepare_exec_creds(void); +extern bool is_dumpability_changed(const struct cred *, const struct cred *);

Using 'extern' for function declarations is deprecated. More importantly, you have two arguments of the same type, and how do I know which one is which if you don't name them?

...

+++ b/kernel/cred.c @@ -375,6 +375,28 @@ static bool cred_cap_issubset(const struct cred *set, const struct cred *subset) return false; } +/**

• • is_dumpability_changed - Will changing creds from old to new
• • affect the dumpability in commit_creds?
• • Return: false - dumpability will not be changed in commit_creds.

• •     true  - dumpability will be changed to non-dumpable.
    

• • @old: The old credentials
• • @new: The new credentials
• */

Does kernel-doc really parse this correctly? Normal style would be:

Apparently yes, but I think I only added those lines to silence some automatic checking bots.

/**

• is_dumpability_changed - Will changing creds affect dumpability?
• @old: The old credentials.
• @new: The new credentials.
• If the @new credentials have no elevated privileges compared to the
• @old credentials, the task may remain dumpable. Otherwise we have
• to mark the task as undumpable to avoid information leaks from higher
• to lower privilege domains.
• Return: True if the task will become undumpable.

*/

Thanks a lot, that looks much better. I will use your suggestion as is, when I re-send the patch next time.

...

@@ -508,6 +531,14 @@ static int ptrace_traceme(void) { int ret = -EPERM;

• if (mutex_lock_interruptible(&current->signal->cred_guard_mutex))

• return -ERESTARTNOINTR;
  

Do you really want this to be interruptible by a timer signal or a window resize event?

I think that is kind of okay, as most of the existing users lock the mutex also interruptible, so I just wanted to follow those examples.

Thanks Bernd.

On 1/16/24 16:22, Oleg Nesterov wrote:

I'll try to recall this problem and actually read the patch tommorrow...

Hmm. but it doesn't apply to Linus's tree, you need to rebase it. In particular, please note the recent commit 5431fdd2c181dd2eac2 ("ptrace: Convert ptrace_attach() to use lock guards")

Oh, how ugly... Will this new C++-like "feature" ever make it into a stable branch?

On 01/15, Bernd Edlinger wrote:

...

The problem happens when a tracer tries to ptrace_attach to a multi-threaded process, that does an execve in one of the threads at the same time, without doing that in a forked sub-process. That means: There is a race condition, when one or more of the threads are already ptraced, but the thread that invoked the execve is not yet traced. Now in this case the execve locks the cred_guard_mutex and waits for de_thread to complete. But that waits for the traced sibling threads to exit, and those have to wait for the tracer to receive the exit signal, but the tracer cannot call wait right now, because it is waiting for the ptrace call to complete, and this never does not happen. The traced process and the tracer are now in a deadlock situation, and can only be killed by a fatal signal.

This looks very confusing to me. And even misleading.

So IIRC the problem is "simple".

de_thread() sleeps with cred_guard_mutex waiting for other threads to exit and pass release_task/__exit_signal.

If one of the sub-threads is traced, debugger should do ptrace_detach() or wait() to release this tracee, the killed tracee won't autoreap.

Yes. but the tracer has to do its job, and that is ptrace_attach the remaining treads, it does not know that it would avoid a dead-lock when it calls wait(), instead of ptrace_attach. It does not know that the tracee has just called execve in one of the not yet traced threads.

Now. If debugger tries to take the same cred_guard_mutex before detach/wait we have a deadlock. This is not specific to ptrace_attach(), proc_pid_attr_write() takes this lock too.

Right? Or are there other issues?

No, proc_pid_attr_write has no problem if it waits for cred_guard_mutex, because it is only called from one of the sibling threads, and zap_other_threads sends a SIGKILL to each of them, thus the mutex_lock_interruptible will stop waiting, and the thread will exit normally. It is only problematic when another process wants to lock the cred_guard_mutex, because it is not receiving a signal, when de_thread is waiting. The only other place where I am aware of this happening is ptrace_attach.

...

-static int de_thread(struct task_struct *tsk) +static int de_thread(struct task_struct *tsk, struct linux_binprm *bprm) { struct signal_struct *sig = tsk->signal; struct sighand_struct *oldsighand = tsk->sighand; spinlock_t *lock = &oldsighand->siglock;

• struct task_struct *t = tsk;

• bool unsafe_execve_in_progress = false;

  if (thread_group_empty(tsk)) goto no_thread_group;

@@ -1066,6 +1068,19 @@ static int de_thread(struct task_struct *tsk) if (!thread_group_leader(tsk)) sig->notify_count--;

• while_each_thread(tsk, t) {

for_other_threads()

Ah, okay.

...

• if (unlikely(t->ptrace)
  

•     && (t != tsk->group_leader || !t->exit_state))
  

• 	unsafe_execve_in_progress = true;
  

The !t->exit_state is not right... This sub-thread can already be a zombie with ->exit_state != 0 but see above, it won't be reaped until the debugger does wait().

I dont think so. de_thread() handles the group_leader different than normal threads. That means normal threads have to wait for being released from the zombie state by the tracer: sig->notify_count > 0, and de_thread is woken up by __exit_signal Once those are gone, de_thread waits for the group leader to reach exit_state = ZOMBIE, but again only if the group_leader is not the current thread: signal->notify_count < 0, and de_thread is woken up by exit_notify. So his reflects exactly what condition has to be met, see:

sig->notify_count = -1; if (likely(leader->exit_state)) break; __set_current_state(TASK_KILLABLE); write_unlock_irq(&tasklist_lock); cgroup_threadgroup_change_end(tsk); schedule(); if (__fatal_signal_pending(tsk)) goto killed;

so when the group_leader's exit_state is already != 0 then the second wait state will not be entered.

...

• if (unlikely(unsafe_execve_in_progress)) {

• spin_unlock_irq(lock);
  

• sig->exec_bprm = bprm;
  

• mutex_unlock(&sig->cred_guard_mutex);
  

• spin_lock_irq(lock);
  

I don't understand why do we need to unlock and lock siglock here...

That is just a precaution because I did want to release the mutexes exactly in the reverse order as they were acquired.

But my main question is why do we need the unsafe_execve_in_progress boolean. If this patch is correct and de_thread() can drop and re-acquire cread_guard_mutex when one of the threads is traced, then why can't we do this unconditionally ?

I just wanted to keep the impact of the change as small as possible, including possible performance degradation due to double checking of credentials. Worst thing that could happen with this approach, is that a situation where today a dead-lock is imminentm does still not work correctly, but when no tracer is attached, nothing will change or be less performant than before.

Bernd.

Hello Bernd, As it's been a long standing issue, and test still fails.

Will you be working on fixing it again in future? or it might be better to remove the testcase?

$ ./vmaccess TAP version 13 1..2 # Starting 2 tests from 1 test cases. # RUN global.vmaccess ... # OK global.vmaccess ok 1 global.vmaccess # RUN global.attach ... # attach: Test terminated by timeout # FAIL global.attach not ok 2 global.attach # FAILED: 1 / 2 tests passed. # Totals: pass:1 fail:1 xfail:0 xpass:0 skip:0 error:0

Thanks, Ayush

On 1/23/2024 12:01 AM, Bernd Edlinger wrote:

This introduces signal->exec_bprm, 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 problem happens when a tracer tries to ptrace_attach to a multi-threaded process, that does an execve in one of the threads at the same time, without doing that in a forked sub-process. That means: There is a race condition, when one or more of the threads are already ptraced, but the thread that invoked the execve is not yet traced. Now in this case the execve locks the cred_guard_mutex and waits for de_thread to complete. But that waits for the traced sibling threads to exit, and those have to wait for the tracer to receive the exit signal, but the tracer cannot call wait right now, because it is waiting for the ptrace call to complete, and this never does not happen. The traced process and the tracer are now in a deadlock situation, and can only be killed by a fatal signal.

The solution is to detect this situation and allow ptrace_attach to continue by temporarily releasing the cred_guard_mutex, while de_thread() is still waiting for traced zombies to be eventually released by the tracer. In the case of the thread group leader we only have to wait for the thread to become a zombie, which may also need co-operation from the tracer due to PTRACE_O_TRACEEXIT.

When a tracer wants to ptrace_attach a task that already is in execve, we simply retry the ptrace_may_access check while temporarily installing the new credentials and dumpability which are about to be used after execve completes. If the ptrace_attach happens on a thread that is a sibling-thread of the thread doing execve, it is sufficient to check against the old credentials, as this thread will be waited for, before the new credentials are installed.

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, the non-zero current->signal->exec_bprm 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 | 69 ++++++++--- fs/proc/base.c | 6 + include/linux/cred.h | 1 + include/linux/sched/signal.h | 18 +++ kernel/cred.c | 30 ++++- kernel/ptrace.c | 31 +++++ kernel/seccomp.c | 12 +- tools/testing/selftests/ptrace/vmaccess.c | 135 ++++++++++++++++++++-- 8 files changed, 265 insertions(+), 37 deletions(-)

v10: Changes to previous version, make the PTRACE_ATTACH return -EAGAIN, instead of execve return -ERESTARTSYS. Added some lessions learned to the description.

v11: Check old and new credentials in PTRACE_ATTACH again without changing the API.

Note: I got actually one response from an automatic checker to the v11 patch,

https://lore.kernel.org/lkml/202107121344.wu68hEPF-lkp@intel.com/

which is complaining about:

...

...

...

...

kernel/ptrace.c:425:26: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected struct cred const *old_cred @@ got struct cred const [noderef] __rcu *real_cred @@

417 struct linux_binprm *bprm = task->signal->exec_bprm; 418 const struct cred *old_cred; 419 struct mm_struct *old_mm; 420 421 retval = down_write_killable(&task->signal->exec_update_lock); 422 if (retval) 423 goto unlock_creds; 424 task_lock(task);

...

425 old_cred = task->real_cred;

v12: Essentially identical to v11.

• Fixed a minor merge conflict in linux v5.17, and fixed the

above mentioned nit by adding __rcu to the declaration.

• re-tested the patch with all linux versions from v5.11 to v6.6

v10 was an alternative approach which did imply an API change. But I would prefer to avoid such an API change.

The difficult part is getting the right dumpability flags assigned before de_thread starts, hope you like this version. If not, the v10 is of course also acceptable.

v13: Fixed duplicated Return section in function header of is_dumpability_changed which was reported by the kernel test robot

v14: rebased to v6.7, refreshed and retested. And added a more detailed description of the actual bug.

v15: rebased to v6.8-rc1, addressed some review comments. Split the test case vmaccess into vmaccess1 and vmaccess2 to improve overall test coverage.

Thanks Bernd.

diff --git a/fs/exec.c b/fs/exec.c index e88249a1ce07..499380d74899 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1040,11 +1040,13 @@ static int exec_mmap(struct mm_struct *mm) return 0; } -static int de_thread(struct task_struct *tsk) +static int de_thread(struct task_struct *tsk, struct linux_binprm *bprm) { struct signal_struct *sig = tsk->signal; struct sighand_struct *oldsighand = tsk->sighand; spinlock_t *lock = &oldsighand->siglock;

• struct task_struct *t;
• bool unsafe_execve_in_progress = false;

if (thread_group_empty(tsk)) goto no_thread_group; @@ -1067,6 +1069,19 @@ static int de_thread(struct task_struct *tsk) if (!thread_group_leader(tsk)) sig->notify_count--;

• for_other_threads(tsk, t) {

• if (unlikely(t->ptrace)
  

•     && (t != tsk->group_leader || !t->exit_state))
  

• 	unsafe_execve_in_progress = true;
  

• }
• if (unlikely(unsafe_execve_in_progress)) {

• spin_unlock_irq(lock);
  

• sig->exec_bprm = bprm;
  

• mutex_unlock(&sig->cred_guard_mutex);
  

• spin_lock_irq(lock);
  

• }
• while (sig->notify_count) { __set_current_state(TASK_KILLABLE); spin_unlock_irq(lock);

@@ -1157,6 +1172,11 @@ static int de_thread(struct task_struct *tsk) release_task(leader); }

• if (unlikely(unsafe_execve_in_progress)) {

• mutex_lock(&sig->cred_guard_mutex);
  

• sig->exec_bprm = NULL;
  

• }
• sig->group_exec_task = NULL; sig->notify_count = 0;

@@ -1168,6 +1188,11 @@ static int de_thread(struct task_struct *tsk) return 0; killed:

• if (unlikely(unsafe_execve_in_progress)) {

• mutex_lock(&sig->cred_guard_mutex);
  

• sig->exec_bprm = NULL;
  

• }
• /* protects against exit_notify() and __exit_signal() */ read_lock(&tasklist_lock); sig->group_exec_task = NULL;

@@ -1252,6 +1277,24 @@ int begin_new_exec(struct linux_binprm * bprm) if (retval) return retval;

• /* If the binary is not readable then enforce mm->dumpable=0 */
• would_dump(bprm, bprm->file);
• if (bprm->have_execfd)

• would_dump(bprm, bprm->executable);
  

• /*

• * Figure out dumpability. Note that this checking only of current
  

• * is wrong, but userspace depends on it. This should be testing
  

• * bprm->secureexec instead.
  

• */
  

• if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||

•    is_dumpability_changed(current_cred(), bprm->cred) ||
  

•    !(uid_eq(current_euid(), current_uid()) &&
  

•      gid_eq(current_egid(), current_gid())))
  

• set_dumpable(bprm->mm, suid_dumpable);
  

• else

• set_dumpable(bprm->mm, SUID_DUMP_USER);
  

• /*
  • Ensure all future errors are fatal.
  */

@@ -1260,7 +1303,7 @@ int begin_new_exec(struct linux_binprm * bprm) /* * Make this the only thread in the thread group. */

• retval = de_thread(me);

• retval = de_thread(me, bprm); if (retval) goto out;

@@ -1283,11 +1326,6 @@ int begin_new_exec(struct linux_binprm * bprm) if (retval) goto out;

• /* If the binary is not readable then enforce mm->dumpable=0 */
• would_dump(bprm, bprm->file);
• if (bprm->have_execfd)

• would_dump(bprm, bprm->executable);
  

• /*
  • Release all of the old mmap stuff
  */

@@ -1349,18 +1387,6 @@ int begin_new_exec(struct linux_binprm * bprm) me->sas_ss_sp = me->sas_ss_size = 0;

• /*

• * Figure out dumpability. Note that this checking only of current
  

• * is wrong, but userspace depends on it. This should be testing
  

• * bprm->secureexec instead.
  

• */
  

• if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||

•    !(uid_eq(current_euid(), current_uid()) &&
  

•      gid_eq(current_egid(), current_gid())))
  

• set_dumpable(current->mm, suid_dumpable);
  

• else

• set_dumpable(current->mm, SUID_DUMP_USER);
  

• perf_event_exec(); __set_task_comm(me, kbasename(bprm->filename), true);

@@ -1479,6 +1505,11 @@ static int prepare_bprm_creds(struct linux_binprm *bprm) if (mutex_lock_interruptible(&current->signal->cred_guard_mutex)) return -ERESTARTNOINTR;

• if (unlikely(current->signal->exec_bprm)) {

• mutex_unlock(&current->signal->cred_guard_mutex);
  

• return -ERESTARTNOINTR;
  

• }
• bprm->cred = prepare_exec_creds(); if (likely(bprm->cred)) return 0;

diff --git a/fs/proc/base.c b/fs/proc/base.c index 98a031ac2648..eab3461e4da7 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2785,6 +2785,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->exec_bprm)) {

• mutex_unlock(&current->signal->cred_guard_mutex);
  

• rv = -ERESTARTNOINTR;
  

• goto out_free;
  

• }
• rv = security_setprocattr(PROC_I(inode)->op.lsmid, file->f_path.dentry->d_name.name, page, count);

diff --git a/include/linux/cred.h b/include/linux/cred.h index 2976f534a7a3..a1a1ac38f749 100644 --- a/include/linux/cred.h +++ b/include/linux/cred.h @@ -153,6 +153,7 @@ extern const struct cred *get_task_cred(struct task_struct *); extern struct cred *cred_alloc_blank(void); extern struct cred *prepare_creds(void); extern struct cred *prepare_exec_creds(void); +extern bool is_dumpability_changed(const struct cred *, const struct cred *); extern int commit_creds(struct cred *); extern void abort_creds(struct cred *); extern const struct cred *override_creds(const struct cred *); diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 4b7664c56208..6364e115e9e9 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -235,9 +235,27 @@ struct signal_struct { struct mm_struct *oom_mm; /* recorded mm when the thread group got * killed by the oom killer */

• struct linux_binprm *exec_bprm; /* Used to check ptrace_may_access

• 			 * against new credentials while
  

• 			 * de_thread is waiting for other
  

• 			 * traced threads to terminate.
  

• 			 * Set while de_thread is executing.
  

• 			 * 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->exec_bprm happens
  

• 			 * to be non-zero after the
  

• 			 * cred_guard_mutex was acquired.
  

• 			 */
  

• 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/cred.c b/kernel/cred.c index c033a201c808..0066b5b0f052 100644 --- a/kernel/cred.c +++ b/kernel/cred.c @@ -375,6 +375,30 @@ static bool cred_cap_issubset(const struct cred *set, const struct cred *subset) return false; } +/**

• • is_dumpability_changed - Will changing creds affect dumpability?
• • @old: The old credentials.
• • @new: The new credentials.
• • If the @new credentials have no elevated privileges compared to the
• • @old credentials, the task may remain dumpable. Otherwise we have
• • to mark the task as undumpable to avoid information leaks from higher
• • to lower privilege domains.
• • Return: True if the task will become undumpable.
• */

+bool is_dumpability_changed(const struct cred *old, const struct cred *new) +{

• if (!uid_eq(old->euid, new->euid) ||

•    !gid_eq(old->egid, new->egid) ||
  

•    !uid_eq(old->fsuid, new->fsuid) ||
  

•    !gid_eq(old->fsgid, new->fsgid) ||
  

•    !cred_cap_issubset(old, new))
  

• return true;
  

• return false;

+}

/**

• commit_creds - Install new credentials upon the current task
• @new: The credentials to be assigned

@@ -403,11 +427,7 @@ int commit_creds(struct cred *new) get_cred(new); /* we will require a ref for the subj creds too */ /* dumpability changes */

• if (!uid_eq(old->euid, new->euid) ||

•    !gid_eq(old->egid, new->egid) ||
  

•    !uid_eq(old->fsuid, new->fsuid) ||
  

•    !gid_eq(old->fsgid, new->fsgid) ||
  

•    !cred_cap_issubset(old, new)) {
  

• if (is_dumpability_changed(old, new)) { if (task->mm) set_dumpable(task->mm, suid_dumpable); task->pdeath_signal = 0;

diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 2fabd497d659..4b9a951b38f1 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -20,6 +20,7 @@ #include <linux/pagemap.h> #include <linux/ptrace.h> #include <linux/security.h> +#include <linux/binfmts.h> #include <linux/signal.h> #include <linux/uio.h> #include <linux/audit.h> @@ -450,6 +451,27 @@ static int ptrace_attach(struct task_struct *task, long request, return retval; }

• if (unlikely(task->in_execve)) {
  

• 	retval = down_write_killable(&task->signal->exec_update_lock);
  

• 	if (retval)
  

• 		return retval;
  

• 	scoped_guard (task_lock, task) {
  

• 		struct linux_binprm *bprm = task->signal->exec_bprm;
  

• 		const struct cred __rcu *old_cred = task->real_cred;
  

• 		struct mm_struct *old_mm = task->mm;
  

• 		rcu_assign_pointer(task->real_cred, bprm->cred);
  

• 		task->mm = bprm->mm;
  

• 		retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
  

• 		rcu_assign_pointer(task->real_cred, old_cred);
  

• 		task->mm = old_mm;
  

• 	}
  

• 	up_write(&task->signal->exec_update_lock);
  

• 	if (retval)
  

• 		return retval;
  

• }
  

• scoped_guard (write_lock_irq, &tasklist_lock) { if (unlikely(task->exit_state)) return -EPERM;

@@ -491,6 +513,14 @@ static int ptrace_traceme(void) { int ret = -EPERM;

• if (mutex_lock_interruptible(&current->signal->cred_guard_mutex))

• return -ERESTARTNOINTR;
  

• if (unlikely(current->signal->exec_bprm)) {

• mutex_unlock(&current->signal->cred_guard_mutex);
  

• return -ERESTARTNOINTR;
  

• }
• write_lock_irq(&tasklist_lock); /* Are we already being traced? */ if (!current->ptrace) {

@@ -506,6 +536,7 @@ static int ptrace_traceme(void) } } write_unlock_irq(&tasklist_lock);

• mutex_unlock(&current->signal->cred_guard_mutex);

return ret; } diff --git a/kernel/seccomp.c b/kernel/seccomp.c index aca7b437882e..32ed0da5939a 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1955,9 +1955,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(&current->signal->cred_guard_mutex))
  

• goto out_put_fd;
  

• if (flags & SECCOMP_FILTER_FLAG_TSYNC) {

• if (mutex_lock_killable(&current->signal->cred_guard_mutex))
  

• 	goto out_put_fd;
  

• if (unlikely(current->signal->exec_bprm)) {
  

• 	mutex_unlock(&current->signal->cred_guard_mutex);
  

• 	goto out_put_fd;
  

• }
  

• }

spin_lock_irq(&current->sighand->siglock); diff --git a/tools/testing/selftests/ptrace/vmaccess.c b/tools/testing/selftests/ptrace/vmaccess.c index 4db327b44586..5d4a65eb5a8d 100644 --- a/tools/testing/selftests/ptrace/vmaccess.c +++ b/tools/testing/selftests/ptrace/vmaccess.c @@ -14,6 +14,7 @@ #include <signal.h> #include <unistd.h> #include <sys/ptrace.h> +#include <sys/syscall.h> static void *thread(void *arg) { @@ -23,7 +24,7 @@ static void *thread(void *arg) TEST(vmaccess) {

• int f, pid = fork();

• int s, f, pid = fork(); char mm[64];

if (!pid) { @@ -31,19 +32,42 @@ TEST(vmaccess) pthread_create(&pt, NULL, thread, NULL); pthread_join(pt, NULL);

• execlp("true", "true", NULL);
  

• execlp("false", "false", NULL);
  

• return;
  

  }

sleep(1); sprintf(mm, "/proc/%d/mem", pid);

• /* deadlock did happen here */ f = open(mm, O_RDONLY); ASSERT_GE(f, 0); close(f);

• f = kill(pid, SIGCONT);
• ASSERT_EQ(f, 0);

• f = waitpid(-1, &s, WNOHANG);
• ASSERT_NE(f, -1);
• ASSERT_NE(f, 0);
• ASSERT_NE(f, pid);
• ASSERT_EQ(WIFEXITED(s), 1);
• ASSERT_EQ(WEXITSTATUS(s), 0);
• f = waitpid(-1, &s, 0);
• ASSERT_EQ(f, pid);
• ASSERT_EQ(WIFEXITED(s), 1);
• ASSERT_EQ(WEXITSTATUS(s), 1);
• f = waitpid(-1, NULL, 0);
• ASSERT_EQ(f, -1);
• ASSERT_EQ(errno, ECHILD);

} -TEST(attach) +/*

• • Same test as previous, except that
• • we try to ptrace the group leader,
• • which is about to call execve,
• • when the other thread is already ptraced.
• • This exercises the code in de_thread
• • where it is waiting inside the
• • while (sig->notify_count) {
• • loop.
• */

+TEST(attach1) { int s, k, pid = fork(); @@ -52,19 +76,76 @@ TEST(attach) pthread_create(&pt, NULL, thread, NULL); pthread_join(pt, NULL);

• execlp("sleep", "sleep", "2", NULL);
  

• execlp("false", "false", NULL);
  

• return;
  

  }

sleep(1);

• /* deadlock may happen here */ 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);
• k = waitpid(-1, &s, 0);
• ASSERT_EQ(k, pid);
• ASSERT_EQ(WIFSTOPPED(s), 1);
• ASSERT_EQ(WSTOPSIG(s), SIGTRAP);
• k = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, 0);
• 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), SIGSTOP);
• k = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, 0);
• k = ptrace(PTRACE_CONT, pid, 0L, 0L);
• ASSERT_EQ(k, 0);
• k = waitpid(-1, &s, 0);
• ASSERT_EQ(k, pid);
• ASSERT_EQ(WIFEXITED(s), 1);
• ASSERT_EQ(WEXITSTATUS(s), 1);
• k = waitpid(-1, NULL, 0);
• ASSERT_EQ(k, -1);
• ASSERT_EQ(errno, ECHILD);

+}

+/*

• • Same test as previous, except that
• • the group leader is ptraced first,
• • but this time with PTRACE_O_TRACEEXIT,
• • and the thread that does execve is
• • not yet ptraced. This exercises the
• • code block in de_thread where the
• • if (!thread_group_leader(tsk)) {
• • is executed and enters a wait state.
• */

+static long thread2_tid; +static void *thread2(void *arg) +{

• thread2_tid = syscall(__NR_gettid);
• sleep(2);
• execlp("false", "false", NULL);
• return NULL;

+}

+TEST(attach2) +{

• int s, k, pid = fork();
• if (!pid) {

• pthread_t pt;
  

• pthread_create(&pt, NULL, thread2, NULL);
  

• pthread_join(pt, NULL);
  

• return;
  

• }
• sleep(1); k = ptrace(PTRACE_ATTACH, pid, 0L, 0L); ASSERT_EQ(k, 0);

@@ -72,12 +153,46 @@ TEST(attach) ASSERT_EQ(k, pid); ASSERT_EQ(WIFSTOPPED(s), 1); ASSERT_EQ(WSTOPSIG(s), SIGSTOP);

• k = ptrace(PTRACE_DETACH, pid, 0L, 0L);

• k = ptrace(PTRACE_SETOPTIONS, pid, 0L, PTRACE_O_TRACEEXIT);
• ASSERT_EQ(k, 0);
• thread2_tid = ptrace(PTRACE_PEEKDATA, pid, &thread2_tid, 0L);
• ASSERT_NE(thread2_tid, -1);
• ASSERT_NE(thread2_tid, 0);
• ASSERT_NE(thread2_tid, pid);
• k = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, 0);
• sleep(2);
• /* deadlock may happen here */
• k = ptrace(PTRACE_ATTACH, thread2_tid, 0L, 0L);
• ASSERT_EQ(k, 0);
• k = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, pid);
• ASSERT_EQ(WIFSTOPPED(s), 1);
• ASSERT_EQ(WSTOPSIG(s), SIGTRAP);
• k = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, 0);
• 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 = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, 0);
• 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), SIGSTOP);
• k = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, 0);
• k = ptrace(PTRACE_CONT, pid, 0L, 0L); ASSERT_EQ(k, 0); k = waitpid(-1, &s, 0); ASSERT_EQ(k, pid); ASSERT_EQ(WIFEXITED(s), 1);

• ASSERT_EQ(WEXITSTATUS(s), 0);

• ASSERT_EQ(WEXITSTATUS(s), 1); k = waitpid(-1, NULL, 0); ASSERT_EQ(k, -1); ASSERT_EQ(errno, ECHILD);

On Mon, Aug 18, 2025 at 10:53:43PM +0200, Bernd Edlinger wrote:

This introduces signal->exec_bprm, 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 problem happens when a tracer tries to ptrace_attach to a multi-threaded process, that does an execve in one of the threads at the same time, without doing that in a forked sub-process. That means: There is a race condition, when one or more of the threads are already ptraced, but the thread that invoked the execve is not yet traced. Now in this case the execve locks the cred_guard_mutex and waits for de_thread to complete. But that waits for the traced sibling threads to exit, and those have to wait for the tracer to receive the exit signal, but the tracer cannot call wait right now, because it is waiting for the ptrace call to complete, and this never does not happen. The traced process and the tracer are now in a deadlock situation, and can only be killed by a fatal signal.

The solution is to detect this situation and allow ptrace_attach to continue by temporarily releasing the cred_guard_mutex, while de_thread() is still waiting for traced zombies to be eventually released by the tracer. In the case of the thread group leader we only have to wait for the thread to become a zombie, which may also need co-operation from the tracer due to PTRACE_O_TRACEEXIT.

When a tracer wants to ptrace_attach a task that already is in execve, we simply retry the ptrace_may_access check while temporarily installing the new credentials and dumpability which are about to be used after execve completes. If the ptrace_attach happens on a thread that is a sibling-thread of the thread doing execve, it is sufficient to check against the old credentials, as this thread will be waited for, before the new credentials are installed.

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, the non-zero current->signal->exec_bprm 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 | 69 ++++++++--- fs/proc/base.c | 6 + include/linux/cred.h | 1 + include/linux/sched/signal.h | 18 +++ kernel/cred.c | 30 ++++- kernel/ptrace.c | 31 +++++ kernel/seccomp.c | 12 +- tools/testing/selftests/ptrace/vmaccess.c | 135 ++++++++++++++++++++-- 8 files changed, 265 insertions(+), 37 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c index 2a1e5e4042a1..31c6ceaa5f69 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -905,11 +905,13 @@ static int exec_mmap(struct mm_struct *mm) return 0; } -static int de_thread(struct task_struct *tsk) +static int de_thread(struct task_struct *tsk, struct linux_binprm *bprm) { struct signal_struct *sig = tsk->signal; struct sighand_struct *oldsighand = tsk->sighand; spinlock_t *lock = &oldsighand->siglock;

• struct task_struct *t;
• bool unsafe_execve_in_progress = false;

if (thread_group_empty(tsk)) goto no_thread_group; @@ -932,6 +934,19 @@ static int de_thread(struct task_struct *tsk) if (!thread_group_leader(tsk)) sig->notify_count--;

• for_other_threads(tsk, t) {

• if (unlikely(t->ptrace)
  

•     && (t != tsk->group_leader || !t->exit_state))
  

• 	unsafe_execve_in_progress = true;
  

• }
• if (unlikely(unsafe_execve_in_progress)) {

• spin_unlock_irq(lock);
  

• sig->exec_bprm = bprm;
  

• mutex_unlock(&sig->cred_guard_mutex);
  

• spin_lock_irq(lock);
  

• }

cred_guard_mutex has a comment about it being deprecated and shouldn't be used in "new code"... Regardless, what cred_guard_mutex is trying to protect is changing to credentials.

But what we want to stop is having new threads appear, which spin_lock_irq(lock) should stop, yes?

while (sig->notify_count) { __set_current_state(TASK_KILLABLE); spin_unlock_irq(lock); @@ -1021,6 +1036,11 @@ static int de_thread(struct task_struct *tsk) release_task(leader); }

• if (unlikely(unsafe_execve_in_progress)) {

• mutex_lock(&sig->cred_guard_mutex);
  

• sig->exec_bprm = NULL;
  

• }
• sig->group_exec_task = NULL; sig->notify_count = 0;

@@ -1032,6 +1052,11 @@ static int de_thread(struct task_struct *tsk) return 0; killed:

• if (unlikely(unsafe_execve_in_progress)) {

• mutex_lock(&sig->cred_guard_mutex);
  

• sig->exec_bprm = NULL;
  

• }

I think we need to document that cred_guard_mutex now protects sig->exec_bprm.

• /* protects against exit_notify() and __exit_signal() */ read_lock(&tasklist_lock); sig->group_exec_task = NULL;

@@ -1114,13 +1139,31 @@ int begin_new_exec(struct linux_binprm * bprm) */ trace_sched_prepare_exec(current, bprm);

• /* If the binary is not readable then enforce mm->dumpable=0 */
• would_dump(bprm, bprm->file);
• if (bprm->have_execfd)

• would_dump(bprm, bprm->executable);
  

• /*

• * Figure out dumpability. Note that this checking only of current
  

• * is wrong, but userspace depends on it. This should be testing
  

• * bprm->secureexec instead.
  

• */
  

• if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||

•    is_dumpability_changed(current_cred(), bprm->cred) ||
  

•    !(uid_eq(current_euid(), current_uid()) &&
  

•      gid_eq(current_egid(), current_gid())))
  

• set_dumpable(bprm->mm, suid_dumpable);
  

• else

• set_dumpable(bprm->mm, SUID_DUMP_USER);
  

Why is this move needed? While it's writing to bprm, I see it's reading from "current". Is this safe to do before de_thread() has happened? Can't a sibling thread manipulate things here? What lock am I missing?

/* * Ensure all future errors are fatal. */ bprm->point_of_no_return = true; /* Make this the only thread in the thread group */

• retval = de_thread(me);

• retval = de_thread(me, bprm); if (retval) goto out; /* see the comment in check_unsafe_exec() */

@@ -1144,11 +1187,6 @@ int begin_new_exec(struct linux_binprm * bprm) if (retval) goto out;

• /* If the binary is not readable then enforce mm->dumpable=0 */
• would_dump(bprm, bprm->file);
• if (bprm->have_execfd)

• would_dump(bprm, bprm->executable);
  

• /*
  • Release all of the old mmap stuff
  */

@@ -1210,18 +1248,6 @@ int begin_new_exec(struct linux_binprm * bprm) me->sas_ss_sp = me->sas_ss_size = 0;

• /*

• * Figure out dumpability. Note that this checking only of current
  

• * is wrong, but userspace depends on it. This should be testing
  

• * bprm->secureexec instead.
  

• */
  

• if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||

•    !(uid_eq(current_euid(), current_uid()) &&
  

•      gid_eq(current_egid(), current_gid())))
  

• set_dumpable(current->mm, suid_dumpable);
  

• else

• set_dumpable(current->mm, SUID_DUMP_USER);
  

• perf_event_exec();

/* @@ -1361,6 +1387,11 @@ static int prepare_bprm_creds(struct linux_binprm *bprm) if (mutex_lock_interruptible(&current->signal->cred_guard_mutex)) return -ERESTARTNOINTR;

• if (unlikely(current->signal->exec_bprm)) {

• mutex_unlock(&current->signal->cred_guard_mutex);
  

• return -ERESTARTNOINTR;
  

• }
• bprm->cred = prepare_exec_creds(); if (likely(bprm->cred)) return 0;

diff --git a/fs/proc/base.c b/fs/proc/base.c index 62d35631ba8c..e5bcf812cee0 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2838,6 +2838,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->exec_bprm)) {

• mutex_unlock(&current->signal->cred_guard_mutex);
  

• rv = -ERESTARTNOINTR;
  

• goto out_free;
  

• }
• rv = security_setprocattr(PROC_I(inode)->op.lsmid, file->f_path.dentry->d_name.name, page, count);

diff --git a/include/linux/cred.h b/include/linux/cred.h index a102a10f833f..fb0361911489 100644 --- a/include/linux/cred.h +++ b/include/linux/cred.h @@ -153,6 +153,7 @@ extern const struct cred *get_task_cred(struct task_struct *); extern struct cred *cred_alloc_blank(void); extern struct cred *prepare_creds(void); extern struct cred *prepare_exec_creds(void); +extern bool is_dumpability_changed(const struct cred *, const struct cred *); extern int commit_creds(struct cred *); extern void abort_creds(struct cred *); extern struct cred *prepare_kernel_cred(struct task_struct *); diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 1ef1edbaaf79..3c47d8b55863 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -237,9 +237,27 @@ struct signal_struct { struct mm_struct *oom_mm; /* recorded mm when the thread group got * killed by the oom killer */

• struct linux_binprm *exec_bprm; /* Used to check ptrace_may_access

• 			 * against new credentials while
  

• 			 * de_thread is waiting for other
  

• 			 * traced threads to terminate.
  

• 			 * Set while de_thread is executing.
  

• 			 * 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->exec_bprm happens
  

• 			 * to be non-zero after the
  

• 			 * cred_guard_mutex was acquired.
  

• 			 */
  

• 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/cred.c b/kernel/cred.c index 9676965c0981..0b2822c762df 100644 --- a/kernel/cred.c +++ b/kernel/cred.c @@ -375,6 +375,30 @@ static bool cred_cap_issubset(const struct cred *set, const struct cred *subset) return false; } +/**

• • is_dumpability_changed - Will changing creds affect dumpability?
• • @old: The old credentials.
• • @new: The new credentials.
• • If the @new credentials have no elevated privileges compared to the
• • @old credentials, the task may remain dumpable. Otherwise we have
• • to mark the task as undumpable to avoid information leaks from higher
• • to lower privilege domains.
• • Return: True if the task will become undumpable.
• */

+bool is_dumpability_changed(const struct cred *old, const struct cred *new)

This should be "static", I think?

+{

• if (!uid_eq(old->euid, new->euid) ||

•    !gid_eq(old->egid, new->egid) ||
  

•    !uid_eq(old->fsuid, new->fsuid) ||
  

•    !gid_eq(old->fsgid, new->fsgid) ||
  

•    !cred_cap_issubset(old, new))
  

• return true;
  

• return false;

+}

/**

• commit_creds - Install new credentials upon the current task
• @new: The credentials to be assigned

@@ -403,11 +427,7 @@ int commit_creds(struct cred *new) get_cred(new); /* we will require a ref for the subj creds too */ /* dumpability changes */

• if (!uid_eq(old->euid, new->euid) ||

•    !gid_eq(old->egid, new->egid) ||
  

•    !uid_eq(old->fsuid, new->fsuid) ||
  

•    !gid_eq(old->fsgid, new->fsgid) ||
  

•    !cred_cap_issubset(old, new)) {
  

• if (is_dumpability_changed(old, new)) { if (task->mm) set_dumpable(task->mm, suid_dumpable); task->pdeath_signal = 0;

diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 75a84efad40f..deacdf133f8b 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -20,6 +20,7 @@ #include <linux/pagemap.h> #include <linux/ptrace.h> #include <linux/security.h> +#include <linux/binfmts.h> #include <linux/signal.h> #include <linux/uio.h> #include <linux/audit.h> @@ -453,6 +454,27 @@ static int ptrace_attach(struct task_struct *task, long request, return retval; }

• if (unlikely(task->in_execve)) {
  

Urgh, we're trying to get rid of this bit too. https://lore.kernel.org/all/72da7003-a115-4162-b235-53cd3da8a90e@I-love.SAKU...

Can we find a better indicator?

• 	retval = down_write_killable(&task->signal->exec_update_lock);
  

• 	if (retval)
  

• 		return retval;
  

• 	scoped_guard (task_lock, task) {
  

• 		struct linux_binprm *bprm = task->signal->exec_bprm;
  

• 		const struct cred __rcu *old_cred = task->real_cred;
  

• 		struct mm_struct *old_mm = task->mm;
  

• 		rcu_assign_pointer(task->real_cred, bprm->cred);
  

• 		task->mm = bprm->mm;
  

• 		retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
  

• 		rcu_assign_pointer(task->real_cred, old_cred);
  

• 		task->mm = old_mm;
  

• 	}
  

• 	up_write(&task->signal->exec_update_lock);
  

• 	if (retval)
  

• 		return retval;
  

• }
  

• scoped_guard (write_lock_irq, &tasklist_lock) { if (unlikely(task->exit_state)) return -EPERM;

@@ -488,6 +510,14 @@ static int ptrace_traceme(void) { int ret = -EPERM;

• if (mutex_lock_interruptible(&current->signal->cred_guard_mutex))

• return -ERESTARTNOINTR;
  

• if (unlikely(current->signal->exec_bprm)) {

• mutex_unlock(&current->signal->cred_guard_mutex);
  

• return -ERESTARTNOINTR;
  

• }

I mention this hunk below...

• write_lock_irq(&tasklist_lock); /* Are we already being traced? */ if (!current->ptrace) {

@@ -503,6 +533,7 @@ static int ptrace_traceme(void) } } write_unlock_irq(&tasklist_lock);

• mutex_unlock(&current->signal->cred_guard_mutex);

return ret; } diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 41aa761c7738..d61fc275235a 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1994,9 +1994,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(&current->signal->cred_guard_mutex))
  

• goto out_put_fd;
  

• if (flags & SECCOMP_FILTER_FLAG_TSYNC) {

• if (mutex_lock_killable(&current->signal->cred_guard_mutex))
  

• 	goto out_put_fd;
  

• if (unlikely(current->signal->exec_bprm)) {
  

• 	mutex_unlock(&current->signal->cred_guard_mutex);
  

• 	goto out_put_fd;
  

• }
  

This updated test and the hunk noted above are _almost_ identical (interruptible vs killable). Could a helper with a descriptive name be used here instead? (And does the former hunk need interruptible, or could it use killable?) I'd just like to avoid having repeated dependent logic created ("we have to get the lock AND check for exec_bprm") when something better named than "lock_if_not_racing_exec(...)" could be used.

• }

spin_lock_irq(&current->sighand->siglock); diff --git a/tools/testing/selftests/ptrace/vmaccess.c b/tools/testing/selftests/ptrace/vmaccess.c index 4db327b44586..5d4a65eb5a8d 100644 --- a/tools/testing/selftests/ptrace/vmaccess.c +++ b/tools/testing/selftests/ptrace/vmaccess.c @@ -14,6 +14,7 @@ #include <signal.h> #include <unistd.h> #include <sys/ptrace.h> +#include <sys/syscall.h> static void *thread(void *arg) { @@ -23,7 +24,7 @@ static void *thread(void *arg) TEST(vmaccess) {

• int f, pid = fork();

• int s, f, pid = fork(); char mm[64];

if (!pid) { @@ -31,19 +32,42 @@ TEST(vmaccess) pthread_create(&pt, NULL, thread, NULL); pthread_join(pt, NULL);

• execlp("true", "true", NULL);
  

• execlp("false", "false", NULL);
  

• return;
  

  }

sleep(1); sprintf(mm, "/proc/%d/mem", pid);

• /* deadlock did happen here */ f = open(mm, O_RDONLY); ASSERT_GE(f, 0); close(f);

• f = kill(pid, SIGCONT);
• ASSERT_EQ(f, 0);

• f = waitpid(-1, &s, WNOHANG);
• ASSERT_NE(f, -1);
• ASSERT_NE(f, 0);
• ASSERT_NE(f, pid);
• ASSERT_EQ(WIFEXITED(s), 1);
• ASSERT_EQ(WEXITSTATUS(s), 0);
• f = waitpid(-1, &s, 0);
• ASSERT_EQ(f, pid);
• ASSERT_EQ(WIFEXITED(s), 1);
• ASSERT_EQ(WEXITSTATUS(s), 1);
• f = waitpid(-1, NULL, 0);
• ASSERT_EQ(f, -1);
• ASSERT_EQ(errno, ECHILD);

} -TEST(attach) +/*

• • Same test as previous, except that
• • we try to ptrace the group leader,
• • which is about to call execve,
• • when the other thread is already ptraced.
• • This exercises the code in de_thread
• • where it is waiting inside the
• • while (sig->notify_count) {
• • loop.
• */

+TEST(attach1) { int s, k, pid = fork(); @@ -52,19 +76,76 @@ TEST(attach) pthread_create(&pt, NULL, thread, NULL); pthread_join(pt, NULL);

• execlp("sleep", "sleep", "2", NULL);
  

• execlp("false", "false", NULL);
  

• return;
  

  }

sleep(1);

• /* deadlock may happen here */ 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);
• k = waitpid(-1, &s, 0);
• ASSERT_EQ(k, pid);
• ASSERT_EQ(WIFSTOPPED(s), 1);
• ASSERT_EQ(WSTOPSIG(s), SIGTRAP);
• k = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, 0);
• 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), SIGSTOP);
• k = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, 0);
• k = ptrace(PTRACE_CONT, pid, 0L, 0L);
• ASSERT_EQ(k, 0);
• k = waitpid(-1, &s, 0);
• ASSERT_EQ(k, pid);
• ASSERT_EQ(WIFEXITED(s), 1);
• ASSERT_EQ(WEXITSTATUS(s), 1);
• k = waitpid(-1, NULL, 0);
• ASSERT_EQ(k, -1);
• ASSERT_EQ(errno, ECHILD);

+}

+/*

• • Same test as previous, except that
• • the group leader is ptraced first,
• • but this time with PTRACE_O_TRACEEXIT,
• • and the thread that does execve is
• • not yet ptraced. This exercises the
• • code block in de_thread where the
• • if (!thread_group_leader(tsk)) {
• • is executed and enters a wait state.
• */

+static long thread2_tid; +static void *thread2(void *arg) +{

• thread2_tid = syscall(__NR_gettid);
• sleep(2);
• execlp("false", "false", NULL);
• return NULL;

+}

+TEST(attach2) +{

• int s, k, pid = fork();
• if (!pid) {

• pthread_t pt;
  

• pthread_create(&pt, NULL, thread2, NULL);
  

• pthread_join(pt, NULL);
  

• return;
  

• }
• sleep(1); k = ptrace(PTRACE_ATTACH, pid, 0L, 0L); ASSERT_EQ(k, 0);

@@ -72,12 +153,46 @@ TEST(attach) ASSERT_EQ(k, pid); ASSERT_EQ(WIFSTOPPED(s), 1); ASSERT_EQ(WSTOPSIG(s), SIGSTOP);

• k = ptrace(PTRACE_DETACH, pid, 0L, 0L);

• k = ptrace(PTRACE_SETOPTIONS, pid, 0L, PTRACE_O_TRACEEXIT);
• ASSERT_EQ(k, 0);
• thread2_tid = ptrace(PTRACE_PEEKDATA, pid, &thread2_tid, 0L);
• ASSERT_NE(thread2_tid, -1);
• ASSERT_NE(thread2_tid, 0);
• ASSERT_NE(thread2_tid, pid);
• k = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, 0);
• sleep(2);
• /* deadlock may happen here */
• k = ptrace(PTRACE_ATTACH, thread2_tid, 0L, 0L);
• ASSERT_EQ(k, 0);
• k = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, pid);
• ASSERT_EQ(WIFSTOPPED(s), 1);
• ASSERT_EQ(WSTOPSIG(s), SIGTRAP);
• k = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, 0);
• 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 = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, 0);
• 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), SIGSTOP);
• k = waitpid(-1, &s, WNOHANG);
• ASSERT_EQ(k, 0);
• k = ptrace(PTRACE_CONT, pid, 0L, 0L); ASSERT_EQ(k, 0); k = waitpid(-1, &s, 0); ASSERT_EQ(k, pid); ASSERT_EQ(WIFEXITED(s), 1);

• ASSERT_EQ(WEXITSTATUS(s), 0);

• ASSERT_EQ(WEXITSTATUS(s), 1); k = waitpid(-1, NULL, 0); ASSERT_EQ(k, -1); ASSERT_EQ(errno, ECHILD);

Thank you for adding tests! This will be a nice deadlock to get fixed.

-Kees

This introduces signal->exec_bprm, 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 problem happens when a tracer tries to ptrace_attach to a multi-threaded process, that does an execve in one of the threads at the same time, without doing that in a forked sub-process. That means: There is a race condition, when one or more of the threads are already ptraced, but the thread that invoked the execve is not yet traced. Now in this case the execve locks the cred_guard_mutex and waits for de_thread to complete. But that waits for the traced sibling threads to exit, and those have to wait for the tracer to receive the exit signal, but the tracer cannot call wait right now, because it is waiting for the ptrace call to complete, and this never does not happen. The traced process and the tracer are now in a deadlock situation, and can only be killed by a fatal signal.

The solution is to detect this situation and allow ptrace_attach to continue by temporarily releasing the cred_guard_mutex, while de_thread() is still waiting for traced zombies to be eventually released by the tracer. In the case of the thread group leader we only have to wait for the thread to become a zombie, which may also need co-operation from the tracer due to PTRACE_O_TRACEEXIT.

When a tracer wants to ptrace_attach a task that already is in execve, we simply retry the ptrace_may_access check while temporarily installing the new credentials and dumpability which are about to be used after execve completes. If the ptrace_attach happens on a thread that is a sibling-thread of the thread doing execve, it is sufficient to check against the old credentials, as this thread will be waited for, before the new credentials are installed.

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, the non-zero current->signal->exec_bprm 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 | 69 ++++++++--- fs/proc/base.c | 6 + include/linux/cred.h | 1 + include/linux/sched/signal.h | 18 +++ kernel/cred.c | 30 ++++- kernel/ptrace.c | 32 +++++ kernel/seccomp.c | 12 +- tools/testing/selftests/ptrace/vmaccess.c | 135 ++++++++++++++++++++-- 8 files changed, 266 insertions(+), 37 deletions(-)

v10: Changes to previous version, make the PTRACE_ATTACH return -EAGAIN, instead of execve return -ERESTARTSYS. Added some lessions learned to the description.

v11: Check old and new credentials in PTRACE_ATTACH again without changing the API.

Note: I got actually one response from an automatic checker to the v11 patch,

https://lore.kernel.org/lkml/202107121344.wu68hEPF-lkp@intel.com/

which is complaining about:

...

...

...

kernel/ptrace.c:425:26: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected struct cred const *old_cred @@ got struct cred const [noderef] __rcu *real_cred @@

417 struct linux_binprm *bprm = task->signal->exec_bprm; 418 const struct cred *old_cred; 419 struct mm_struct *old_mm; 420 421 retval = down_write_killable(&task->signal->exec_update_lock); 422 if (retval) 423 goto unlock_creds; 424 task_lock(task);

425 old_cred = task->real_cred;

v12: Essentially identical to v11.

- Fixed a minor merge conflict in linux v5.17, and fixed the above mentioned nit by adding __rcu to the declaration.

- re-tested the patch with all linux versions from v5.11 to v6.6

v10 was an alternative approach which did imply an API change. But I would prefer to avoid such an API change.

The difficult part is getting the right dumpability flags assigned before de_thread starts, hope you like this version. If not, the v10 is of course also acceptable.

v13: Fixed duplicated Return section in function header of is_dumpability_changed which was reported by the kernel test robot

v14: rebased to v6.7, refreshed and retested. And added a more detailed description of the actual bug.

v15: rebased to v6.8-rc1, addressed some review comments. Split the test case vmaccess into vmaccess1 and vmaccess2 to improve overall test coverage.

v16: rebased to 6.17-rc2, fixed some minor merge conflicts.

v17: avoid use of task->in_execve in ptrace_attach.

Thanks Bernd.

diff --git a/fs/exec.c b/fs/exec.c index 2a1e5e4042a1..31c6ceaa5f69 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -905,11 +905,13 @@ static int exec_mmap(struct mm_struct *mm) return 0; }

-static int de_thread(struct task_struct *tsk) +static int de_thread(struct task_struct *tsk, struct linux_binprm *bprm) { struct signal_struct *sig = tsk->signal; struct sighand_struct *oldsighand = tsk->sighand; spinlock_t *lock = &oldsighand->siglock; + struct task_struct *t; + bool unsafe_execve_in_progress = false;

if (thread_group_empty(tsk)) goto no_thread_group; @@ -932,6 +934,19 @@ static int de_thread(struct task_struct *tsk) if (!thread_group_leader(tsk)) sig->notify_count--;

+ for_other_threads(tsk, t) { + if (unlikely(t->ptrace) + && (t != tsk->group_leader || !t->exit_state)) + unsafe_execve_in_progress = true; + } + + if (unlikely(unsafe_execve_in_progress)) { + spin_unlock_irq(lock); + sig->exec_bprm = bprm; + mutex_unlock(&sig->cred_guard_mutex); + spin_lock_irq(lock); + } + while (sig->notify_count) { __set_current_state(TASK_KILLABLE); spin_unlock_irq(lock); @@ -1021,6 +1036,11 @@ static int de_thread(struct task_struct *tsk) release_task(leader); }

+ if (unlikely(unsafe_execve_in_progress)) { + mutex_lock(&sig->cred_guard_mutex); + sig->exec_bprm = NULL; + } + sig->group_exec_task = NULL; sig->notify_count = 0;

@@ -1032,6 +1052,11 @@ static int de_thread(struct task_struct *tsk) return 0;

killed: + if (unlikely(unsafe_execve_in_progress)) { + mutex_lock(&sig->cred_guard_mutex); + sig->exec_bprm = NULL; + } + /* protects against exit_notify() and __exit_signal() */ read_lock(&tasklist_lock); sig->group_exec_task = NULL; @@ -1114,13 +1139,31 @@ int begin_new_exec(struct linux_binprm * bprm) */ trace_sched_prepare_exec(current, bprm);

+ /* If the binary is not readable then enforce mm->dumpable=0 */ + would_dump(bprm, bprm->file); + if (bprm->have_execfd) + would_dump(bprm, bprm->executable); + + /* + * Figure out dumpability. Note that this checking only of current + * is wrong, but userspace depends on it. This should be testing + * bprm->secureexec instead. + */ + if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP || + is_dumpability_changed(current_cred(), bprm->cred) || + !(uid_eq(current_euid(), current_uid()) && + gid_eq(current_egid(), current_gid()))) + set_dumpable(bprm->mm, suid_dumpable); + else + set_dumpable(bprm->mm, SUID_DUMP_USER); + /* * Ensure all future errors are fatal. */ bprm->point_of_no_return = true;

/* Make this the only thread in the thread group */ - retval = de_thread(me); + retval = de_thread(me, bprm); if (retval) goto out; /* see the comment in check_unsafe_exec() */ @@ -1144,11 +1187,6 @@ int begin_new_exec(struct linux_binprm * bprm) if (retval) goto out;

- /* If the binary is not readable then enforce mm->dumpable=0 */ - would_dump(bprm, bprm->file); - if (bprm->have_execfd) - would_dump(bprm, bprm->executable); - /* * Release all of the old mmap stuff */ @@ -1210,18 +1248,6 @@ int begin_new_exec(struct linux_binprm * bprm)

me->sas_ss_sp = me->sas_ss_size = 0;

- /* - * Figure out dumpability. Note that this checking only of current - * is wrong, but userspace depends on it. This should be testing - * bprm->secureexec instead. - */ - if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP || - !(uid_eq(current_euid(), current_uid()) && - gid_eq(current_egid(), current_gid()))) - set_dumpable(current->mm, suid_dumpable); - else - set_dumpable(current->mm, SUID_DUMP_USER); - perf_event_exec();

/* @@ -1361,6 +1387,11 @@ static int prepare_bprm_creds(struct linux_binprm *bprm) if (mutex_lock_interruptible(&current->signal->cred_guard_mutex)) return -ERESTARTNOINTR;

+ if (unlikely(current->signal->exec_bprm)) { + mutex_unlock(&current->signal->cred_guard_mutex); + return -ERESTARTNOINTR; + } + bprm->cred = prepare_exec_creds(); if (likely(bprm->cred)) return 0; diff --git a/fs/proc/base.c b/fs/proc/base.c index 62d35631ba8c..e5bcf812cee0 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2838,6 +2838,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->exec_bprm)) { + mutex_unlock(&current->signal->cred_guard_mutex); + rv = -ERESTARTNOINTR; + goto out_free; + } + rv = security_setprocattr(PROC_I(inode)->op.lsmid, file->f_path.dentry->d_name.name, page, count); diff --git a/include/linux/cred.h b/include/linux/cred.h index a102a10f833f..fb0361911489 100644 --- a/include/linux/cred.h +++ b/include/linux/cred.h @@ -153,6 +153,7 @@ extern const struct cred *get_task_cred(struct task_struct *); extern struct cred *cred_alloc_blank(void); extern struct cred *prepare_creds(void); extern struct cred *prepare_exec_creds(void); +extern bool is_dumpability_changed(const struct cred *, const struct cred *); extern int commit_creds(struct cred *); extern void abort_creds(struct cred *); extern struct cred *prepare_kernel_cred(struct task_struct *); diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 1ef1edbaaf79..3c47d8b55863 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -237,9 +237,27 @@ struct signal_struct { struct mm_struct *oom_mm; /* recorded mm when the thread group got * killed by the oom killer */

+ struct linux_binprm *exec_bprm; /* Used to check ptrace_may_access + * against new credentials while + * de_thread is waiting for other + * traced threads to terminate. + * Set while de_thread is executing. + * 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->exec_bprm happens + * to be non-zero after the + * cred_guard_mutex was acquired. + */ + 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/cred.c b/kernel/cred.c index 9676965c0981..0b2822c762df 100644 --- a/kernel/cred.c +++ b/kernel/cred.c @@ -375,6 +375,30 @@ static bool cred_cap_issubset(const struct cred *set, const struct cred *subset) return false; }

+/** + * is_dumpability_changed - Will changing creds affect dumpability? + * @old: The old credentials. + * @new: The new credentials. + * + * If the @new credentials have no elevated privileges compared to the + * @old credentials, the task may remain dumpable. Otherwise we have + * to mark the task as undumpable to avoid information leaks from higher + * to lower privilege domains. + * + * Return: True if the task will become undumpable. + */ +bool is_dumpability_changed(const struct cred *old, const struct cred *new) +{ + if (!uid_eq(old->euid, new->euid) || + !gid_eq(old->egid, new->egid) || + !uid_eq(old->fsuid, new->fsuid) || + !gid_eq(old->fsgid, new->fsgid) || + !cred_cap_issubset(old, new)) + return true; + + return false; +} + /** * commit_creds - Install new credentials upon the current task * @new: The credentials to be assigned @@ -403,11 +427,7 @@ int commit_creds(struct cred *new) get_cred(new); /* we will require a ref for the subj creds too */

/* dumpability changes */ - if (!uid_eq(old->euid, new->euid) || - !gid_eq(old->egid, new->egid) || - !uid_eq(old->fsuid, new->fsuid) || - !gid_eq(old->fsgid, new->fsgid) || - !cred_cap_issubset(old, new)) { + if (is_dumpability_changed(old, new)) { if (task->mm) set_dumpable(task->mm, suid_dumpable); task->pdeath_signal = 0; diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 75a84efad40f..230298817dbf 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -20,6 +20,7 @@ #include <linux/pagemap.h> #include <linux/ptrace.h> #include <linux/security.h> +#include <linux/binfmts.h> #include <linux/signal.h> #include <linux/uio.h> #include <linux/audit.h> @@ -453,6 +454,28 @@ static int ptrace_attach(struct task_struct *task, long request, return retval; }

+ if (unlikely(task == task->signal->group_exec_task)) { + retval = down_write_killable(&task->signal->exec_update_lock); + if (retval) + return retval; + + scoped_guard (task_lock, task) { + struct linux_binprm *bprm = task->signal->exec_bprm; + const struct cred __rcu *old_cred = task->real_cred; + struct mm_struct *old_mm = task->mm; + + rcu_assign_pointer(task->real_cred, bprm->cred); + task->mm = bprm->mm; + retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS); + rcu_assign_pointer(task->real_cred, old_cred); + task->mm = old_mm; + } + + up_write(&task->signal->exec_update_lock); + if (retval) + return retval; + } + scoped_guard (write_lock_irq, &tasklist_lock) { if (unlikely(task->exit_state)) return -EPERM; @@ -488,6 +511,14 @@ static int ptrace_traceme(void) { int ret = -EPERM;

+ if (mutex_lock_interruptible(&current->signal->cred_guard_mutex)) + return -ERESTARTNOINTR; + + if (unlikely(current->signal->exec_bprm)) { + mutex_unlock(&current->signal->cred_guard_mutex); + return -ERESTARTNOINTR; + } + write_lock_irq(&tasklist_lock); /* Are we already being traced? */ if (!current->ptrace) { @@ -503,6 +534,7 @@ static int ptrace_traceme(void) } } write_unlock_irq(&tasklist_lock); + mutex_unlock(&current->signal->cred_guard_mutex);

return ret; } diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 41aa761c7738..d61fc275235a 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1994,9 +1994,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(&current->signal->cred_guard_mutex)) - goto out_put_fd; + if (flags & SECCOMP_FILTER_FLAG_TSYNC) { + if (mutex_lock_killable(&current->signal->cred_guard_mutex)) + goto out_put_fd; + + if (unlikely(current->signal->exec_bprm)) { + mutex_unlock(&current->signal->cred_guard_mutex); + goto out_put_fd; + } + }

spin_lock_irq(&current->sighand->siglock);

diff --git a/tools/testing/selftests/ptrace/vmaccess.c b/tools/testing/selftests/ptrace/vmaccess.c index 4db327b44586..5d4a65eb5a8d 100644 --- a/tools/testing/selftests/ptrace/vmaccess.c +++ b/tools/testing/selftests/ptrace/vmaccess.c @@ -14,6 +14,7 @@ #include <signal.h> #include <unistd.h> #include <sys/ptrace.h> +#include <sys/syscall.h>

static void *thread(void *arg) { @@ -23,7 +24,7 @@ static void *thread(void *arg)

TEST(vmaccess) { - int f, pid = fork(); + int s, f, pid = fork(); char mm[64];

if (!pid) { @@ -31,19 +32,42 @@ TEST(vmaccess)

pthread_create(&pt, NULL, thread, NULL); pthread_join(pt, NULL); - execlp("true", "true", NULL); + execlp("false", "false", NULL); + return; }

sleep(1); sprintf(mm, "/proc/%d/mem", pid); + /* deadlock did happen here */ f = open(mm, O_RDONLY); ASSERT_GE(f, 0); close(f); - f = kill(pid, SIGCONT); - ASSERT_EQ(f, 0); + f = waitpid(-1, &s, WNOHANG); + ASSERT_NE(f, -1); + ASSERT_NE(f, 0); + ASSERT_NE(f, pid); + ASSERT_EQ(WIFEXITED(s), 1); + ASSERT_EQ(WEXITSTATUS(s), 0); + f = waitpid(-1, &s, 0); + ASSERT_EQ(f, pid); + ASSERT_EQ(WIFEXITED(s), 1); + ASSERT_EQ(WEXITSTATUS(s), 1); + f = waitpid(-1, NULL, 0); + ASSERT_EQ(f, -1); + ASSERT_EQ(errno, ECHILD); }

-TEST(attach) +/* + * Same test as previous, except that + * we try to ptrace the group leader, + * which is about to call execve, + * when the other thread is already ptraced. + * This exercises the code in de_thread + * where it is waiting inside the + * while (sig->notify_count) { + * loop. + */ +TEST(attach1) { int s, k, pid = fork();

@@ -52,19 +76,76 @@ TEST(attach)

pthread_create(&pt, NULL, thread, NULL); pthread_join(pt, NULL); - execlp("sleep", "sleep", "2", NULL); + execlp("false", "false", NULL); + return; }

sleep(1); + /* deadlock may happen here */ 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); + k = waitpid(-1, &s, 0); + ASSERT_EQ(k, pid); + ASSERT_EQ(WIFSTOPPED(s), 1); + ASSERT_EQ(WSTOPSIG(s), SIGTRAP); + k = waitpid(-1, &s, WNOHANG); + ASSERT_EQ(k, 0); + 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), SIGSTOP); + k = waitpid(-1, &s, WNOHANG); + ASSERT_EQ(k, 0); + k = ptrace(PTRACE_CONT, pid, 0L, 0L); + ASSERT_EQ(k, 0); + k = waitpid(-1, &s, 0); + ASSERT_EQ(k, pid); + ASSERT_EQ(WIFEXITED(s), 1); + ASSERT_EQ(WEXITSTATUS(s), 1); + k = waitpid(-1, NULL, 0); + ASSERT_EQ(k, -1); + ASSERT_EQ(errno, ECHILD); +} + +/* + * Same test as previous, except that + * the group leader is ptraced first, + * but this time with PTRACE_O_TRACEEXIT, + * and the thread that does execve is + * not yet ptraced. This exercises the + * code block in de_thread where the + * if (!thread_group_leader(tsk)) { + * is executed and enters a wait state. + */ +static long thread2_tid; +static void *thread2(void *arg) +{ + thread2_tid = syscall(__NR_gettid); + sleep(2); + execlp("false", "false", NULL); + return NULL; +} + +TEST(attach2) +{ + int s, k, pid = fork(); + + if (!pid) { + pthread_t pt; + + pthread_create(&pt, NULL, thread2, NULL); + pthread_join(pt, NULL); + return; + } + sleep(1); k = ptrace(PTRACE_ATTACH, pid, 0L, 0L); ASSERT_EQ(k, 0); @@ -72,12 +153,46 @@ TEST(attach) ASSERT_EQ(k, pid); ASSERT_EQ(WIFSTOPPED(s), 1); ASSERT_EQ(WSTOPSIG(s), SIGSTOP); - k = ptrace(PTRACE_DETACH, pid, 0L, 0L); + k = ptrace(PTRACE_SETOPTIONS, pid, 0L, PTRACE_O_TRACEEXIT); + ASSERT_EQ(k, 0); + thread2_tid = ptrace(PTRACE_PEEKDATA, pid, &thread2_tid, 0L); + ASSERT_NE(thread2_tid, -1); + ASSERT_NE(thread2_tid, 0); + ASSERT_NE(thread2_tid, pid); + k = waitpid(-1, &s, WNOHANG); + ASSERT_EQ(k, 0); + sleep(2); + /* deadlock may happen here */ + k = ptrace(PTRACE_ATTACH, thread2_tid, 0L, 0L); + ASSERT_EQ(k, 0); + k = waitpid(-1, &s, WNOHANG); + ASSERT_EQ(k, pid); + ASSERT_EQ(WIFSTOPPED(s), 1); + ASSERT_EQ(WSTOPSIG(s), SIGTRAP); + k = waitpid(-1, &s, WNOHANG); + ASSERT_EQ(k, 0); + 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 = waitpid(-1, &s, WNOHANG); + ASSERT_EQ(k, 0); + 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), SIGSTOP); + k = waitpid(-1, &s, WNOHANG); + ASSERT_EQ(k, 0); + k = ptrace(PTRACE_CONT, pid, 0L, 0L); ASSERT_EQ(k, 0); k = waitpid(-1, &s, 0); ASSERT_EQ(k, pid); ASSERT_EQ(WIFEXITED(s), 1); - ASSERT_EQ(WEXITSTATUS(s), 0); + ASSERT_EQ(WEXITSTATUS(s), 1); k = waitpid(-1, NULL, 0); ASSERT_EQ(k, -1); ASSERT_EQ(errno, ECHILD);

On Thu, Aug 21, 2025 at 07:34:58PM +0200, Bernd Edlinger wrote:

The solution is to detect this situation and allow ptrace_attach to continue by temporarily releasing the cred_guard_mutex, while de_thread() is still waiting for traced zombies to be eventually released by the tracer. In the case of the thread group leader we only have to wait for the thread to become a zombie, which may also need co-operation from the tracer due to PTRACE_O_TRACEEXIT.

When a tracer wants to ptrace_attach a task that already is in execve, we simply retry the ptrace_may_access check while temporarily installing the new credentials and dumpability which are about to be used after execve completes. If the ptrace_attach happens on a thread that is a sibling-thread of the thread doing execve, it is sufficient to check against the old credentials, as this thread will be waited for, before the new credentials are installed.

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, the non-zero current->signal->exec_bprm makes sure they release the mutex immediately and return with -ERESTARTNOINTR.

diff --git a/fs/exec.c b/fs/exec.c index 2a1e5e4042a1..31c6ceaa5f69 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -905,11 +905,13 @@ static int exec_mmap(struct mm_struct *mm) return 0; } -static int de_thread(struct task_struct *tsk) +static int de_thread(struct task_struct *tsk, struct linux_binprm *bprm) { struct signal_struct *sig = tsk->signal; struct sighand_struct *oldsighand = tsk->sighand; spinlock_t *lock = &oldsighand->siglock;

• struct task_struct *t;
• bool unsafe_execve_in_progress = false;

if (thread_group_empty(tsk)) goto no_thread_group; @@ -932,6 +934,19 @@ static int de_thread(struct task_struct *tsk) if (!thread_group_leader(tsk)) sig->notify_count--;

• for_other_threads(tsk, t) {

• if (unlikely(t->ptrace)
  

•     && (t != tsk->group_leader || !t->exit_state))
  

&& goes at the end of the previous line

• 	unsafe_execve_in_progress = true;
  

• }
• if (unlikely(unsafe_execve_in_progress)) {

• spin_unlock_irq(lock);
  

• sig->exec_bprm = bprm;
  

• mutex_unlock(&sig->cred_guard_mutex);
  

• spin_lock_irq(lock);
  

I'm not clear why we need to drop and re-acquire siglock here.

And I would like a very large comment here explaining why it is safe to drop cred_guard_mutex here.

• }
• while (sig->notify_count) { __set_current_state(TASK_KILLABLE); spin_unlock_irq(lock);

@@ -1021,6 +1036,11 @@ static int de_thread(struct task_struct *tsk) release_task(leader); }

• if (unlikely(unsafe_execve_in_progress)) {

• mutex_lock(&sig->cred_guard_mutex);
  

• sig->exec_bprm = NULL;
  

• }
• sig->group_exec_task = NULL; sig->notify_count = 0;

@@ -1032,6 +1052,11 @@ static int de_thread(struct task_struct *tsk) return 0; killed:

• if (unlikely(unsafe_execve_in_progress)) {

• mutex_lock(&sig->cred_guard_mutex);
  

• sig->exec_bprm = NULL;
  

• }
• /* protects against exit_notify() and __exit_signal() */ read_lock(&tasklist_lock); sig->group_exec_task = NULL;

@@ -1114,13 +1139,31 @@ int begin_new_exec(struct linux_binprm * bprm) */ trace_sched_prepare_exec(current, bprm);

• /* If the binary is not readable then enforce mm->dumpable=0 */
• would_dump(bprm, bprm->file);
• if (bprm->have_execfd)

• would_dump(bprm, bprm->executable);
  

• /*

• * Figure out dumpability. Note that this checking only of current
  

• * is wrong, but userspace depends on it. This should be testing
  

• * bprm->secureexec instead.
  

• */
  

• if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||

•    is_dumpability_changed(current_cred(), bprm->cred) ||
  

•    !(uid_eq(current_euid(), current_uid()) &&
  

•      gid_eq(current_egid(), current_gid())))
  

• set_dumpable(bprm->mm, suid_dumpable);
  

• else

• set_dumpable(bprm->mm, SUID_DUMP_USER);
  

I feel like moving this dumpable stuff around could be a separate patch. Which can explain how that is correct and why it is needed and all that.

/* * Ensure all future errors are fatal. */ bprm->point_of_no_return = true; /* Make this the only thread in the thread group */

• retval = de_thread(me);

• retval = de_thread(me, bprm); if (retval) goto out; /* see the comment in check_unsafe_exec() */

@@ -1144,11 +1187,6 @@ int begin_new_exec(struct linux_binprm * bprm) if (retval) goto out;

• /* If the binary is not readable then enforce mm->dumpable=0 */
• would_dump(bprm, bprm->file);
• if (bprm->have_execfd)

• would_dump(bprm, bprm->executable);
  

• /*
  • Release all of the old mmap stuff
  */

@@ -1210,18 +1248,6 @@ int begin_new_exec(struct linux_binprm * bprm) me->sas_ss_sp = me->sas_ss_size = 0;

• /*

• * Figure out dumpability. Note that this checking only of current
  

• * is wrong, but userspace depends on it. This should be testing
  

• * bprm->secureexec instead.
  

• */
  

• if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||

•    !(uid_eq(current_euid(), current_uid()) &&
  

•      gid_eq(current_egid(), current_gid())))
  

• set_dumpable(current->mm, suid_dumpable);
  

• else

• set_dumpable(current->mm, SUID_DUMP_USER);
  

• perf_event_exec();

/* @@ -1361,6 +1387,11 @@ static int prepare_bprm_creds(struct linux_binprm *bprm) if (mutex_lock_interruptible(&current->signal->cred_guard_mutex)) return -ERESTARTNOINTR;

• if (unlikely(current->signal->exec_bprm)) {

• mutex_unlock(&current->signal->cred_guard_mutex);
  

• return -ERESTARTNOINTR;
  

• }

#1

• bprm->cred = prepare_exec_creds(); if (likely(bprm->cred)) return 0;

diff --git a/fs/proc/base.c b/fs/proc/base.c index 62d35631ba8c..e5bcf812cee0 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2838,6 +2838,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, if (rv < 0) goto out_free;

Comment explaining why this needs checking goes here.

• if (unlikely(current->signal->exec_bprm)) {

• mutex_unlock(&current->signal->cred_guard_mutex);
  

• rv = -ERESTARTNOINTR;
  

• goto out_free;
  

• }
• rv = security_setprocattr(PROC_I(inode)->op.lsmid, file->f_path.dentry->d_name.name, page, count);

diff --git a/include/linux/cred.h b/include/linux/cred.h index a102a10f833f..fb0361911489 100644 --- a/include/linux/cred.h +++ b/include/linux/cred.h @@ -153,6 +153,7 @@ extern const struct cred *get_task_cred(struct task_struct *); extern struct cred *cred_alloc_blank(void); extern struct cred *prepare_creds(void); extern struct cred *prepare_exec_creds(void); +extern bool is_dumpability_changed(const struct cred *, const struct cred *); extern int commit_creds(struct cred *); extern void abort_creds(struct cred *); extern struct cred *prepare_kernel_cred(struct task_struct *); diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 1ef1edbaaf79..3c47d8b55863 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -237,9 +237,27 @@ struct signal_struct { struct mm_struct *oom_mm; /* recorded mm when the thread group got * killed by the oom killer */

• struct linux_binprm *exec_bprm; /* Used to check ptrace_may_access

• 			 * against new credentials while
  

• 			 * de_thread is waiting for other
  

• 			 * traced threads to terminate.
  

• 			 * Set while de_thread is executing.
  

• 			 * 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->exec_bprm happens
  

• 			 * to be non-zero after the
  

• 			 * cred_guard_mutex was acquired.
  

• 			 */
  

• 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/cred.c b/kernel/cred.c index 9676965c0981..0b2822c762df 100644 --- a/kernel/cred.c +++ b/kernel/cred.c @@ -375,6 +375,30 @@ static bool cred_cap_issubset(const struct cred *set, const struct cred *subset) return false; } +/**

• • is_dumpability_changed - Will changing creds affect dumpability?
• • @old: The old credentials.
• • @new: The new credentials.
• • If the @new credentials have no elevated privileges compared to the
• • @old credentials, the task may remain dumpable. Otherwise we have
• • to mark the task as undumpable to avoid information leaks from higher
• • to lower privilege domains.
• • Return: True if the task will become undumpable.
• */

+bool is_dumpability_changed(const struct cred *old, const struct cred *new) +{

• if (!uid_eq(old->euid, new->euid) ||

•    !gid_eq(old->egid, new->egid) ||
  

•    !uid_eq(old->fsuid, new->fsuid) ||
  

•    !gid_eq(old->fsgid, new->fsgid) ||
  

•    !cred_cap_issubset(old, new))
  

• return true;
  

• return false;

+}

/**

• commit_creds - Install new credentials upon the current task
• @new: The credentials to be assigned

@@ -403,11 +427,7 @@ int commit_creds(struct cred *new) get_cred(new); /* we will require a ref for the subj creds too */ /* dumpability changes */

• if (!uid_eq(old->euid, new->euid) ||

•    !gid_eq(old->egid, new->egid) ||
  

•    !uid_eq(old->fsuid, new->fsuid) ||
  

•    !gid_eq(old->fsgid, new->fsgid) ||
  

•    !cred_cap_issubset(old, new)) {
  

• if (is_dumpability_changed(old, new)) { if (task->mm) set_dumpable(task->mm, suid_dumpable); task->pdeath_signal = 0;

diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 75a84efad40f..230298817dbf 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -20,6 +20,7 @@ #include <linux/pagemap.h> #include <linux/ptrace.h> #include <linux/security.h> +#include <linux/binfmts.h> #include <linux/signal.h> #include <linux/uio.h> #include <linux/audit.h> @@ -453,6 +454,28 @@ static int ptrace_attach(struct task_struct *task, long request, return retval; }

• if (unlikely(task == task->signal->group_exec_task)) {
  

• 	retval = down_write_killable(&task->signal->exec_update_lock);
  

• 	if (retval)
  

• 		return retval;
  

This could be written like:

ACQUIRE(rwsem_write_kill, guard)(&task->signal->exec_update_lock); retval = ACQUIRE_ERR(rwsem_write_kill, guard); if (retval) return retval;

• 	scoped_guard (task_lock, task) {
  

• 		struct linux_binprm *bprm = task->signal->exec_bprm;
  

• 		const struct cred __rcu *old_cred = task->real_cred;
  

• 		struct mm_struct *old_mm = task->mm;
  

• 		rcu_assign_pointer(task->real_cred, bprm->cred);
  

• 		task->mm = bprm->mm;
  

• 		retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
  

• 		rcu_assign_pointer(task->real_cred, old_cred);
  

• 		task->mm = old_mm;
  

• 	}
  

• 	up_write(&task->signal->exec_update_lock);
  

And then this goes away ^

• 	if (retval)
  

• 		return retval;
  

• }
  

• scoped_guard (write_lock_irq, &tasklist_lock) { if (unlikely(task->exit_state)) return -EPERM;

@@ -488,6 +511,14 @@ static int ptrace_traceme(void) { int ret = -EPERM;

This needs comments.

• if (mutex_lock_interruptible(&current->signal->cred_guard_mutex))

• return -ERESTARTNOINTR;
  

• if (unlikely(current->signal->exec_bprm)) {

• mutex_unlock(&current->signal->cred_guard_mutex);
  

• return -ERESTARTNOINTR;
  

• }

#2

• write_lock_irq(&tasklist_lock); /* Are we already being traced? */ if (!current->ptrace) {

@@ -503,6 +534,7 @@ static int ptrace_traceme(void) } } write_unlock_irq(&tasklist_lock);

• mutex_unlock(&current->signal->cred_guard_mutex);

return ret; } diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 41aa761c7738..d61fc275235a 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1994,9 +1994,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(&current->signal->cred_guard_mutex))
  

• goto out_put_fd;
  

• if (flags & SECCOMP_FILTER_FLAG_TSYNC) {

• if (mutex_lock_killable(&current->signal->cred_guard_mutex))
  

• 	goto out_put_fd;
  

• if (unlikely(current->signal->exec_bprm)) {
  

• 	mutex_unlock(&current->signal->cred_guard_mutex);
  

• 	goto out_put_fd;
  

• }
  

#3, and after typing this same pattern 3 times, you didn't think it needed a helper function ?

• }

spin_lock_irq(&current->sighand->siglock);

I am still thinking about another approach, will write another email. But let me take a closer look at your patch.

First of all, can you split it? See below.

On 08/21, Bernd Edlinger wrote:

-static int de_thread(struct task_struct *tsk) +static int de_thread(struct task_struct *tsk, struct linux_binprm *bprm) { struct signal_struct *sig = tsk->signal; struct sighand_struct *oldsighand = tsk->sighand; spinlock_t *lock = &oldsighand->siglock;

• struct task_struct *t;

• bool unsafe_execve_in_progress = false;

  if (thread_group_empty(tsk)) goto no_thread_group;

@@ -932,6 +934,19 @@ static int de_thread(struct task_struct *tsk) if (!thread_group_leader(tsk)) sig->notify_count--;

• for_other_threads(tsk, t) {

• if (unlikely(t->ptrace)
  

•     && (t != tsk->group_leader || !t->exit_state))
  

• 	unsafe_execve_in_progress = true;
  

you can add "break" into the "if ()" block...

But this is minor. Why do we need "bool unsafe_execve_in_progress" ? If this patch is correct, de_thread() can drop/reacquire cred_guard_mutex unconditionally.

If you really think it makes sense, please make another patch with the changelog.

I'd certainly prefer to avoid this boolean at least for the start. If nothing else to catch the potential problems earlier.

• if (unlikely(unsafe_execve_in_progress)) {

• spin_unlock_irq(lock);
  

• sig->exec_bprm = bprm;
  

• mutex_unlock(&sig->cred_guard_mutex);
  

• spin_lock_irq(lock);
  

I don't think spin_unlock_irq() + spin_lock_irq() makes any sense...

@@ -1114,13 +1139,31 @@ int begin_new_exec(struct linux_binprm * bprm) */ trace_sched_prepare_exec(current, bprm);

• /* If the binary is not readable then enforce mm->dumpable=0 */
• would_dump(bprm, bprm->file);
• if (bprm->have_execfd)

• would_dump(bprm, bprm->executable);
  

• /*

• * Figure out dumpability. Note that this checking only of current
  

• * is wrong, but userspace depends on it. This should be testing
  

• * bprm->secureexec instead.
  

• */
  

• if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||

•    is_dumpability_changed(current_cred(), bprm->cred) ||
  

•    !(uid_eq(current_euid(), current_uid()) &&
  

•      gid_eq(current_egid(), current_gid())))
  

• set_dumpable(bprm->mm, suid_dumpable);
  

• else

• set_dumpable(bprm->mm, SUID_DUMP_USER);
  

OK, we need to do this before de_thread() drops cred_guard_mutex. But imo this too should be done in a separate patch, the changelog should explain this change.

@@ -1361,6 +1387,11 @@ static int prepare_bprm_creds(struct linux_binprm *bprm) if (mutex_lock_interruptible(&current->signal->cred_guard_mutex)) return -ERESTARTNOINTR;

• if (unlikely(current->signal->exec_bprm)) {

• mutex_unlock(&current->signal->cred_guard_mutex);
  

• return -ERESTARTNOINTR;
  

• }

OK, if signal->exec_bprm != NULL, then current is already killed. But proc_pid_attr_write() and ptrace_traceme() do the same. So how about something like

int lock_current_cgm(void) { if (mutex_lock_interruptible(&current->signal->cred_guard_mutex)) return -ERESTARTNOINTR;

if (!current->signal->group_exec_task) return 0;

WARN_ON(!fatal_signal_pending(current)); mutex_unlock(&current->signal->cred_guard_mutex); return -ERESTARTNOINTR; }

?

Note that it checks ->group_exec_task, not ->exec_bprm. So this change can come in a separate patch too, but I won't insist.

@@ -453,6 +454,28 @@ static int ptrace_attach(struct task_struct *task, long request, return retval; }

• if (unlikely(task == task->signal->group_exec_task)) {
  

• 	retval = down_write_killable(&task->signal->exec_update_lock);
  

• 	if (retval)
  

• 		return retval;
  

• 	scoped_guard (task_lock, task) {
  

• 		struct linux_binprm *bprm = task->signal->exec_bprm;
  

• 		const struct cred __rcu *old_cred = task->real_cred;
  

• 		struct mm_struct *old_mm = task->mm;
  

• 		rcu_assign_pointer(task->real_cred, bprm->cred);
  

• 		task->mm = bprm->mm;
  

• 		retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
  

• 		rcu_assign_pointer(task->real_cred, old_cred);
  

• 		task->mm = old_mm;
  

• 	}
  

This is the most problematic change which I can't review...

Firstly, it changes task->mm/real_cred for __ptrace_may_access() and this looks dangerous to me.

Say, current_is_single_threaded() called by another CLONE_VM process can miss group_exec_task and falsely return true. Probably not that bad, in this case old_mm should go away soon, but still...

And I don't know if this can fool the users of task_cred_xxx/__task_cred somehow.

Or. check_unsafe_exec() sets LSM_UNSAFE_PTRACE if ptrace. Is it safe to ptrace the execing task after that? I have no idea what the security hooks can do...

Again, can't review this part.

Oleg.

On 11/11/25 10:21, Christian Brauner wrote:

On Wed, Nov 05, 2025 at 03:32:10PM +0100, Oleg Nesterov wrote:

...

I am still thinking about another approach, will write another email. But let me take a closer look at your patch.

First of all, can you split it? See below.

On 08/21, Bernd Edlinger wrote:

...

-static int de_thread(struct task_struct *tsk) +static int de_thread(struct task_struct *tsk, struct linux_binprm *bprm) { struct signal_struct *sig = tsk->signal; struct sighand_struct *oldsighand = tsk->sighand; spinlock_t *lock = &oldsighand->siglock;

• struct task_struct *t;

• bool unsafe_execve_in_progress = false;

  if (thread_group_empty(tsk)) goto no_thread_group;

@@ -932,6 +934,19 @@ static int de_thread(struct task_struct *tsk) if (!thread_group_leader(tsk)) sig->notify_count--;

• for_other_threads(tsk, t) {

• if (unlikely(t->ptrace)
  

•     && (t != tsk->group_leader || !t->exit_state))
  

• 	unsafe_execve_in_progress = true;
  

you can add "break" into the "if ()" block...

But this is minor. Why do we need "bool unsafe_execve_in_progress" ? If this patch is correct, de_thread() can drop/reacquire cred_guard_mutex unconditionally.

If you really think it makes sense, please make another patch with the changelog.

I'd certainly prefer to avoid this boolean at least for the start. If nothing else to catch the potential problems earlier.

...

• if (unlikely(unsafe_execve_in_progress)) {

• spin_unlock_irq(lock);
  

• sig->exec_bprm = bprm;
  

• mutex_unlock(&sig->cred_guard_mutex);
  

• spin_lock_irq(lock);
  

I don't think spin_unlock_irq() + spin_lock_irq() makes any sense...

...

@@ -1114,13 +1139,31 @@ int begin_new_exec(struct linux_binprm * bprm) */ trace_sched_prepare_exec(current, bprm);

• /* If the binary is not readable then enforce mm->dumpable=0 */
• would_dump(bprm, bprm->file);
• if (bprm->have_execfd)

• would_dump(bprm, bprm->executable);
  

• /*

• * Figure out dumpability. Note that this checking only of current
  

• * is wrong, but userspace depends on it. This should be testing
  

• * bprm->secureexec instead.
  

• */
  

• if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||

•    is_dumpability_changed(current_cred(), bprm->cred) ||
  

•    !(uid_eq(current_euid(), current_uid()) &&
  

•      gid_eq(current_egid(), current_gid())))
  

• set_dumpable(bprm->mm, suid_dumpable);
  

• else

• set_dumpable(bprm->mm, SUID_DUMP_USER);
  

OK, we need to do this before de_thread() drops cred_guard_mutex. But imo this too should be done in a separate patch, the changelog should explain this change.

...

@@ -1361,6 +1387,11 @@ static int prepare_bprm_creds(struct linux_binprm *bprm) if (mutex_lock_interruptible(&current->signal->cred_guard_mutex)) return -ERESTARTNOINTR;

• if (unlikely(current->signal->exec_bprm)) {

• mutex_unlock(&current->signal->cred_guard_mutex);
  

• return -ERESTARTNOINTR;
  

• }

OK, if signal->exec_bprm != NULL, then current is already killed. But proc_pid_attr_write() and ptrace_traceme() do the same. So how about something like

int lock_current_cgm(void) { if (mutex_lock_interruptible(&current->signal->cred_guard_mutex)) return -ERESTARTNOINTR;

if (!current->signal->group_exec_task)
	return 0;

WARN_ON(!fatal_signal_pending(current));
mutex_unlock(&current->signal->cred_guard_mutex);
return -ERESTARTNOINTR;

}

?

Note that it checks ->group_exec_task, not ->exec_bprm. So this change can come in a separate patch too, but I won't insist.

...

@@ -453,6 +454,28 @@ static int ptrace_attach(struct task_struct *task, long request, return retval; }

• if (unlikely(task == task->signal->group_exec_task)) {
  

• 	retval = down_write_killable(&task->signal->exec_update_lock);
  

• 	if (retval)
  

• 		return retval;
  

• 	scoped_guard (task_lock, task) {
  

• 		struct linux_binprm *bprm = task->signal->exec_bprm;
  

• 		const struct cred __rcu *old_cred = task->real_cred;
  

• 		struct mm_struct *old_mm = task->mm;
  

• 		rcu_assign_pointer(task->real_cred, bprm->cred);
  

• 		task->mm = bprm->mm;
  

• 		retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
  

• 		rcu_assign_pointer(task->real_cred, old_cred);
  

• 		task->mm = old_mm;
  

• 	}
  

This is the most problematic change which I can't review...

Firstly, it changes task->mm/real_cred for __ptrace_may_access() and this looks dangerous to me.

Yeah, that is not ok. This is effectively override_creds for real_cred and that is not a pattern I want to see us establish at all! Temporary credential overrides for the subjective credentials is already terrible but at least we have the explicit split between real_cred and cred expressely for that. So no, that's not an acceptable solution.

Well when this is absolutely not acceptable then I would have to change all security engines to be aware of the current and the new credentials. That may be as well be possible but would be a rather big change. Of course that was only meant as a big exception, and somehow safe as long as it is protected under the right mutexes: cred_guard_mutex, exec_update_lock and task_lock at least.

...

Say, current_is_single_threaded() called by another CLONE_VM process can miss group_exec_task and falsely return true. Probably not that bad, in this case old_mm should go away soon, but still...

And I don't know if this can fool the users of task_cred_xxx/__task_cred somehow.

Or. check_unsafe_exec() sets LSM_UNSAFE_PTRACE if ptrace. Is it safe to ptrace the execing task after that? I have no idea what the security hooks can do...

Again, can't review this part.

Never mind, your review was really helpful. At the very least it pointed out some places where better comments are needed.

Thanks Bernd.

...

Oleg.

Preparation. After the next change setup_new_exec() can fail.

Signed-off-by: Oleg Nesterov oleg@redhat.com --- fs/binfmt_elf.c | 4 +++- fs/binfmt_elf_fdpic.c | 4 +++- fs/binfmt_flat.c | 4 +++- fs/exec.c | 4 +++- include/linux/binfmts.h | 2 +- 5 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index e4653bb99946..8a38689ae4b0 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1021,7 +1021,9 @@ static int load_elf_binary(struct linux_binprm *bprm) if (!(current->personality & ADDR_NO_RANDOMIZE) && snapshot_randomize_va_space) current->flags |= PF_RANDOMIZE;

- setup_new_exec(bprm); + retval = setup_new_exec(bprm); + if (retval) + goto out_free_dentry;

/* Do this so that we can load the interpreter, if need be. We will change some of these later */ diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index 48fd2de3bca0..7ad98b8132fc 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -351,7 +351,9 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) if (elf_read_implies_exec(&exec_params.hdr, executable_stack)) current->personality |= READ_IMPLIES_EXEC;

- setup_new_exec(bprm); + retval = setup_new_exec(bprm); + if (retval) + goto error;

set_binfmt(&elf_fdpic_format);

diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c index b5b5ca1a44f7..78e9ca128ea7 100644 --- a/fs/binfmt_flat.c +++ b/fs/binfmt_flat.c @@ -512,7 +512,9 @@ static int load_flat_file(struct linux_binprm *bprm,

/* OK, This is the point of no return */ set_personality(PER_LINUX_32BIT); - setup_new_exec(bprm); + ret = setup_new_exec(bprm); + if (ret) + goto err;

/* * calculate the extra space we need to map in diff --git a/fs/exec.c b/fs/exec.c index 6b70c6726d31..136a7ab5d91c 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1321,7 +1321,7 @@ void would_dump(struct linux_binprm *bprm, struct file *file) } EXPORT_SYMBOL(would_dump);

-void setup_new_exec(struct linux_binprm * bprm) +int setup_new_exec(struct linux_binprm * bprm) { /* Setup things that can depend upon the personality */ struct task_struct *me = current; @@ -1337,6 +1337,8 @@ void setup_new_exec(struct linux_binprm * bprm) me->mm->task_size = TASK_SIZE; up_write(&me->signal->exec_update_lock); mutex_unlock(&me->signal->cred_guard_mutex); + + return 0; } EXPORT_SYMBOL(setup_new_exec);

diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 65abd5ab8836..678b7525ac5a 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -123,7 +123,7 @@ extern void unregister_binfmt(struct linux_binfmt *);

extern int __must_check remove_arg_zero(struct linux_binprm *); extern int begin_new_exec(struct linux_binprm * bprm); -extern void setup_new_exec(struct linux_binprm * bprm); +extern int setup_new_exec(struct linux_binprm * bprm); extern void finalize_exec(struct linux_binprm *bprm); extern void would_dump(struct linux_binprm *, struct file *);

The previous patch fixed the deadlock when mt-exec waits for debugger which should reap a zombie thread, but we can hit the same problem if the killed sub-thread stops in ptrace_event(PTRACE_EVENT_EXIT). Change ptrace_stop() to check signal->group_exit_task.

This is a user-visible change. But hopefully it can't break anything. Note that the semantics of PTRACE_EVENT_EXIT was never really defined, it depends on /dev/random. Just for example, currently a sub-thread killed by exec will stop, but if it exits on its own and races with exec it will not stop, so nobody can rely on PTRACE_EVENT_EXIT anyway.

We really need to finally define what PTRACE_EVENT_EXIT should actually do, but this needs other changes.

Signed-off-by: Oleg Nesterov oleg@redhat.com --- kernel/signal.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/kernel/signal.c b/kernel/signal.c index 334212044940..59f61e07905b 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2376,6 +2376,10 @@ static int ptrace_stop(int exit_code, int why, unsigned long message, if (!current->ptrace || __fatal_signal_pending(current)) return exit_code;

+ /* de_thread() -> wait_for_notify_count() waits for us */ + if (current->signal->group_exec_task) + return exit_code; + set_special_state(TASK_TRACED); current->jobctl |= JOBCTL_TRACED;