diff mbox series

exec: Add comments on check_unsafe_exec() fs counting

Message ID 20221018071537.never.662-kees@kernel.org (mailing list archive)
State Mainlined
Commit 275498a98b1fe77deebddfc4f8986c0cf2c3ced7
Headers show
Series exec: Add comments on check_unsafe_exec() fs counting | expand

Commit Message

Kees Cook Oct. 18, 2022, 7:17 a.m. UTC 
Add some comments about what the fs counting is doing in
check_unsafe_exec() and how it relates to the call graph.
Specifically, we can't force an unshare of the fs because
of at least Chrome:
https://lore.kernel.org/lkml/86CE201B-5632-4BB7-BCF6-7CB2C2895409@chromium.org/

Cc: Eric Biederman <ebiederm@xmission.com>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/exec.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Christian Brauner Oct. 19, 2022, 11:35 a.m. UTC | #1 
On Tue, Oct 18, 2022 at 12:17:24AM -0700, Kees Cook wrote:
> Add some comments about what the fs counting is doing in
> check_unsafe_exec() and how it relates to the call graph.
> Specifically, we can't force an unshare of the fs because
> of at least Chrome:
> https://lore.kernel.org/lkml/86CE201B-5632-4BB7-BCF6-7CB2C2895409@chromium.org/
> 
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>
diff mbox series

Patch

diff --git a/fs/exec.c b/fs/exec.c
index 902bce45b116..01659c2ac7d8 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1571,6 +1571,12 @@  static void check_unsafe_exec(struct linux_binprm *bprm)
 	if (task_no_new_privs(current))
 		bprm->unsafe |= LSM_UNSAFE_NO_NEW_PRIVS;
 
+	/*
+	 * If another task is sharing our fs, we cannot safely
+	 * suid exec because the differently privileged task
+	 * will be able to manipulate the current directory, etc.
+	 * It would be nice to force an unshare instead...
+	 */
 	t = p;
 	n_fs = 1;
 	spin_lock(&p->fs->lock);
@@ -1752,6 +1758,7 @@  static int search_binary_handler(struct linux_binprm *bprm)
 	return retval;
 }
 
+/* binfmt handlers will call back into begin_new_exec() on success. */
 static int exec_binprm(struct linux_binprm *bprm)
 {
 	pid_t old_pid, old_vpid;
@@ -1810,6 +1817,11 @@  static int bprm_execve(struct linux_binprm *bprm,
 	if (retval)
 		return retval;
 
+	/*
+	 * Check for unsafe execution states before exec_binprm(), which
+	 * will call back into begin_new_exec(), into bprm_creds_from_file(),
+	 * where setuid-ness is evaluated.
+	 */
 	check_unsafe_exec(bprm);
 	current->in_execve = 1;