ebiederm@xxxxxxxxxxxx (Eric W. Biederman) writes: > Bernd Edlinger <bernd.edlinger@xxxxxxxxxx> writes: > >> On 3/6/20 6:17 AM, Eric W. Biederman wrote: >>> Bernd Edlinger <bernd.edlinger@xxxxxxxxxx> writes: >>> >>>> On 3/5/20 10:16 PM, Eric W. Biederman wrote: >>>>> >>>>> The cred_guard_mutex is problematic. The cred_guard_mutex is held >>>>> over the userspace accesses as the arguments from userspace are read. >>>>> The cred_guard_mutex is held of PTRACE_EVENT_EXIT as the the other >>>>> threads are killed. The cred_guard_mutex is held over >>>>> "put_user(0, tsk->clear_child_tid)" in exit_mm(). >>>>> >> >> I am all for this patch, and the direction it is heading, Eric. >> >> I just wanted to add a note that I think it is >> possible that exec_mm_release can also invoke put_user(0, tsk->clear_child_tid), >> under the new exec_update_mutex, since vm_access increments the >> mm->mm_users, under the cred_update_mutex, but releases the mutex, >> and the caller can hold the reference for a while and then exec_mmap is not >> releasing the last reference. > > Good catch. I really appreciate your close look at the details. > > I am wondering if process_vm_readv and process_vm_writev could be > safely changed to use mmgrab and mmdrop, instead of mmget and mmput. > > That would resolve the potential issue you have pointed out. I just > haven't figured out if it is safe. The mm code has been seriously > refactored since I knew how it all worked. Nope, mmget can not be replaced by mmgrab. It might be possible to do something creative like store a cred in place of the userns on the mm and use that for mm_access permission checks. Still we are talking a pretty narrow window, and a case that no one has figured out how to trigger yet. So I will leave that corner case as something for future improvements. Eric
