On Wed, Nov 02, 2016 at 07:18:06PM +0100, Oleg Nesterov wrote: > On 10/30, Jann Horn wrote: > > > > This is a new per-threadgroup lock that can often be taken instead of > > cred_guard_mutex and has less deadlock potential. I'm doing this because > > Oleg Nesterov mentioned the potential for deadlocks, in particular if a > > debugged task is stuck in execve, trying to get rid of a ptrace-stopped > > thread, and the debugger attempts to inspect procfs files of the debugged > > task. > > Yes, but let me repeat that we need to fix this anyway. So I don't really > understand why should we add yet another mutex. execve() only takes the new mutex immediately after de_thread(), so this problem shouldn't occur there. Basically, I think that I'm not making the problem worse with my patches this way. I believe that it should be possible to convert most existing users of the cred_guard_mutex to the new cred_guard_light - exceptions to that that I see are: - PTRACE_ATTACH - SECCOMP_FILTER_FLAG_TSYNC (sets NO_NEW_PRIVS on remote task) Beyond that, conceptually, the new cred_guard_light could also be turned into a read-write mutex to prevent deadlocks between its users (where execve would take it for writing and everyone else would take it for reading), but afaik the kernel doesn't have an implementation of read-write mutexes yet? cred_guard_light would mean that you could theoretically still create deadlocks, but afaics only if you do things like trying to read /proc/$pid/mem in the FUSE read handler for the file that is currently being executed - and in that case, I think it's okay to have a killable deadlock. Do you think that, if (apart from execve) only PTRACE_ATTACH and SECCOMP_FILTER_FLAG_TSYNC remain as users of cred_guard_mutex and everything else used my new cred_guard_light, that would be sufficient to fix the races you are concerned about? It seems to me like SECCOMP_FILTER_FLAG_TSYNC doesn't really have deadlocking issues. PTRACE_ATTACH isn't that clear to me; if a debugger tries to attach to a newly spawned thread while another ptraced thread is dying because of de_thread() in a third thread, that might still cause the debugger to deadlock, right? The problem with PTRACE_ATTACH is basically that bprm->unsafe is used in the bprm_set_creds LSM hook, so it needs to have been calculated when that hook is executed. (Also in the bprm_secureexec hook, but that one happens after install_exec_creds(), so that's unproblematic.) security_bprm_set_creds() is called in prepare_binprm(), which is executed very early in do_execveat_common(), at a point where failures should still be graceful (return an error code instead of killing the whole process), and therefore other threads can still run and debuggers can still attach. The LSM hooks that execute at that point e.g. inspect and modify bprm->cred, and they can still cleanly prohibit execution. E.g. SELinux does this - it can cancel execution with errors like -EPERM and -EACCES. AFAICS the hard case is: - Multithreaded process with tasks A and B is running. - Task C attaches to B via ptrace. - Task A calls execve(), takes the mutex, reaches de_thread(), kills task B. - Task C tries to attach to A, tries to take the mutex again, deadlock. I'm not sure whether it'd be possible to get rid of the deadlock for PTRACE_ATTACH without ABI changes, and I would be surprised if it was doable without nontrivial additional logic.
Attachment:
signature.asc
Description: Digital signature
