lkml.org 
[lkml]   [2020]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v6 15/16] exec: Fix dead-lock in de_thread with ptrace_attach
Bernd Edlinger <bernd.edlinger@hotmail.de> writes:

> This removes the last users of cred_guard_mutex
> and replaces it with a new mutex exec_guard_mutex,
> and a boolean unsafe_execve_in_progress.
>
> This addresses the case when at least one of the
> sibling threads is traced, and therefore the trace
> process may dead-lock in ptrace_attach, but de_thread
> will need to wait for the tracer to continue execution.
>
> The solution is to detect this situation and make
> ptrace_attach and similar functions return -EAGAIN,
> but only in a situation where a dead-lock is imminent.
>
> This means this is an API change, but only when the
> process is traced while execve happens in a
> multi-threaded application.
>
> See tools/testing/selftests/ptrace/vmaccess.c
> for a test case that gets fixed by this change.

Hmm. The logic with unsafe_execve_in_progress is interesting.
I think I see what you are aiming for.

So far as you have hit what you are aiming for I think this is
a safe change as the only cases that will change are the cases
that would deadlock today.

At a minimum the code is subtle and I don't see big fat
warning comments that subtle code needs to keep people
from using it wrong.

Further while the change below to proc_pid_attr_write looks
like it is being treated the same as ptrace_attach. When in
fact proc_pid_attr_write needs the no_new_privs and ptrace_attach
protection the same as exec. As the updated cred won't be used in an
ongoing exec, exec does not need protection from proc_pid_attr_write,
other than deadlock protection.

Having the relevant lock be per task_struct lock would probably be a
better way to avoid deadlock with a concurrent proc_pid_attr_write.


So I am going to pass on these last two patches for now, and apply the
rest and get them into linux-next.

Eric


> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6b13fc4..a428536 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2680,14 +2680,17 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
> }
>
> /* Guard against adverse ptrace interaction */
> - rv = mutex_lock_interruptible(&current->signal->cred_guard_mutex);
> + rv = mutex_lock_interruptible(&current->signal->exec_guard_mutex);
> if (rv < 0)
> goto out_free;
>
> - rv = security_setprocattr(PROC_I(inode)->op.lsm,
> - file->f_path.dentry->d_name.name, page,
> - count);
> - mutex_unlock(&current->signal->cred_guard_mutex);
> + if (unlikely(current->signal->unsafe_execve_in_progress))
> + rv = -EAGAIN;
> + else
> + rv = security_setprocattr(PROC_I(inode)->op.lsm,
> + file->f_path.dentry->d_name.name,
> + page, count);
> + mutex_unlock(&current->signal->exec_guard_mutex);
> out_free:
> kfree(page);
> out:

\
 
 \ /
  Last update: 2020-03-25 15:30    [W:0.056 / U:27.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site