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.

+++ 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:

/** * 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. */

@@ -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'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")

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.

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?

-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()

• 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().

• 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...

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 ?

Oleg.

On 1/22/24 22:30, Kees Cook wrote:

On Mon, Jan 22, 2024 at 02:24:37PM +0100, Bernd Edlinger wrote:

...

The main concern was when a set-suid program is executed by execve. Then it makes a difference if the current thread is traced before the execve or not. That means if the current thread is already traced, the decision, which credentials will be used is different than otherwise.

So currently there are two possbilities, either the trace happens before the execve, and the suid-bit will be ignored, or the trace happens after the execve, but it is checked that the now potentially more privileged credentials allow the tracer to proceed.

With this patch we will have a third prossibility, that is in order to avoid the possible dead-lock we allow the suid-bit to take effect, but only if the tracer's privileges allow both to attach the current credentials and the new credentials. But I would only do that as a last resort, to avoid the possible dead-lock, and not unless a dead-lock is really expected to happen.

Instead of doing this special cred check (which I am worried could become fragile -- I'd prefer all privilege checks happen in the same place and in the same way...), could we just fail the ptrace_attach of the execve?

Hmm, yes. That is also possible, and that was actually my first approach, but I think the current patch is superior. I have nevertheless tried it again, to get a better picture of the differences between those two approaches.

See below for how that alternative approach would look like: + the advantage of that would be simplicity. + it avoids the dead-lock in the tracer. - it is an API change, which we normally try to avoid. - the adjusted test case(s) show that the tracer cannot successfully attach to the resulting process before the execve'd process starts up. So although there is no suid process involved in my test cases, the traced program simply escapes out of the tracer's control.

The advantage of the current approach would be: + it avoids the dead-lock in the tracer + it avoids a potentially breaking API change. + the tracer is normally able to successfully attach to the resulting process after the execve completes, before it starts to execute. + the debug experience is just better. - worst case that can happen, is that the security policy denies the tracer the access to the new process after the execve. In that case the PTRACE_ATTACH will fail each time it is attempted, in a similar way as the the alternate approach. But the overall result is still correct. The privileged process escapes, and that is okay in that case. - it is theoretically possible that the security engine gets confused by the additional call to security_ptrace_access_check, but that will be something that can be fixed, when it happens.

However my main motivation, why I started this work was the security implication.

I assume the tracer is a privileged process, like an anti virus program, that supervises all processes and if it detects some anomaly it can ptrace attach to the target, check what it does and prevent it from doing bad things.

- Currently a non-privileged program can potentially send such a privileged tracer into a deadlock. - With the alternative patch below that non-privileged can no longer send the tracer into a deadlock, but it can still quickly escape out of the tracer's control. - But with my latest patch a sufficiently privileged tracer can neither be sent into a deadlock nor can the attached process escape. Mission completed.

Thanks Bernd.

Here is the alternative patch for reference: diff --git a/fs/exec.c b/fs/exec.c index e88249a1ce07..0a948f5821b7 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1045,6 +1045,8 @@ static int de_thread(struct task_struct *tsk) struct signal_struct *sig = tsk->signal; struct sighand_struct *oldsighand = tsk->sighand; spinlock_t *lock = &oldsighand->siglock; + struct task_struct *t; + bool unsafe_execve_in_progress = false;

if (thread_group_empty(tsk)) goto no_thread_group; @@ -1067,6 +1069,18 @@ 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); + 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 +1171,9 @@ static int de_thread(struct task_struct *tsk) release_task(leader); }

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

@@ -1168,6 +1185,9 @@ static int de_thread(struct task_struct *tsk) return 0;

