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

[Al Viro]

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?