Re: [PATCH v6 15/16] exec: Fix dead-lock in de_thread with ptrace_attach

On 3/25/20 3:27 PM, Eric W. Biederman wrote:

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.

Okay, I can add big fat warning comments, yeah.

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.

Not sure I understand this comment correct. You refer to this block here:

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

I think the logic is correct, but instead if an if-then-else, I need the big-fat-warning-comment followed by if-unsafe-goto-mutex-unlock kind of thing, so it looks more like the other places, right?

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.

Please elaborate your idea a bit.

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

No problem, I can update this patch and if you like take it to your tree, otherwise it is of course not the most important issue in the world ;-)

Thanks Bernd.