killed: + if (unlikely(unsafe_execve_in_progress)) + mutex_lock(&sig->cred_guard_mutex); + /* protects against exit_notify() and __exit_signal() */ read_lock(&tasklist_lock); sig->group_exec_task = NULL; @@ -1479,6 +1499,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->group_exec_task)) { + 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..55816320c103 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->group_exec_task)) { + 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/kernel/ptrace.c b/kernel/ptrace.c index 2fabd497d659..162e4c8f7b08 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -444,6 +444,9 @@ static int ptrace_attach(struct task_struct *task, long request, scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR, &task->signal->cred_guard_mutex) {

+ if (unlikely(task->signal->group_exec_task)) + return -EAGAIN; + scoped_guard (task_lock, task) { retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS); if (retval) @@ -491,6 +494,14 @@ static int ptrace_traceme(void) { int ret = -EPERM;

+ if (mutex_lock_interruptible(&current->signal->cred_guard_mutex)) + return -ERESTARTNOINTR; + + if (unlikely(current->signal->group_exec_task)) { + mutex_unlock(&current->signal->cred_guard_mutex); + return -ERESTARTNOINTR; + } + write_lock_irq(&tasklist_lock); /* Are we already being traced? */ if (!current->ptrace) { @@ -506,6 +517,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..6a136d6ddf7c 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->group_exec_task)) { + 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..7a51a350a068 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,67 @@ 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(errno, EAGAIN); 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 = ptrace(PTRACE_ATTACH, pid, 0L, 0L); + ASSERT_EQ(k, -1); + ASSERT_EQ(errno, EAGAIN); + k = waitpid(-1, &s, 0); + ASSERT_EQ(k, pid); + ASSERT_EQ(WIFEXITED(s), 1); + ASSERT_EQ(WEXITSTATUS(s), 1); + k = ptrace(PTRACE_ATTACH, pid, 0L, 0L); + ASSERT_EQ(k, -1); + ASSERT_EQ(errno, ESRCH); + 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 +144,40 @@ 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, -1); + ASSERT_EQ(errno, EAGAIN); + k = waitpid(-1, &s, WNOHANG); + ASSERT_EQ(k, pid); + ASSERT_EQ(WIFSTOPPED(s), 1); + ASSERT_EQ(WSTOPSIG(s), SIGTRAP); + k = ptrace(PTRACE_ATTACH, thread2_tid, 0L, 0L); + ASSERT_EQ(k, -1); + ASSERT_EQ(errno, EAGAIN); + k = waitpid(-1, &s, WNOHANG); + ASSERT_EQ(k, 0); + k = ptrace(PTRACE_CONT, pid, 0L, 0L); ASSERT_EQ(k, 0); + k = ptrace(PTRACE_ATTACH, thread2_tid, 0L, 0L); + ASSERT_EQ(k, -1); + ASSERT_EQ(errno, EAGAIN); 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 = ptrace(PTRACE_ATTACH, thread2_tid, 0L, 0L); + ASSERT_EQ(k, -1); + ASSERT_EQ(errno, ESRCH); k = waitpid(-1, NULL, 0); ASSERT_EQ(k, -1); ASSERT_EQ(errno, ECHILD);

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

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.

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

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..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)) { + 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; + } + 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; + } + }

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

Hi Kees,

thanks for your response.

On 8/19/25 06:36, Kees Cook wrote:

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.

[...]

...

-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?

Yes, but after the fatal signal is sent to all threads, releasing the sighand->siglock is okay, since no new threads can be cloned any more, because kernel/fork.c checks for pending fatal signals after acquiring the mutex and before adding another thread to the list:

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

rv_task_fork(p);

rseq_fork(p, clone_flags);

/* Don't start children in a dying pid namespace */ if (unlikely(!(ns_of_pid(pid)->pid_allocated & PIDNS_ADDING))) { retval = -ENOMEM; goto bad_fork_core_free; }

/* Let kill terminate clone/fork in the middle */ if (fatal_signal_pending(current)) { retval = -EINTR; goto bad_fork_core_free; }

/* No more failure paths after this point. */

...

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.

Maybe, I thought that it would be clear by this description:

--- 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. */

...

