Re: [PATCH] exec: do_close_on_exec() before taking exec_update_lock

[Jan Kara]

On Mon 07-09-26 23:26:32, Jann Horn wrote:
> do_close_on_exec() currently happens while holding the exec_update_lock,
> which is used in a lot of places that access process state to
> synchronize access checks.
> I recently added another such use of exec_update_lock, causing a
> regression.
> 
> do_close_on_exec() can block waiting for a reply from a filesystem.
> That means a hung filesystem can block codepaths that use
> exec_update_lock; and it also means that a FUSE filesystem which
> attempts to inspect the calling process can deadlock.
> 
> To avoid such problems, move do_close_on_exec() before the
> exec_update_lock is taken, but after the FD table has been copied if
> necessary.
> 
> I have looked through all the calls between the old and new position of
> the do_close_on_exec() call; there seems to be no file descriptor table
> access in between.
> 
> Reported-by: Benjamin Peterson <benjamin@xxxxxxxxxxx>
> Closes: https://lore.kernel.org/r/f5e8166a-88be-46c5-8939-1e5227ffe4c2@xxxxxxxxxxxxxxxx
> Fixes: 6650527444da ("proc: protect ptrace_may_access() with exec_update_lock (part 1)")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/exec.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 745f6eb5279e..b51e5d7e4536 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1164,6 +1164,20 @@ int begin_new_exec(struct linux_binprm * bprm)
>  	if (retval)
>  		goto out;
>  
> +	/*
> +	 * We have to apply CLOEXEC before we change whether the process is
> +	 * dumpable (in setup_new_exec) to avoid a race with a process in userspace
> +	 * trying to access the should-be-closed file descriptors of a process
> +	 * undergoing exec(2).
> +	 *
> +	 * This can block on filesystem ->flush() handlers, including waiting
> +	 * for FUSE daemons, so do it before exec_mmap takes the
> +	 * exec_update_lock.
> +	 * This must happen after the point of no return, and after unsharing
> +	 * the FD table.
> +	 */
> +	do_close_on_exec(me->files);
> +
>  	/*
>  	 * Must be called _before_ exec_mmap() as bprm->mm is
>  	 * not visible until then. Doing it here also ensures
> @@ -1214,14 +1228,6 @@ int begin_new_exec(struct linux_binprm * bprm)
>  
>  	clear_syscall_work_syscall_user_dispatch(me);
>  
> -	/*
> -	 * We have to apply CLOEXEC before we change whether the process is
> -	 * dumpable (in setup_new_exec) to avoid a race with a process in userspace
> -	 * trying to access the should-be-closed file descriptors of a process
> -	 * undergoing exec(2).
> -	 */
> -	do_close_on_exec(me->files);
> -
>  	if (bprm->secureexec) {
>  		/* Make sure parent cannot signal privileged process. */
>  		me->pdeath_signal = 0;
> 
> ---
> base-commit: 73ae59e975966d24e32926247ddb45a537ebe184
> change-id: 20260907-cloexec-before-exec-update-lock-972ad108510c
> 
> Best regards,
> --  
> Jann Horn <jannh@xxxxxxxxxx>
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR