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

["Eric W. Biederman" <ebiederm@xmission.com>]

Jann Horn <jannh@google.com> writes:

> 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.

A small nit.

It has always been a requirement that in exec_update_lock not be held
for writing over any userspace accesses.  Which is why it is taken in
right after exec_mmap is done updating userspace.

In the original version I missed that do_close_on_exec calls flush
which can block waiting on userspace (Is that just a fuse thing?).

So unless I am mistaken I don't think your change technically
created a new bug, so much as aggravated an existing bug.  It was
definitely a regression in user experience.

I am mentioning this just to make it clear what is going on.

> 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.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

> Reported-by: Benjamin Peterson <benjamin@locrian.net>
> Closes: https://lore.kernel.org/r/f5e8166a-88be-46c5-8939-1e5227ffe4c2@app.fastmail.com
> Fixes: 6650527444da ("proc: protect ptrace_may_access() with exec_update_lock (part 1)")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  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@google.com>