• /* 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?

The credentials should be protected by cred_guard_mutex here, and/or current->signal->exec_bprm in case there are ptraced threads.

I need this earlier because the outcome of ptrace_may_access for the new credentials depends on the dumpability.

So when de_thread has to release the mutex, a ptrace_attach is possible without deadlock, but the tracer's access rights must be sufficient to access the current and the new credentials because, the tracer will receive the stop event immediately after the execve succeeds and the new credentials are in effect, or if the execve fails the stop event will see the old process using the old credentials dying of a fatal signal.

...

/* * 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/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?

No, because I want to use this function also in begin_new_exec in the moved code, to include the expected outcome of commit_creds before de_thread, to make sure the result of ptrace_may_access is as if the new credentials were already committed.

...

+{

• 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?

Hmm, Yes, I agree. I start to think that task->signal->exec_bprm would indeed be a better indicator:

--- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -454,7 +454,7 @@ static int ptrace_attach(struct task_struct *task, long request, return retval; }

- if (unlikely(task->in_execve)) { + if (unlikely(task->signal->exec_bprm)) { retval = down_write_killable(&task->signal->exec_update_lock); if (retval) return retval;

So I will add this to the next patch version.

...

• 	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.

My problem here is that we have a pre-exisiting code using mutex_lock_killable(&current->signal->cred_guard_mutex)) and other pre-existiong code using mutex_lock_interruptible I would not want to make any changes here sine there are probably good reasons why one is ignoring interrupts while the others do not ignore interrupts.

So currently I would not think that a helper function for the two interruptible mutex locking instances would really make the code more easy to understand...

...

• }

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

Thank you for your review, if you would like me to add some more commments and maybe have any suggestions what to write and where, then I would be happy to add that in the next patch version.

Thanks Bernd.

Hi all,

This is a friendly ping, just a gentle reminder since this series has been around a while. FYI the patch still applies cleanly to current kernel sources, compiles correctly and tests are still passed.

Thanks Bernd.

On 8/21/25 19:34, 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 | 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 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.

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/17, Bernd Edlinger wrote:

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

...

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

...

...

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.

I would not like to drop the mutex when no absolutely necessary for performance reasons.

OK, I won't insist... But I don't really understand how this can help to improve the performance. If nothing else, this adds another for_other_threads() loop.

And again, the unsafe_execve_in_progress == T case is unlikely. I'm afraid this case (de_thread() without cred_guard_mutex) won't have enough testing.

In any case, why you dislike the suggestion to add this unsafe_execve_in_progress logic in a separate patch?

...

...

...

• 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...

Since the spin lock was acquired while holding the mutex, both should be unlocked in reverse sequence and the spin lock re-acquired after releasing the mutex.

Why?

I'd expect the scheduler to do a task switch after the cred_guard_mutex is unlocked, at least in the RT-linux variant, while the spin lock is not yet unlocked.

I must have missed something, but I still don't understand why this would be wrong...

...

...

...

@@ -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.

The dumpability need to be determined before de_thread, because ptrace_may_access needs this information to determine if the tracer is allowed to ptrace. That is part of the core of the patch, it would not work without that.

Yes,

I will add more comments to make that more easy to understand.

But again, why this change can't come in a separate patch? Before the patch which drops cred_guard_mutex in de_thread().

...

...

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;

}

?

Some use mutex_lock_interruptible and some use mutex_lock_killable here, so it wont work for all of them. I would not consider this a new kind of dead-lock free mutex, but just an open-coded state machine, handling the state that the tasks have whild de_thread is running.

OK. and we don't have mutex_lock_state(). I think that all users could use mutex_lock_killable(), but you are right anyway, and this is minor.

...

...

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.

Yes. Although this is minor too ;)

...

...

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.

Okay I understand your point. I did this originally just to avoid to have to change the interface to all the security engines, but instead I could add a flag PTRACE_MODE_BPRMCREDS to the ptrace_may_access which must be handled in all security engines, to use child->signal->exec_bprm->creds instead of __task_cred(child).

Can't comment... I don't understand your idea, but this is my fault. I guess this needs more changes, in particular __ptrace_may_access_mm_cred(), but most probably I misunderstood your idea.

...

...

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...

That means the tracee is already ptraced before the execve, and SUID-bits do not work as usual, and are more or less ignored. But in this patch the tracee is not yet ptraced.

Well. I meant that if LSM_UNSAFE_PTRACE is not set, then currently (say) security_bprm_committing_creds() has all rights to assume that the execing task is not ptraced. Yes, I don't see any potential problem right now, but still.

And just in case... Lets look at this code

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

again.

This is mostly theoretical, but what if begin_new_exec() fails after de_thread() and before exec_mmap() and/or commit_creds(bprm->cred) ? In this case the execing thread will report SIGSEGV to debugger which can (say) read old_mm.

No?

I am starting to think that ptrace_attach() should simply fail with -EWOULDBLOCK if it detects "unsafe_execve_in_progress" ... And perhaps this is what you already tried to do in the past, I can't recall :/

Oleg.

This simple program

#include <unistd.h> #include <signal.h> #include <sys/ptrace.h> #include <pthread.h>

void *thread(void *arg) { ptrace(PTRACE_TRACEME, 0,0,0); return NULL; }

int main(void) { int pid = fork();

if (!pid) { pthread_t pt; pthread_create(&pt, NULL, thread, NULL); pthread_join(pt, NULL); execlp("echo", "echo", "passed", NULL); }

sleep(1); ptrace(PTRACE_ATTACH, pid, 0,0); kill(pid, SIGCONT);

return 0; }

hangs because de_thread() waits for debugger which should release the killed thread with cred_guard_mutex held, while the debugger sleeps waiting for the same mutex. Not really that bad, the tracer can be killed, but still this is a bug and people hit it in practice.

With this patch:

- de_thread() waits until all the sub-threads pass exit_notify() and become zombies.

- setup_new_exec() waits until all the sub-threads are reaped without cred_guard_mutex held.

- unshare_sighand() and flush_signal_handlers() are moved from begin_new_exec() to setup_new_exec(), we can't call them until all sub-threads go away.

Signed-off-by: Oleg Nesterov oleg@redhat.com --- fs/exec.c | 140 +++++++++++++++++++++++------------------------- kernel/exit.c | 9 ++-- kernel/signal.c | 2 +- 3 files changed, 71 insertions(+), 80 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c index 136a7ab5d91c..2bac7deb9a98 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -905,42 +905,56 @@ static int exec_mmap(struct mm_struct *mm) return 0; }

-static int de_thread(struct task_struct *tsk) +static int kill_sub_threads(struct task_struct *tsk) { struct signal_struct *sig = tsk->signal; - struct sighand_struct *oldsighand = tsk->sighand; - spinlock_t *lock = &oldsighand->siglock; - - if (thread_group_empty(tsk)) - goto no_thread_group; + int err = -EINTR;

- /* - * Kill all other threads in the thread group. - */ - spin_lock_irq(lock); - if ((sig->flags & SIGNAL_GROUP_EXIT) || sig->group_exec_task) { - /* - * Another group action in progress, just - * return so that the signal is processed. - */ - spin_unlock_irq(lock); - return -EAGAIN; + read_lock(&tasklist_lock); + spin_lock_irq(&tsk->sighand->siglock); + if (!((sig->flags & SIGNAL_GROUP_EXIT) || sig->group_exec_task)) { + sig->group_exec_task = tsk; + sig->notify_count = -zap_other_threads(tsk); + err = 0; } + spin_unlock_irq(&tsk->sighand->siglock); + read_unlock(&tasklist_lock);

- sig->group_exec_task = tsk; - sig->notify_count = zap_other_threads(tsk); - if (!thread_group_leader(tsk)) - sig->notify_count--; + return err; +}

