[01/17] exec: Move unshare_files to fix posix file locking during exec

Commit Message

Eric W. Biederman Aug. 17, 2020, 10:04 p.m. UTC

Many moons ago the binfmts were doing some very questionable things
with file descriptors and an unsharing of the file descriptor table
was added to make things better[1][2].  The helper steal_files was
added to avoid breaking the userspace programs[3][4][6].

Unfortunately it turned out that steal_locks did not work for network
file systems[5], so it was removed to see if anyone would
complain[7][8].  It was thought at the time that NPTL would not be
affected as the unshare_files happened after the other threads were
killed[8].  Unfortunately because there was an unshare_files in
binfmt_elf.c before the threads were killed this analysis was
incorrect.

This unshare_files in binfmt_elf.c resulted in the unshares_files
happening whenever threads were present.  Which led to unshare_files
being moved to the start of do_execve[9].

Later the problems were rediscovered and suggested approach was to
readd steal_locks under a different name[10].  I happened to be
reviewing patches and I noticed that this approach was a step
backwards[11].

I proposed simply moving unshare_files[12] and it was pointed
out that moving unshare_files without auditing the code was
also unsafe[13].

There were then several attempts to solve this[14][15][16] and I even
posted this set of changes[17].  Unfortunately because auditing all of
execve is time consuming this change did not make it in at the time.

Well now that I am cleaning up exec I have made the time to read
through all of the binfmts and the only playing with file descriptors
is either the security modules closing them in
security_bprm_committing_creds or is in the generic code in fs/exec.c.
None of it happens before begin_new_exec is called.

So move unshare_files into begin_new_exec, after the point of no
return.  If memory is very very very low and the application calling
exec is sharing file descriptor tables between processes we might fail
past the point of no return.  Which is unfortunate but no different
than any of the other places where we allocate memory after the point
of no return.

This movement allows another process that shares the file table, or
another thread of the same process and that closes files or changes
their close on exec behavior and races with execve to cause some
unexpected things to happen.  There is only one time of check to time
of use race and it is just there so that execve fails instead of
an interpreter failing when it tries to open the file it is supposed
to be interpreting.   Failing later if userspace is being silly is
not a problem.

With this change it the following discription from the removal
of steal_locks[8] finally becomes true.

    Apps using NPTL are not affected, since all other threads are killed before
    execve.

    Apps using LinuxThreads are only affected if they

      - have multiple threads during exec (LinuxThreads doesn't kill other
        threads, the app may do it with pthread_kill_other_threads_np())
      - rely on POSIX locks being inherited across exec

    Both conditions are documented, but not their interaction.

    Apps using clone() natively are affected if they

      - use clone(CLONE_FILES)
      - rely on POSIX locks being inherited across exec

I have investigated some paths to make it possible to solve this
without moving unshare_files but they all look more complicated[18].

