Re: [PATCH v2 5/5] exec: Add a exec_update_mutex to replace cred_guard_mutex

Eric W. Biederman
2020-03-10T21:05:11Z

Kees Cook <keescook@chromium.org> writes:

> On Mon, Mar 09, 2020 at 02:02:37PM -0500, Eric W. Biederman wrote:
>> exec: Add exec_update_mutex to replace cred_guard_mutex
>>
>> The cred_guard_mutex is problematic as it is held over possibly
>> indefinite waits for userspace. The possilbe indefinite waits for
>> userspace that I have identified are: The cred_guard_mutex is held in
>> PTRACE_EVENT_EXIT waiting for the tracer. The cred_guard_mutex is
>> held over "put_user(0, tsk->clear_child_tid)" in exit_mm(). The
>> cred_guard_mutex is held over "get_user(futex_offset, ...") in
>> exit_robust_list. The cred_guard_mutex held over copy_strings.
>
> I suspect you're not trying to make a comprehensive list here, but do
> you want to mention seccomp too (since it's yet another weird case).

I was calling out all of the places I have found so far where
cred_guard_mutex is held over waiting for userspace to maybe do
something. Those places are what cause our deadlocks.

>> [...]
>> Holding a mutex over any of those possibly indefinite waits for
>> userspace does not appear necessary. Add exec_update_mutex that will
>> just cover updating the process during exec where the permissions and
>> the objects pointed to by the task struct may be out of sync.
>
> Should the specific resources be pointed out here? creds, mm, ... ?
>
> But otherwise, yup, looks sane:

Probably not. The design is if exec changes it we will hold the
cred_guard_mutex over it, so things are semi-atomic.

> Reviewed-by: Kees Cook <keescook@chromium.org>

Eric