- while (sig->notify_count) { - __set_current_state(TASK_KILLABLE); - spin_unlock_irq(lock); - schedule(); +static int wait_for_notify_count(struct task_struct *tsk) +{ + for (;;) { if (__fatal_signal_pending(tsk)) - goto killed; - spin_lock_irq(lock); + return -EINTR; + set_current_state(TASK_KILLABLE); + if (!tsk->signal->notify_count) + break; + schedule(); } - spin_unlock_irq(lock); + __set_current_state(TASK_RUNNING); + return 0; +} + +static void clear_group_exec_task(struct task_struct *tsk) +{ + struct signal_struct *sig = tsk->signal; + + /* protects against exit_notify() and __exit_signal() */ + read_lock(&tasklist_lock); + sig->group_exec_task = NULL; + sig->notify_count = 0; + read_unlock(&tasklist_lock); +} + +static int de_thread(struct task_struct *tsk) +{ + if (thread_group_empty(tsk)) + goto no_thread_group; + + if (kill_sub_threads(tsk) || wait_for_notify_count(tsk)) + return -EINTR;

/* * At this point all other threads have exited, all we have to @@ -948,26 +962,10 @@ static int de_thread(struct task_struct *tsk) * and to assume its PID: */ if (!thread_group_leader(tsk)) { - struct task_struct *leader = tsk->group_leader; - - for (;;) { - cgroup_threadgroup_change_begin(tsk); - write_lock_irq(&tasklist_lock); - /* - * Do this under tasklist_lock to ensure that - * exit_notify() can't miss ->group_exec_task - */ - 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; - } + struct task_struct *leader = tsk->group_leader, *t;

+ cgroup_threadgroup_change_begin(tsk); + write_lock_irq(&tasklist_lock); /* * The only record we have of the real-time age of a * process, regardless of execs it's done, is start_time. @@ -1000,8 +998,8 @@ static int de_thread(struct task_struct *tsk) list_replace_rcu(&leader->tasks, &tsk->tasks); list_replace_init(&leader->sibling, &tsk->sibling);

- tsk->group_leader = tsk; - leader->group_leader = tsk; + for_each_thread(tsk, t) + t->group_leader = tsk;

tsk->exit_signal = SIGCHLD; leader->exit_signal = -1; @@ -1021,23 +1019,11 @@ static int de_thread(struct task_struct *tsk) release_task(leader); }

- sig->group_exec_task = NULL; - sig->notify_count = 0; - no_thread_group: /* we have changed execution domain */ tsk->exit_signal = SIGCHLD; - BUG_ON(!thread_group_leader(tsk)); return 0; - -killed: - /* protects against exit_notify() and __exit_signal() */ - read_lock(&tasklist_lock); - sig->group_exec_task = NULL; - sig->notify_count = 0; - read_unlock(&tasklist_lock); - return -EAGAIN; }

@@ -1171,13 +1157,6 @@ int begin_new_exec(struct linux_binprm * bprm) flush_itimer_signals(); #endif

- /* - * Make the signal table private. - */ - retval = unshare_sighand(me); - if (retval) - goto out_unlock; - me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_NOFREEZE | PF_NO_SETAFFINITY); flush_thread(); @@ -1249,7 +1228,6 @@ int begin_new_exec(struct linux_binprm * bprm) /* An exec changes our domain. We are no longer part of the thread group */ WRITE_ONCE(me->self_exec_id, me->self_exec_id + 1); - flush_signal_handlers(me, 0);

retval = set_cred_ucounts(bprm->cred); if (retval < 0) @@ -1293,8 +1271,9 @@ int begin_new_exec(struct linux_binprm * bprm) up_write(&me->signal->exec_update_lock); if (!bprm->cred) mutex_unlock(&me->signal->cred_guard_mutex); - out: + if (me->signal->group_exec_task == me) + clear_group_exec_task(me); return retval; } EXPORT_SYMBOL(begin_new_exec); @@ -1325,6 +1304,8 @@ int setup_new_exec(struct linux_binprm * bprm) { /* Setup things that can depend upon the personality */ struct task_struct *me = current; + struct signal_struct *sig = me->signal; + int err = 0;

arch_pick_mmap_layout(me->mm, &bprm->rlim_stack);

@@ -1335,10 +1316,23 @@ int setup_new_exec(struct linux_binprm * bprm) * some architectures like powerpc */ me->mm->task_size = TASK_SIZE; - up_write(&me->signal->exec_update_lock); - mutex_unlock(&me->signal->cred_guard_mutex); + up_write(&sig->exec_update_lock); + mutex_unlock(&sig->cred_guard_mutex);

- return 0; + if (sig->group_exec_task) { + spin_lock_irq(&me->sighand->siglock); + sig->notify_count = sig->nr_threads - 1; + spin_unlock_irq(&me->sighand->siglock); + + err = wait_for_notify_count(me); + clear_group_exec_task(me); + } + + if (!err) + err = unshare_sighand(me); + if (!err) + flush_signal_handlers(me, 0); + return err; } EXPORT_SYMBOL(setup_new_exec);

