Re: [PATCH v2 4/5] exec: Move exec_mmap right after de_thread in flush_old_exec

[Eric W. Biederman]

Kees Cook <keescook@xxxxxxxxxxxx> writes:

> On Sun, Mar 08, 2020 at 04:38:00PM -0500, Eric W. Biederman wrote:
>> 
>> I have read through the code in exec_mmap and I do not see anything
>> that depends on sighand or the sighand lock, or on signals in anyway
>> so this should be safe.
>> 
>> This rearrangement of code has two siginficant benefits.  It makes
>> the determination of passing the point of no return by testing bprm->mm
>> accurate.  All failures prior to that point in flush_old_exec are
>> either truly recoverable or they are fatal.
>
> Agreed. Though I see a use of "current", which maybe you want to
> parameterize to a "me" argument in acct_arg_size(). (Though looking at
> the callers, perhaps there is no benefit?)

My testing suggests there is a small benefit on x86.

The code is just "#define current get_current()"
and get_current() revoles into a read of "%gs:current_task".

But looking at the code I find gcc can sometimes when the
reads are close in the source code can optimize the read
away.  But gcc does not manage to optimize the extra
read of "%gs:current_task" away.

So I think things are much much better than they used to be,
code generation wise.  But it still helps to cache current
in a local variable.

>> Futher this consolidates all of the possible indefinite waits for
>> userspace together at the top of flush_old_exec.  The possible wait
>> for a ptracer on PTRACE_EVENT_EXIT, the possible wait for a page fault
>> to be resolved in clear_child_tid, and the possible wait for a page
>> fault in exit_robust_list.
>> 
>> This consolidation allows the creation of a mutex to replace
>> cred_guard_mutex that is not held of possible indefinite userspace
>> waits.  Which will allow removing deadlock scenarios from the kernel.
>> 
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
>> ---
>>  fs/exec.c | 24 ++++++++++++------------
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>> 
>> diff --git a/fs/exec.c b/fs/exec.c
>> index 215d86f77b63..d820a7272a76 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1272,18 +1272,6 @@ int flush_old_exec(struct linux_binprm * bprm)
>>  	if (retval)
>>  		goto out;
>>  
>> -#ifdef CONFIG_POSIX_TIMERS
>> -	exit_itimers(me->signal);
>> -	flush_itimer_signals();
>> -#endif
>
> I think this comment:
>
> /*
>  * This is called by do_exit or de_thread, only when there are no more
>  * references to the shared signal_struct.
>  */
> void exit_itimers(struct signal_struct *sig)
>
> Refers to there being other threads, yes? Not that the signal table is
> private yet?

The signal table is in sighand_struct.

So yes that refers to the other threads being gone.


>> -
>> -	/*
>> -	 * Make the signal table private.
>> -	 */
>> -	retval = unshare_sighand(me);
>> -	if (retval)
>> -		goto out;
>> -
>>  	/*
>>  	 * Must be called _before_ exec_mmap() as bprm->mm is
>>  	 * not visibile until then. This also enables the update
>> @@ -1307,6 +1295,18 @@ int flush_old_exec(struct linux_binprm * bprm)
>>  	 */
>>  	bprm->mm = NULL;
>>  
>> +#ifdef CONFIG_POSIX_TIMERS
>> +	exit_itimers(me->signal);
>> +	flush_itimer_signals();
>> +#endif
>
> I've mostly convinced myself that there are no "side-effects" from having
> these timers expire as the mm is going away. I think some kind of comment
> of that intent should be explicitly stated here above the timer work.

The timers can at most generate signals.  And we are not handling
signals in the middle of exec.

So the only possible interaction would be to set a timeout and then try
exec, and have the timer kill the caller.

Maybe we get a killable signal from a scenario like that and maybe this
changes the time before the timer expires into the dangerous zone.
But that is all I can think of.

We have to return to the edge of userspace before any signals are
delivered.


> Beyond that:
>
> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
>
> -Kees
>
>> +
>> +	/*
>> +	 * Make the signal table private.
>> +	 */
>> +	retval = unshare_sighand(me);
>> +	if (retval)
>> +		goto out;
>> +
>>  	set_fs(USER_DS);
>>  	me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD |
>>  					PF_NOFREEZE | PF_NO_SETAFFINITY);
>> -- 
>> 2.25.0
>> 

Eric