| Message ID | AS8P193MB12851AC1F862B97FCE9B3F4FE4AAA@AS8P193MB1285.EURP193.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [v13] exec: Fix dead-lock in de_thread with ptrace_attach | expand |
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(¤t->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/16/24 16:22, Oleg Nesterov wrote: > I'll try to recall this problem and actually read the patch tommorrow... > > Hmm. but it doesn't apply to Linus's tree, you need to rebase it. > In particular, please note the recent commit 5431fdd2c181dd2eac2 > ("ptrace: Convert ptrace_attach() to use lock guards") > Oh, how ugly... Will this new C++-like "feature" ever make it into a stable branch? > On 01/15, Bernd Edlinger wrote: >> >> The problem happens when a tracer tries to ptrace_attach >> to a multi-threaded process, that does an execve in one of >> the threads at the same time, without doing that in a forked >> sub-process. That means: There is a race condition, when one >> or more of the threads are already ptraced, but the thread >> that invoked the execve is not yet traced. Now in this >> case the execve locks the cred_guard_mutex and waits for >> de_thread to complete. But that waits for the traced >> sibling threads to exit, and those have to wait for the >> tracer to receive the exit signal, but the tracer cannot >> call wait right now, because it is waiting for the ptrace >> call to complete, and this never does not happen. >> The traced process and the tracer are now in a deadlock >> situation, and can only be killed by a fatal signal. > > This looks very confusing to me. And even misleading. > > So IIRC the problem is "simple". > > de_thread() sleeps with cred_guard_mutex waiting for other threads to > exit and pass release_task/__exit_signal. > > If one of the sub-threads is traced, debugger should do ptrace_detach() > or wait() to release this tracee, the killed tracee won't autoreap. > Yes. but the tracer has to do its job, and that is ptrace_attach the remaining treads, it does not know that it would avoid a dead-lock when it calls wait(), instead of ptrace_attach. It does not know that the tracee has just called execve in one of the not yet traced threads. > Now. If debugger tries to take the same cred_guard_mutex before > detach/wait we have a deadlock. This is not specific to ptrace_attach(), > proc_pid_attr_write() takes this lock too. > > Right? Or are there other issues? > No, proc_pid_attr_write has no problem if it waits for cred_guard_mutex, because it is only called from one of the sibling threads, and zap_other_threads sends a SIGKILL to each of them, thus the mutex_lock_interruptible will stop waiting, and the thread will exit normally. It is only problematic when another process wants to lock the cred_guard_mutex, because it is not receiving a signal, when de_thread is waiting. The only other place where I am aware of this happening is ptrace_attach. >> -static int de_thread(struct task_struct *tsk) >> +static int de_thread(struct task_struct *tsk, struct linux_binprm *bprm) >> { >> struct signal_struct *sig = tsk->signal; >> struct sighand_struct *oldsighand = tsk->sighand; >> spinlock_t *lock = &oldsighand->siglock; >> + struct task_struct *t = tsk; >> + bool unsafe_execve_in_progress = false; >> >> if (thread_group_empty(tsk)) >> goto no_thread_group; >> @@ -1066,6 +1068,19 @@ static int de_thread(struct task_struct *tsk) >> if (!thread_group_leader(tsk)) >> sig->notify_count--; >> >> + while_each_thread(tsk, t) { > > for_other_threads() > Ah, okay. >> + if (unlikely(t->ptrace) >> + && (t != tsk->group_leader || !t->exit_state)) >> + unsafe_execve_in_progress = true; > > The !t->exit_state is not right... This sub-thread can already be a zombie > with ->exit_state != 0 but see above, it won't be reaped until the debugger > does wait(). > I dont think so. de_thread() handles the group_leader different than normal threads. That means normal threads have to wait for being released from the zombie state by the tracer: sig->notify_count > 0, and de_thread is woken up by __exit_signal Once those are gone, de_thread waits for the group leader to reach exit_state = ZOMBIE, but again only if the group_leader is not the current thread: signal->notify_count < 0, and de_thread is woken up by exit_notify. So his reflects exactly what condition has to be met, see: sig->notify_count = -1; if (likely(leader->exit_state)) break; __set_current_state(TASK_KILLABLE); write_unlock_irq(&tasklist_lock); cgroup_threadgroup_change_end(tsk); schedule(); if (__fatal_signal_pending(tsk)) goto killed; so when the group_leader's exit_state is already != 0 then the second wait state will not be entered. >> + if (unlikely(unsafe_execve_in_progress)) { >> + spin_unlock_irq(lock); >> + sig->exec_bprm = bprm; >> + mutex_unlock(&sig->cred_guard_mutex); >> + spin_lock_irq(lock); > > I don't understand why do we need to unlock and lock siglock here... > That is just a precaution because I did want to release the mutexes exactly in the reverse order as they were acquired. > But my main question is why do we need the unsafe_execve_in_progress boolean. > If this patch is correct and de_thread() can drop and re-acquire cread_guard_mutex > when one of the threads is traced, then why can't we do this unconditionally ? > I just wanted to keep the impact of the change as small as possible, including possible performance degradation due to double checking of credentials. Worst thing that could happen with this approach, is that a situation where today a dead-lock is imminentm does still not work correctly, but when no tracer is attached, nothing will change or be less performant than before. Bernd.
On 01/17, Bernd Edlinger wrote: > > >> > >> The problem happens when a tracer tries to ptrace_attach > >> to a multi-threaded process, that does an execve in one of > >> the threads at the same time, without doing that in a forked > >> sub-process. That means: There is a race condition, when one > >> or more of the threads are already ptraced, but the thread > >> that invoked the execve is not yet traced. Now in this > >> case the execve locks the cred_guard_mutex and waits for > >> de_thread to complete. But that waits for the traced > >> sibling threads to exit, and those have to wait for the > >> tracer to receive the exit signal, but the tracer cannot > >> call wait right now, because it is waiting for the ptrace > >> call to complete, and this never does not happen. > >> The traced process and the tracer are now in a deadlock > >> situation, and can only be killed by a fatal signal. > > > > This looks very confusing to me. And even misleading. > > > > So IIRC the problem is "simple". > > > > de_thread() sleeps with cred_guard_mutex waiting for other threads to > > exit and pass release_task/__exit_signal. > > > > If one of the sub-threads is traced, debugger should do ptrace_detach() > > or wait() to release this tracee, the killed tracee won't autoreap. > > > > Yes. but the tracer has to do its job, and that is ptrace_attach the > remaining treads, it does not know that it would avoid a dead-lock > when it calls wait(), instead of ptrace_attach. It does not know > that the tracee has just called execve in one of the not yet traced > threads. Hmm. I don't understand you. I agree we have a problem which should be fixed. Just the changelog looks confusing to me, imo it doesn't explain the race/problem clearly. > > Now. If debugger tries to take the same cred_guard_mutex before > > detach/wait we have a deadlock. This is not specific to ptrace_attach(), > > proc_pid_attr_write() takes this lock too. > > > > Right? Or are there other issues? > > > > No, proc_pid_attr_write has no problem if it waits for cred_guard_mutex, > because it is only called from one of the sibling threads, OK, thanks, I was wrong. I forgot about "A task may only write its own attributes". So yes, ptrace_attach() is the only source of problematic mutex_lock() today. There were more in the past. > >> + if (unlikely(t->ptrace) > >> + && (t != tsk->group_leader || !t->exit_state)) > >> + unsafe_execve_in_progress = true; > > > > The !t->exit_state is not right... This sub-thread can already be a zombie > > with ->exit_state != 0 but see above, it won't be reaped until the debugger > > does wait(). > > > > I dont think so. > de_thread() handles the group_leader different than normal threads. I don't follow... I didn't say that t is a group leader. I said it can be a zombie sub-thread with ->exit_state != 0. > That means normal threads have to wait for being released from the zombie > state by the tracer: > sig->notify_count > 0, and de_thread is woken up by __exit_signal That is what I said before. Debugger should release a zombie sub-thread, it won't do __exit_signal() on its own. > >> + if (unlikely(unsafe_execve_in_progress)) { > >> + spin_unlock_irq(lock); > >> + sig->exec_bprm = bprm; > >> + mutex_unlock(&sig->cred_guard_mutex); > >> + spin_lock_irq(lock); > > > > I don't understand why do we need to unlock and lock siglock here... > > That is just a precaution because I did want to release the > mutexes exactly in the reverse order as they were acquired. To me this adds the unnecessary complication. > > But my main question is why do we need the unsafe_execve_in_progress boolean. > > If this patch is correct and de_thread() can drop and re-acquire cread_guard_mutex > > when one of the threads is traced, then why can't we do this unconditionally ? > > > > I just wanted to keep the impact of the change as small as possible, But the unsafe_execve_in_progress logic increases the impact and complicates the patch. I think the fix should be as simple as possible. (to be honest, right now I don't think this is a right approach). > including > possible performance degradation due to double checking of credentials. Not sure I understand, but you can add the performance improvements later. Not to mention that this should be justified, and the for_other_threads() loop added by this patch into de_thread() is not nice performance-wise. Oleg.
I'll try to read your email later, just one note for now... On 01/22, Bernd Edlinger wrote: > > > I didn't say that t is a group leader. I said it can be a zombie sub-thread > > with ->exit_state != 0. > > the condition here is > > (t != tsk->group_leader || !t->exit_state) > > so in other words, if t is a sub-thread, i.e. t != tsk->group_leader > then the t->exit_state does not count, Ah indeed, somehow I misread this check as if you skip the sub-threads with ->exit_state != 0. Sorry for noise. 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(¤t->signal->cred_guard_mutex)) return -ERESTARTNOINTR; + if (unlikely(current->signal->group_exec_task)) { + mutex_unlock(¤t->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(¤t->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(¤t->signal->cred_guard_mutex)) + return -ERESTARTNOINTR; + + if (unlikely(current->signal->group_exec_task)) { + mutex_unlock(¤t->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(¤t->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(¤t->signal->cred_guard_mutex)) - goto out_put_fd; + if (flags & SECCOMP_FILTER_FLAG_TSYNC) { + if (mutex_lock_killable(¤t->signal->cred_guard_mutex)) + goto out_put_fd; + + if (unlikely(current->signal->group_exec_task)) { + mutex_unlock(¤t->signal->cred_guard_mutex); + goto out_put_fd; + } + } spin_lock_irq(¤t->sighand->siglock); diff --git a/tools/testing/selftests/ptrace/vmaccess.c b/tools/testing/selftests/ptrace/vmaccess.c index 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);
On Tue, Jan 23, 2024 at 07:30:52PM +0100, Bernd Edlinger wrote: > - 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 for the details. And it would be pretty unfriendly to fail the execve() too (or, rather, it makes the execve failure unpredictable). I'll keep reading your patch...
diff --git a/fs/exec.c b/fs/exec.c index 2f2b0acec4f0..902d3b230485 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1041,11 +1041,13 @@ static int exec_mmap(struct mm_struct *mm) return 0; } -static int de_thread(struct task_struct *tsk) +static int de_thread(struct task_struct *tsk, struct linux_binprm *bprm) { struct signal_struct *sig = tsk->signal; struct sighand_struct *oldsighand = tsk->sighand; spinlock_t *lock = &oldsighand->siglock; + struct task_struct *t = tsk; + bool unsafe_execve_in_progress = false; if (thread_group_empty(tsk)) goto no_thread_group; @@ -1068,6 +1070,19 @@ static int de_thread(struct task_struct *tsk) if (!thread_group_leader(tsk)) sig->notify_count--; + while_each_thread(tsk, t) { + if (unlikely(t->ptrace) + && (t != tsk->group_leader || !t->exit_state)) + unsafe_execve_in_progress = true; + } + + if (unlikely(unsafe_execve_in_progress)) { + spin_unlock_irq(lock); + sig->exec_bprm = bprm; + mutex_unlock(&sig->cred_guard_mutex); + spin_lock_irq(lock); + } + while (sig->notify_count) { __set_current_state(TASK_KILLABLE); spin_unlock_irq(lock); @@ -1158,6 +1173,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; @@ -1169,6 +1189,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; @@ -1253,6 +1278,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. */ @@ -1261,7 +1304,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; @@ -1284,11 +1327,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 */ @@ -1350,18 +1388,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); @@ -1480,6 +1506,11 @@ static int prepare_bprm_creds(struct linux_binprm *bprm) if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex)) return -ERESTARTNOINTR; + if (unlikely(current->signal->exec_bprm)) { + mutex_unlock(¤t->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 ffd54617c354..0da9adfadb48 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2788,6 +2788,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(¤t->signal->cred_guard_mutex); + rv = -ERESTARTNOINTR; + goto out_free; + } + rv = security_setprocattr(PROC_I(inode)->op.lsm, file->f_path.dentry->d_name.name, page, count); diff --git a/include/linux/cred.h b/include/linux/cred.h index f923528d5cc4..b01e309f5686 100644 --- a/include/linux/cred.h +++ b/include/linux/cred.h @@ -159,6 +159,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 0014d3adaf84..14df7073a0a8 100644 --- a/include/linux/sched/signal.h +++ b/include/linux/sched/signal.h @@ -234,9 +234,27 @@ struct signal_struct { struct mm_struct *oom_mm; /* recorded mm when the thread group got * killed by the oom killer */ + struct linux_binprm *exec_bprm; /* Used to check ptrace_may_access + * against new credentials while + * de_thread is waiting for other + * traced threads to terminate. + * Set while de_thread is executing. + * The cred_guard_mutex is released + * after de_thread() has called + * zap_other_threads(), therefore + * a fatal signal is guaranteed to be + * already pending in the unlikely + * event, that + * current->signal->exec_bprm happens + * to be non-zero after the + * cred_guard_mutex was acquired. + */ + struct mutex cred_guard_mutex; /* guard against foreign influences on * credential calculations * (notably. ptrace) + * Held while execve runs, except when + * a sibling thread is being traced. * Deprecated do not use in new code. * Use exec_update_lock instead. */ diff --git a/kernel/cred.c b/kernel/cred.c index 98cb4eca23fb..586cb6c7cf6b 100644 --- a/kernel/cred.c +++ b/kernel/cred.c @@ -433,6 +433,28 @@ static bool cred_cap_issubset(const struct cred *set, const struct cred *subset) return false; } +/** + * is_dumpability_changed - Will changing creds from old to new + * affect the dumpability in commit_creds? + * + * Return: false - dumpability will not be changed in commit_creds. + * true - dumpability will be changed to non-dumpable. + * + * @old: The old credentials + * @new: The new credentials + */ +bool is_dumpability_changed(const struct cred *old, const struct cred *new) +{ + if (!uid_eq(old->euid, new->euid) || + !gid_eq(old->egid, new->egid) || + !uid_eq(old->fsuid, new->fsuid) || + !gid_eq(old->fsgid, new->fsgid) || + !cred_cap_issubset(old, new)) + return true; + + return false; +} + /** * commit_creds - Install new credentials upon the current task * @new: The credentials to be assigned @@ -467,11 +489,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 443057bee87c..eb1c450bb7d7 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -20,6 +20,7 @@ #include <linux/pagemap.h> #include <linux/ptrace.h> #include <linux/security.h> +#include <linux/binfmts.h> #include <linux/signal.h> #include <linux/uio.h> #include <linux/audit.h> @@ -435,6 +436,28 @@ static int ptrace_attach(struct task_struct *task, long request, if (retval) goto unlock_creds; + if (unlikely(task->in_execve)) { + struct linux_binprm *bprm = task->signal->exec_bprm; + const struct cred __rcu *old_cred; + struct mm_struct *old_mm; + + retval = down_write_killable(&task->signal->exec_update_lock); + if (retval) + goto unlock_creds; + task_lock(task); + old_cred = task->real_cred; + old_mm = task->mm; + rcu_assign_pointer(task->real_cred, bprm->cred); + task->mm = bprm->mm; + retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS); + rcu_assign_pointer(task->real_cred, old_cred); + task->mm = old_mm; + task_unlock(task); + up_write(&task->signal->exec_update_lock); + if (retval) + goto unlock_creds; + } + write_lock_irq(&tasklist_lock); retval = -EPERM; if (unlikely(task->exit_state)) @@ -508,6 +531,14 @@ static int ptrace_traceme(void) { int ret = -EPERM; + if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex)) + return -ERESTARTNOINTR; + + if (unlikely(current->signal->exec_bprm)) { + mutex_unlock(¤t->signal->cred_guard_mutex); + return -ERESTARTNOINTR; + } + write_lock_irq(&tasklist_lock); /* Are we already being traced? */ if (!current->ptrace) { @@ -523,6 +554,7 @@ static int ptrace_traceme(void) } } write_unlock_irq(&tasklist_lock); + mutex_unlock(¤t->signal->cred_guard_mutex); return ret; } diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 255999ba9190..b29bbfa0b044 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1955,9 +1955,15 @@ static long seccomp_set_mode_filter(unsigned int flags, * Make sure we cannot change seccomp or nnp state via TSYNC * while another thread is in the middle of calling exec. */ - if (flags & SECCOMP_FILTER_FLAG_TSYNC && - mutex_lock_killable(¤t->signal->cred_guard_mutex)) - goto out_put_fd; + if (flags & SECCOMP_FILTER_FLAG_TSYNC) { + if (mutex_lock_killable(¤t->signal->cred_guard_mutex)) + goto out_put_fd; + + if (unlikely(current->signal->exec_bprm)) { + mutex_unlock(¤t->signal->cred_guard_mutex); + goto out_put_fd; + } + } spin_lock_irq(¤t->sighand->siglock); diff --git a/tools/testing/selftests/ptrace/vmaccess.c b/tools/testing/selftests/ptrace/vmaccess.c index 4db327b44586..3b7d81fb99bb 100644 --- a/tools/testing/selftests/ptrace/vmaccess.c +++ b/tools/testing/selftests/ptrace/vmaccess.c @@ -39,8 +39,15 @@ TEST(vmaccess) f = open(mm, O_RDONLY); ASSERT_GE(f, 0); close(f); - f = kill(pid, SIGCONT); - ASSERT_EQ(f, 0); + f = waitpid(-1, NULL, 0); + ASSERT_NE(f, -1); + ASSERT_NE(f, 0); + ASSERT_NE(f, pid); + f = waitpid(-1, NULL, 0); + ASSERT_EQ(f, pid); + f = waitpid(-1, NULL, 0); + ASSERT_EQ(f, -1); + ASSERT_EQ(errno, ECHILD); } TEST(attach) @@ -57,22 +64,24 @@ TEST(attach) sleep(1); k = ptrace(PTRACE_ATTACH, pid, 0L, 0L); - ASSERT_EQ(errno, EAGAIN); - ASSERT_EQ(k, -1); + ASSERT_EQ(k, 0); k = waitpid(-1, &s, WNOHANG); ASSERT_NE(k, -1); ASSERT_NE(k, 0); ASSERT_NE(k, pid); ASSERT_EQ(WIFEXITED(s), 1); ASSERT_EQ(WEXITSTATUS(s), 0); - sleep(1); - k = ptrace(PTRACE_ATTACH, pid, 0L, 0L); + k = waitpid(-1, &s, 0); + ASSERT_EQ(k, pid); + ASSERT_EQ(WIFSTOPPED(s), 1); + ASSERT_EQ(WSTOPSIG(s), SIGTRAP); + k = ptrace(PTRACE_CONT, pid, 0L, 0L); ASSERT_EQ(k, 0); k = waitpid(-1, &s, 0); ASSERT_EQ(k, pid); ASSERT_EQ(WIFSTOPPED(s), 1); ASSERT_EQ(WSTOPSIG(s), SIGSTOP); - k = ptrace(PTRACE_DETACH, pid, 0L, 0L); + k = ptrace(PTRACE_CONT, pid, 0L, 0L); ASSERT_EQ(k, 0); k = waitpid(-1, &s, 0); ASSERT_EQ(k, pid);