Reported-by: Daniel P. Berrangé <[e-mail]>
Reported-by: Jeff Layton <[e-mail]>
History-tree: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
[1] 02cda956de0b ("[PATCH] unshare_files"
[2] 04e9bcb4d106 ("[PATCH] use new unshare_files helper")
[3] 088f5d7244de ("[PATCH] add steal_locks helper")
[4] 02c541ec8ffa ("[PATCH] use new steal_locks helper")
[5] https://lkml.kernel.org/r/[e-mail]
[6] https://lkml.kernel.org/r/[e-mail]
[7] https://lkml.kernel.org/r/[e-mail]
[8] c89681ed7d0e ("[PATCH] remove steal_locks()")
[9] fd8328be874f ("[PATCH] sanitize handling of shared descriptor tables in failing execve()")
[10] https://lkml.kernel.org/r/[e-mail]
[11] https://lkml.kernel.org/r/[e-mail]
[12] https://lkml.kernel.org/r/[e-mail]
[13] https://lkml.kernel.org/r/[e-mail]
[14] https://lkml.kernel.org/r/[e-mail]
[15] https://lkml.kernel.org/r/[e-mail]
[16] https://lkml.kernel.org/r/[e-mail]
[17] https://lkml.kernel.org/r/[e-mail]
[18] https://lkml.kernel.org/r/[e-mail]
Signed-off-by: "Eric W. Biederman" <[e-mail]>
---
 fs/exec.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

Comments

Christian Brauner Aug. 18, 2020, 10:04 a.m. UTC | #1

On Mon, Aug 17, 2020 at 05:04:09PM -0500, Eric W. Biederman wrote:
> Many moons ago the binfmts were doing some very questionable things
> with file descriptors and an unsharing of the file descriptor table
> was added to make things better[1][2].  The helper steal_files was
> added to avoid breaking the userspace programs[3][4][6].
> 
> Unfortunately it turned out that steal_locks did not work for network
> file systems[5], so it was removed to see if anyone would
> complain[7][8].  It was thought at the time that NPTL would not be
> affected as the unshare_files happened after the other threads were
> killed[8].  Unfortunately because there was an unshare_files in
> binfmt_elf.c before the threads were killed this analysis was
> incorrect.
> 
> This unshare_files in binfmt_elf.c resulted in the unshares_files
> happening whenever threads were present.  Which led to unshare_files
> being moved to the start of do_execve[9].
> 
> Later the problems were rediscovered and suggested approach was to
> readd steal_locks under a different name[10].  I happened to be
> reviewing patches and I noticed that this approach was a step
> backwards[11].
> 
> I proposed simply moving unshare_files[12] and it was pointed
> out that moving unshare_files without auditing the code was
> also unsafe[13].
> 
> There were then several attempts to solve this[14][15][16] and I even
> posted this set of changes[17].  Unfortunately because auditing all of
> execve is time consuming this change did not make it in at the time.
> 
> Well now that I am cleaning up exec I have made the time to read
> through all of the binfmts and the only playing with file descriptors
> is either the security modules closing them in
> security_bprm_committing_creds or is in the generic code in fs/exec.c.
> None of it happens before begin_new_exec is called.
> 
> So move unshare_files into begin_new_exec, after the point of no
> return.  If memory is very very very low and the application calling
> exec is sharing file descriptor tables between processes we might fail
> past the point of no return.  Which is unfortunate but no different
> than any of the other places where we allocate memory after the point
> of no return.
> 
> This movement allows another process that shares the file table, or
> another thread of the same process and that closes files or changes
> their close on exec behavior and races with execve to cause some
> unexpected things to happen.  There is only one time of check to time

It seems to only make the already existing race window wider by moving
it from bprm_execve() to begin_new_exec() which isn't great but probably
ok since done for a good reason.

> of use race and it is just there so that execve fails instead of
> an interpreter failing when it tries to open the file it is supposed
> to be interpreting.   Failing later if userspace is being silly is
> not a problem.
> 
> With this change it the following discription from the removal
> of steal_locks[8] finally becomes true.
> 
>     Apps using NPTL are not affected, since all other threads are killed before
>     execve.
> 
>     Apps using LinuxThreads are only affected if they
> 
>       - have multiple threads during exec (LinuxThreads doesn't kill other
>         threads, the app may do it with pthread_kill_other_threads_np())
>       - rely on POSIX locks being inherited across exec
> 
>     Both conditions are documented, but not their interaction.
> 
>     Apps using clone() natively are affected if they
> 
>       - use clone(CLONE_FILES)
>       - rely on POSIX locks being inherited across exec
> 
> I have investigated some paths to make it possible to solve this
> without moving unshare_files but they all look more complicated[18].
> 
> Reported-by: Daniel P. Berrangé <[e-mail]>
> Reported-by: Jeff Layton <[e-mail]>
> History-tree: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> [1] 02cda956de0b ("[PATCH] unshare_files"
> [2] 04e9bcb4d106 ("[PATCH] use new unshare_files helper")
> [3] 088f5d7244de ("[PATCH] add steal_locks helper")
> [4] 02c541ec8ffa ("[PATCH] use new steal_locks helper")
> [5] https://lkml.kernel.org/r/[e-mail]
> [6] https://lkml.kernel.org/r/[e-mail]
> [7] https://lkml.kernel.org/r/[e-mail]
> [8] c89681ed7d0e ("[PATCH] remove steal_locks()")
> [9] fd8328be874f ("[PATCH] sanitize handling of shared descriptor tables in failing execve()")
> [10] https://lkml.kernel.org/r/[e-mail]
> [11] https://lkml.kernel.org/r/[e-mail]
> [12] https://lkml.kernel.org/r/[e-mail]
> [13] https://lkml.kernel.org/r/[e-mail]
> [14] https://lkml.kernel.org/r/[e-mail]
> [15] https://lkml.kernel.org/r/[e-mail]
> [16] https://lkml.kernel.org/r/[e-mail]
> [17] https://lkml.kernel.org/r/[e-mail]
> [18] https://lkml.kernel.org/r/[e-mail]
> Signed-off-by: "Eric W. Biederman" <[e-mail]>
> ---

Slightly scary change but it solves a problem.
Acked-by: Christian Brauner <[e-mail]>

Patch

diff --git a/fs/exec.c b/fs/exec.c
index a91003e28eaa..17c007bba712 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1354,6 +1354,7 @@  void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
 int begin_new_exec(struct linux_binprm * bprm)
 {
 	struct task_struct *me = current;
+	struct files_struct *displaced;
 	int retval;
 
 	/* Once we are committed compute the creds */
@@ -1373,6 +1374,13 @@  int begin_new_exec(struct linux_binprm * bprm)
 	if (retval)
 		goto out;
 
+	/* Ensure the files table is not shared. */
+	retval = unshare_files(&displaced);
+	if (retval)
+		goto out;
+	if (displaced)
+		put_files_struct(displaced);
+
 	/*
 	 * Must be called _before_ exec_mmap() as bprm->mm is
 	 * not visibile until then. This also enables the update
@@ -1892,16 +1900,11 @@  static int bprm_execve(struct linux_binprm *bprm,
 		       int fd, struct filename *filename, int flags)
 {
 	struct file *file;
-	struct files_struct *displaced;
 	int retval;
 
-	retval = unshare_files(&displaced);
-	if (retval)
-		return retval;
-
 	retval = prepare_bprm_creds(bprm);
 	if (retval)
-		goto out_files;
+		return retval;
 
 	check_unsafe_exec(bprm);
 	current->in_execve = 1;
@@ -1916,8 +1919,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)))
@@ -1938,8 +1945,6 @@  static int bprm_execve(struct linux_binprm *bprm,
 	rseq_execve(current);
 	acct_update_integrals(current);
 	task_numa_free(current, false);
-	if (displaced)
-		put_files_struct(displaced);
 	return retval;
 
 out:
@@ -1956,10 +1961,6 @@  static int bprm_execve(struct linux_binprm *bprm,
 	current->fs->in_exec = 0;
 	current->in_execve = 0;
 
-out_files:
-	if (displaced)
-		reset_files_struct(displaced);
-
 	return retval;
 }