Re: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held

[Eric W. Biederman]

Oleg Nesterov <oleg@xxxxxxxxxx> writes:

> de_thread() waits for other threads with ->cred_guard_mutex held and this
> is really bad because the time is not bounded, debugger can delay the exit
> and this lock has a lot of users (mostly abusers imo) in fs/proc and more.
> And this leads to deadlock if debugger tries to take the same mutex:

Oleg.  I looked at the history in proc of users of cred_guard_mutex
and the proc users are grabbing cred_guard_mutex for the proper
semantic reasons.  To avoid races with setuid exec that could result in
an information disclosure.

I do agree that a mutex is the wrong data structure for the job
cred_guard_mutex is performing.  The job of sorting ensuring debuggers
and proc processes see either the old version or the new version of the
task.

I need to play with the code but I suspect the best we can handle this
preventing both security issues and problems in the future is to create
a new task struct and populate it appropriate with the new data from
exec (at least in the case of setuid exec).

I am thinking of generalizing the case of a non-leader exec where we
have to assume the leaders pid.

I don't yet know what the performance implications would be but
that would clean the users up a lot.

On that score I believe we can incrementally approach that point and
only grab the cred_guard_mutex in exec if we are performing an exec that
changes the processes credentials.

Right now I don't think it introduces any new security information
disclosures but the moving of flush_signal_handlers outside of
cred_guard_mutex feels wrong.

Eric