diff --git a/kernel/exit.c b/kernel/exit.c index f041f0c05ebb..bcde78c97253 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -178,10 +178,7 @@ static void __exit_signal(struct release_task_post *post, struct task_struct *ts tty = sig->tty; sig->tty = NULL; } else { - /* - * If there is any task waiting for the group exit - * then notify it: - */ + /* mt-exec, setup_new_exec() -> wait_for_notify_count() */ if (sig->notify_count > 0 && !--sig->notify_count) wake_up_process(sig->group_exec_task);

@@ -766,8 +763,8 @@ static void exit_notify(struct task_struct *tsk, int group_dead) list_add(&tsk->ptrace_entry, &dead); }

- /* mt-exec, de_thread() is waiting for group leader */ - if (unlikely(tsk->signal->notify_count < 0)) + /* mt-exec, de_thread() -> wait_for_notify_count() */ + if (tsk->signal->notify_count < 0 && !++tsk->signal->notify_count) wake_up_process(tsk->signal->group_exec_task); write_unlock_irq(&tasklist_lock);

diff --git a/kernel/signal.c b/kernel/signal.c index fe9190d84f28..334212044940 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -1343,13 +1343,13 @@ int zap_other_threads(struct task_struct *p)

for_other_threads(p, t) { task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK); - count++;

