linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@redhat.com>
To: syzbot <syzbot+1c486d0b62032c82a968@syzkaller.appspotmail.com>,
	brauner@kernel.org, kees@kernel.org, viro@zeniv.linux.org.uk
Cc: jack@suse.cz, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	syzkaller-bugs@googlegroups.com, mjguzik@gmail.com
Subject: [PATCH] exec: fix the racy usage of fs_struct->in_exec
Date: Mon, 24 Mar 2025 17:00:03 +0100	[thread overview]
Message-ID: <20250324160003.GA8878@redhat.com> (raw)
In-Reply-To: <67dc67f0.050a0220.25ae54.001f.GAE@google.com>

check_unsafe_exec() sets fs->in_exec under cred_guard_mutex, then execve()
paths clear fs->in_exec lockless. This is fine if exec succeeds, but if it
fails we have the following race:

	T1 sets fs->in_exec = 1, fails, drops cred_guard_mutex

	T2 sets fs->in_exec = 1

	T1 clears fs->in_exec

	T2 continues with fs->in_exec == 0

Change fs/exec.c to clear fs->in_exec with cred_guard_mutex held.

Reported-by: syzbot+1c486d0b62032c82a968@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/67dc67f0.050a0220.25ae54.001f.GAE@google.com/
Cc: stable@vger.kernel.org
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/exec.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 506cd411f4ac..17047210be46 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1229,13 +1229,12 @@ int begin_new_exec(struct linux_binprm * bprm)
 	 */
 	bprm->point_of_no_return = true;
 
-	/*
-	 * Make this the only thread in the thread group.
-	 */
+	/* Make this the only thread in the thread group */
 	retval = de_thread(me);
 	if (retval)
 		goto out;
-
+	/* see the comment in check_unsafe_exec() */
+	current->fs->in_exec = 0;
 	/*
 	 * Cancel any io_uring activity across execve
 	 */
@@ -1497,6 +1496,8 @@ static void free_bprm(struct linux_binprm *bprm)
 	}
 	free_arg_pages(bprm);
 	if (bprm->cred) {
+		/* in case exec fails before de_thread() succeeds */
+		current->fs->in_exec = 0;
 		mutex_unlock(&current->signal->cred_guard_mutex);
 		abort_creds(bprm->cred);
 	}
@@ -1618,6 +1619,10 @@ static void check_unsafe_exec(struct linux_binprm *bprm)
 	 * 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...
+	 *
+	 * Otherwise we set fs->in_exec = 1 to deny clone(CLONE_FS)
+	 * from another sub-thread until de_thread() succeeds, this
+	 * state is protected by cred_guard_mutex we hold.
 	 */
 	n_fs = 1;
 	spin_lock(&p->fs->lock);
@@ -1862,7 +1867,6 @@ static int bprm_execve(struct linux_binprm *bprm)
 
 	sched_mm_cid_after_execve(current);
 	/* execve succeeded */
-	current->fs->in_exec = 0;
 	current->in_execve = 0;
 	rseq_execve(current);
 	user_events_execve(current);
@@ -1881,7 +1885,6 @@ static int bprm_execve(struct linux_binprm *bprm)
 		force_fatal_sig(SIGSEGV);
 
 	sched_mm_cid_after_execve(current);
-	current->fs->in_exec = 0;
 	current->in_execve = 0;
 
 	return retval;
-- 
2.25.1.362.g51ebf55




  parent reply	other threads:[~2025-03-24 16:00 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-20 19:09 [syzbot] [fs?] [mm?] KCSAN: data-race in bprm_execve / copy_fs (4) syzbot
2025-03-20 20:09 ` Kees Cook
2025-03-21  1:44   ` Al Viro
2025-03-21  8:10     ` Kees Cook
2025-03-21  8:49       ` Christian Brauner
2025-03-21  8:45   ` Christian Brauner
2025-03-22  1:00     ` Al Viro
2025-03-22  6:26       ` Kees Cook
2025-03-22 10:15         ` Mateusz Guzik
2025-03-22 10:28           ` Christian Brauner
2025-03-22 10:23       ` Christian Brauner
2025-03-22 15:55       ` Oleg Nesterov
2025-03-22 18:50         ` Al Viro
2025-03-23 18:14           ` Oleg Nesterov
2025-03-23 20:57             ` Christian Brauner
2025-03-24 16:00 ` Oleg Nesterov [this message]
2025-03-24 17:01   ` [PATCH] exec: fix the racy usage of fs_struct->in_exec Mateusz Guzik
2025-03-24 18:27     ` Oleg Nesterov
2025-03-24 18:37       ` Oleg Nesterov
2025-03-24 22:24       ` Mateusz Guzik
2025-03-25 10:09         ` Oleg Nesterov
2025-03-25 11:01           ` Mateusz Guzik
2025-03-25 13:21             ` Oleg Nesterov
2025-03-25 13:30               ` Christian Brauner
2025-03-25 14:15                 ` Mateusz Guzik
2025-03-25 14:46                   ` Christian Brauner
2025-03-25 18:40                     ` Kees Cook
2025-04-29 15:49   ` Oleg Nesterov
2025-04-29 16:57     ` Kees Cook
2025-04-29 17:12     ` Mateusz Guzik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250324160003.GA8878@redhat.com \
    --to=oleg@redhat.com \
    --cc=brauner@kernel.org \
    --cc=jack@suse.cz \
    --cc=kees@kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mjguzik@gmail.com \
    --cc=syzbot+1c486d0b62032c82a968@syzkaller.appspotmail.com \
    --cc=syzkaller-bugs@googlegroups.com \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox