On 3/25/20 3:27 PM, Eric W. Biederman wrote: > Bernd Edlinger <bernd.edlinger@xxxxxxxxxx> 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(¤t->signal->cred_guard_mutex); > + rv = mutex_lock_interruptible(¤t->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(¤t->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(¤t->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.