/* Don't bother with already dead threads */ if (t->exit_state) continue; sigaddset(&t->pending.signal, SIGKILL); signal_wake_up(t, 1); + count++; }

return count;

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 with the new PTRACE_MODE_BPRMCREDS flag to check that the tracer has permission to trace 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 | 86 +++++++++++--- fs/proc/base.c | 12 ++ include/linux/cred.h | 1 + include/linux/ptrace.h | 1 + include/linux/sched/signal.h | 18 +++ kernel/cred.c | 30 ++++- kernel/ptrace.c | 29 ++++- kernel/seccomp.c | 18 ++- security/apparmor/lsm.c | 5 +- security/commoncap.c | 5 +- security/landlock/task.c | 7 +- security/selinux/hooks.c | 7 +- security/smack/smack_lsm.c | 5 +- security/yama/yama_lsm.c | 11 +- tools/testing/selftests/ptrace/vmaccess.c | 135 ++++++++++++++++++++-- 15 files changed, 324 insertions(+), 46 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.

v18: Add some more comments, avoid the temporaty impersonation of the new credentials, and use instead a new option to ptrace_may_access. All security engines have to handle this option, but the advantage is that the engines could detect and maybe also deny the unsafe execve.

Thanks Bernd.

diff --git a/fs/exec.c b/fs/exec.c index 4298e7e08d5d..02f3e8469125 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,36 @@ 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; + break; + } + } + + if (unlikely(unsafe_execve_in_progress)) { + /* + * Since the spin lock was acquired while holding the + * mutex, both should be unlocked in reverse sequence and + * the spin lock re-acquired after releasing the mutex. + */ + spin_unlock_irq(lock); + /* + * Sibling threads are notified by the non-zero exec_bprm, + * that they have just been zapped, and the cred_guard_mutex + * is to be released by them immediately. + * The caller of ptrace_attach on the other hand is allowed + * to ptrace any additional sibling threads that may not yet + * have ben ptraced, but if the group_exec_task is being + * ptraced, an additional check has to be performed, that the + * tracer is allowed to ptrace the new exec credentials. + */ + 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 +1053,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 +1069,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 +1156,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 +1204,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 +1265,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();

/* @@ -1275,6 +1318,10 @@ int begin_new_exec(struct linux_binprm * bprm) * cred_guard_mutex must be held at least to this point to prevent * ptrace_attach() from altering our determination of the task's * credentials; any time after this it may be unlocked. + * Note that de_thread may temporarily release the cred_guard_mutex, + * but the credentials are pre-determined in that case and the ptrace + * access check guarantees, that the access permissions of the tracer + * are sufficient to trace the task also with the new credentials. */ security_bprm_committed_creds(bprm);

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

+ /* It is not necessary to check current->signal->exec_bprm here. */ bprm->cred = prepare_exec_creds(); if (likely(bprm->cred)) return 0; diff --git a/fs/proc/base.c b/fs/proc/base.c index 6299878e3d97..f554c2638ffb 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2838,6 +2838,18 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, if (rv < 0) goto out_free;

+ /* + * A fatal signal is guaranteed to be already pending in the + * unlikely event, that current->signal->exec_bprm happens + * to be non-zero here, so just release the mutex again + * and continue as if mutex_lock_interruptible did fail. + */ + 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 89ae50ad2ace..4204c19de714 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/ptrace.h b/include/linux/ptrace.h index 90507d4afcd6..dbd58a4807bc 100644 --- a/include/linux/ptrace.h +++ b/include/linux/ptrace.h @@ -64,6 +64,7 @@ extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead); #define PTRACE_MODE_NOAUDIT 0x04 #define PTRACE_MODE_FSCREDS 0x08 #define PTRACE_MODE_REALCREDS 0x10 +#define PTRACE_MODE_BPRMCREDS 0x20

