[CRIU] [PATCH v2 01/24] exec: Move unshare_files to fix posix file locking during exec

[Eric W. Biederman]

Al Viro <viro at zeniv.linux.org.uk> writes:

> On Fri, Nov 20, 2020 at 05:14:18PM -0600, Eric W. Biederman wrote:
>> @@ -1805,8 +1808,12 @@ static int bprm_execve(struct linux_binprm *bprm,
>>  	bprm->file = file;
>>  	/*
>>  	 * Record that a name derived from an O_CLOEXEC fd will be
>> -	 * inaccessible after exec. Relies on having exclusive access to
>> -	 * current->files (due to unshare_files above).
>> +	 * inaccessible after exec.  This allows the code in exec to
>> +	 * choose to fail when the executable is not mmaped into the
>> +	 * interpreter and an open file descriptor is not passed to
>> +	 * the interpreter.  This makes for a better user experience
>> +	 * than having the interpreter start and then immediately fail
>> +	 * when it finds the executable is inaccessible.
>>  	 */
>>  	if (bprm->fdpath &&
>>  	    close_on_exec(fd, rcu_dereference_raw(current->files->fdt)))
>
> We do not have rcu_read_lock() here.  What would happen if
> 	* we get fdt
> 	* another thread does e.g. dup2() with target descriptor greater than
> the current size
> 	* old fdt gets copied and (RCU-delayed) freed
> 	* nobody is holding rcu_read_lock(), so it gets really freed
> 	* we read a bitmap from the already freed sucker
>
> It's a narrow window, but on SMP KVM it's not impossible to hit; if you
> have preemption enabled, the race window is not so narrow even when
> running on bare metal.  In the mainline it is safe, but only because
> the damn thing is guaranteed to be _not_ shared with any other thread
> (which is what the comment had been about).  Why not simply say
> 	if (bprm->fdpath && get_close_on_exec(fd))
> anyway?

My mistake.  I missed that the actual code was highly optimized and only
safe in the presence of an unshared files struct.

What I saw and what I thought the comment was talking about was that the
result of the test isn't guaranteed to be stable without having an
exclusive access to the files.  I figured and if something changes later
and it becomes safe to pass the file name to a script later we don't
really care.

In any event thank you for the review.  I will queue up a follow on
patch that makes this use get_close_on_exec.

Eric