/* shorthands for READ/ATTACH and FSCREDS/REALCREDS combinations */ #define PTRACE_MODE_READ_FSCREDS (PTRACE_MODE_READ | PTRACE_MODE_FSCREDS) diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h index 7d6449982822..ade7d7173875 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -241,9 +241,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 dbf6b687dc5c..69fd0de813c0 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..ac750d1ccd04 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> @@ -285,6 +286,11 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode) return -EPERM; }

+ if ((mode & PTRACE_MODE_BPRMCREDS) && !task->signal->exec_bprm) { + WARN(1, "denying ptrace access check with PTRACE_MODE_BPRMCREDS\n"); + return -EPERM; + } + /* May we inspect the given task? * This check is used both for attaching with ptrace * and for allowing access to sensitive information in /proc. @@ -313,7 +319,10 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode) caller_uid = cred->uid; caller_gid = cred->gid; } - tcred = __task_cred(task); + if (mode & PTRACE_MODE_BPRMCREDS) + tcred = task->signal->exec_bprm->cred; + else + tcred = __task_cred(task); if (uid_eq(caller_uid, tcred->euid) && uid_eq(caller_uid, tcred->suid) && uid_eq(caller_uid, tcred->uid) && @@ -337,7 +346,10 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode) * Pairs with a write barrier in commit_creds(). */ smp_rmb(); - mm = task->mm; + if (mode & PTRACE_MODE_BPRMCREDS) + mm = task->signal->exec_bprm->mm; + else + mm = task->mm; if (mm && ((get_dumpable(mm) != SUID_DUMP_USER) && !ptrace_has_cap(mm->user_ns, mode))) @@ -451,6 +463,14 @@ static int ptrace_attach(struct task_struct *task, long request, retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS); if (retval) return retval; + + if (unlikely(task == task->signal->group_exec_task)) { + retval = __ptrace_may_access(task, + PTRACE_MODE_ATTACH_REALCREDS | + PTRACE_MODE_BPRMCREDS); + if (retval) + return retval; + } }

scoped_guard (write_lock_irq, &tasklist_lock) { @@ -488,6 +508,10 @@ static int ptrace_traceme(void) { int ret = -EPERM;

+ if (mutex_lock_interruptible(&current->signal->cred_guard_mutex)) + return -ERESTARTNOINTR; + + /* It is not necessary to check current->signal->exec_bprm here. */ write_lock_irq(&tasklist_lock); /* Are we already being traced? */ if (!current->ptrace) { @@ -503,6 +527,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 25f62867a16d..6e4ff108faa0 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -2010,9 +2010,21 @@ 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; + + /* + * A fatal signal is guaranteed to be already pending in the + * unlikely event, that current->signal->exec_bprm happens + * to be non-zero here, so just release the mutex again + * and continue as if mutex_lock_killable did fail. + */ + 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/security/apparmor/lsm.c b/security/apparmor/lsm.c index b3f7a3258a2c..80c2cd968f05 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -129,7 +129,10 @@ static int apparmor_ptrace_access_check(struct task_struct *child, int error; bool needput;

- cred = get_task_cred(child); + if (mode & PTRACE_MODE_BPRMCREDS) + cred = get_cred(child->signal->exec_bprm->cred); + else + cred = get_task_cred(child); tracee = cred_label(cred); /* ref count on cred */ tracer = __begin_current_label_crit_section(&needput); error = aa_may_ptrace(current_cred(), tracer, cred, tracee, diff --git a/security/commoncap.c b/security/commoncap.c index 6bd4adeb4795..e23a78a4514c 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -169,7 +169,10 @@ int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)

rcu_read_lock(); cred = current_cred(); - child_cred = __task_cred(child); + if (mode & PTRACE_MODE_BPRMCREDS) + child_cred = child->signal->exec_bprm->cred; + else + child_cred = __task_cred(child); if (mode & PTRACE_MODE_FSCREDS) caller_caps = &cred->cap_effective; else diff --git a/security/landlock/task.c b/security/landlock/task.c index 2385017418ca..46ce9b6e4728 100644 --- a/security/landlock/task.c +++ b/security/landlock/task.c @@ -17,6 +17,7 @@ #include <linux/rcupdate.h> #include <linux/sched.h> #include <linux/sched/signal.h> +#include <linux/binfmts.h> #include <net/af_unix.h> #include <net/sock.h>

@@ -96,7 +97,11 @@ static int hook_ptrace_access_check(struct task_struct *const child,

scoped_guard(rcu) { - child_dom = landlock_get_task_domain(child); + if (mode & PTRACE_MODE_BPRMCREDS) + child_dom = landlock_cred(child->signal-> + exec_bprm->cred)->domain; + else + child_dom = landlock_get_task_domain(child); err = domain_ptrace(parent_subject->domain, child_dom); }

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index dfc22da42f30..016e21180e96 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2111,7 +2111,12 @@ static int selinux_ptrace_access_check(struct task_struct *child, unsigned int mode) { u32 sid = current_sid(); - u32 csid = task_sid_obj(child); + u32 csid; + + if (mode & PTRACE_MODE_BPRMCREDS) + csid = cred_sid(child->signal->exec_bprm->cred); + else + csid = task_sid_obj(child);

if (mode & PTRACE_MODE_READ) return avc_has_perm(sid, csid, SECCLASS_FILE, FILE__READ, diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index af986587841d..5c2ca49baa05 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -476,7 +476,10 @@ static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode) { struct smack_known *skp;

- skp = smk_of_task_struct_obj(ctp); + if (mode & PTRACE_MODE_BPRMCREDS) + skp = smk_of_task(smack_cred(ctp->signal->exec_bprm->cred)); + else + skp = smk_of_task_struct_obj(ctp);

return smk_ptrace_rule_check(current, skp, mode, __func__); } diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c index 3d064dd4e03f..8ac9ce41d4f0 100644 --- a/security/yama/yama_lsm.c +++ b/security/yama/yama_lsm.c @@ -11,6 +11,7 @@ #include <linux/lsm_hooks.h> #include <linux/sysctl.h> #include <linux/ptrace.h> +#include <linux/binfmts.h> #include <linux/prctl.h> #include <linux/ratelimit.h> #include <linux/workqueue.h> @@ -363,13 +364,19 @@ static int yama_ptrace_access_check(struct task_struct *child, rc = -EPERM; if (!rc && !task_is_descendant(current, child) && !ptracer_exception_found(current, child) && - !ns_capable(__task_cred(child)->user_ns, CAP_SYS_PTRACE)) + !ns_capable(mode & PTRACE_MODE_BPRMCREDS ? + child->signal->exec_bprm->cred->user_ns : + __task_cred(child)->user_ns, + CAP_SYS_PTRACE)) rc = -EPERM; rcu_read_unlock(); break; case YAMA_SCOPE_CAPABILITY: rcu_read_lock(); - if (!ns_capable(__task_cred(child)->user_ns, CAP_SYS_PTRACE)) + if (!ns_capable(mode & PTRACE_MODE_BPRMCREDS ? + child->signal->exec_bprm->cred->user_ns : + __task_cred(child)->user_ns, + CAP_SYS_PTRACE)) rc = -EPERM; rcu_read_unlock(); break; 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);