From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 35AFEC3ABA5 for ; Tue, 29 Apr 2025 16:57:31 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3AB636B000E; Tue, 29 Apr 2025 12:57:29 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 35A7C6B0023; Tue, 29 Apr 2025 12:57:29 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 24AE86B0024; Tue, 29 Apr 2025 12:57:29 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 081A96B000E for ; Tue, 29 Apr 2025 12:57:29 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 3A7C71CE2AA for ; Tue, 29 Apr 2025 16:57:30 +0000 (UTC) X-FDA: 83387687460.26.1B46D08 Received: from tor.source.kernel.org (tor.source.kernel.org [172.105.4.254]) by imf10.hostedemail.com (Postfix) with ESMTP id 951F7C000F for ; Tue, 29 Apr 2025 16:57:28 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=HpAewxvW; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf10.hostedemail.com: domain of kees@kernel.org designates 172.105.4.254 as permitted sender) smtp.mailfrom=kees@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1745945848; a=rsa-sha256; cv=none; b=WQv4fFatXQXtrNzUpY7+wV1w5iQy2jH/r0HqSEFAaHwYKkDkPg2v6e3xRCfpd4stRBHqFL qfdgh2SFnJDCbV+/Ak9uPYsJP4IW0QB0h5/k36+2Vf7t/m58Ct+CS5J+1BLKB/KFg2veK+ HHTaOngEJHZcZctwMdGLG5SD/YFnBS4= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=HpAewxvW; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf10.hostedemail.com: domain of kees@kernel.org designates 172.105.4.254 as permitted sender) smtp.mailfrom=kees@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1745945848; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=iz4wTyf5Y7zT0a2ZsXa/YWT3LgfBFUJ/9kFsNOsC5XE=; b=XhKYOPmdZAkOgMFlCFsXL+sElyH4Up0RViqNyuc7lc8ujTkPJlOPjDbXG3lrnrWSRjbKIe Xt//5JIVUJXTsuX+NXauBFq1t+fsUEmFR7Jcz5IzCWjY17Ci+fmtWxvPDRp/usk/oMT8AJ Br6d/IaNS8ZVPy3jJgo6LbHlRYN3hKw= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id E0DEB68436; Tue, 29 Apr 2025 16:57:01 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B87D8C4CEE9; Tue, 29 Apr 2025 16:57:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1745945847; bh=ipRqIkTVDn1dLGetJ67+0tIDZlhJgt4zp5uPL1A7eL4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=HpAewxvWdIt4hTlSxGh2L6xk+QE7EtRvqSTv9+cvpeifDyPXpJHiJ3riJtquKe36w iJclZWXV68HyQtAKqSuZSWufXcTKiQ4+ij3z61TpHnRwmJa/DAwJtS6+W12viqwPwT kiar65ZM7ojINcILfivDzdtHxhhorpQKJU6RYeCgXkDcpaEpCIjWE3ajvCUFro7a36 LlSYSs5WNGLnzvQh843eghvaBfCExnEsGBkHMB/VlJ5t+TVE2D9aVdwtqsU3L+vIiE ofYwnNpyzok+8OXJX6nLW+Zh+6hs91j8qXtMzGIxQFc3WnaDgoq+PEaVtAXflTPwur 0/aBaZA2pYfjw== Date: Tue, 29 Apr 2025 09:57:24 -0700 From: Kees Cook To: Oleg Nesterov Cc: brauner@kernel.org, viro@zeniv.linux.org.uk, jack@suse.cz, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, syzkaller-bugs@googlegroups.com, mjguzik@gmail.com, Michal Hocko Subject: Re: [PATCH] exec: fix the racy usage of fs_struct->in_exec Message-ID: <202504290957.1D6835B89@keescook> References: <67dc67f0.050a0220.25ae54.001f.GAE@google.com> <20250324160003.GA8878@redhat.com> <20250429154944.GA18907@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250429154944.GA18907@redhat.com> X-Rspamd-Queue-Id: 951F7C000F X-Stat-Signature: qrx5q483bp7scs784qszg9zk1ndbuhq9 X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1745945848-767103 X-HE-Meta: U2FsdGVkX19RsyuNgV7ml0K1x7CkI1tE8AutY1+vAKjKNpV9NTU1VZPED12V5Cyrc/cSjd53Yz47l8YBmaYe8L/Ef5Lo2GG6lrRAYMXQ06SnnUl9SzwD5XWWXRgNTdAtXpuhnGKMYx124nKNlCCFReeNBn8uz3RgPFL4tzZ1Xm6oFRjeIj6r+oBaWHKgIWaxQQqEfTisKW72pWWm6ZbUgbk5PtJqzi+ScQeYGsw66kBwDfCPBFAye3ga79RhOHDx8MjRXruw1cHblb424mqqgy4zH3DFoXxFHjChloI3FCv/s1WI8CoNR7yN0R1CUqwkE4XY9EFqZpCIh7MutpMAtLPK6T5JqKIbxaFDxLnNNzdaFhMsUqp5BFr/456HP+xVlZ9om6MzqSM8lp8upXpKqnkHD9Ld4ABBjdQBc/l+pKba4YpBEe1XxJKNIZr4zU23rylFw22dnHZWxUp+FFZx+2AB6wNxM+fx0ZGSDe6Z61e+O6FzrgcHGhnnrqonNesoST48mTcmjZwTFNWpmzp59LPS3B0/8dHqXrzOnriM5HTpv08+Jdl7nt29sbhhu1i168Q2jc6cY4Zwvnqt0kFT0qt3HNGQGEDAz6Y3dDzNQXcjfT4Pw6g9O+ajZEExRyKbNdxXUV+vaLYkzTqo7U19M7BpYaq8ibm1w9164Z23QWTRumrTEzv4WfjYD1S/0yk2MEMUjms13Z9yx/6S0co7z+M4o6jBfvI82SHP9/KgHfixfBeRp8sqn2x5nkhBJAlKbvlw98a2+f1QuqszcW2HIzr4Ju4qULV8mVz8tloIcwO3dOCoAthBZDqBMp7KmyQFB26Ut68CbCrUgvDCXdzsvsuA6iZhUMBQTwXOmHKbVt0Y87V+gL96c2TNjE2Ww1cSmVcmVKo7Arh/l5GbPZwdnZAL8qI5T3+jCoZvrNRGJBKY/Bro3gxQS7UucxWXWzqYji62NGl9Oef8NPLYIo+ 4VlQaaOG fiXWb0aRj76V0Z1WEjg8xcaDEH8fYRSX1JIZlz9XG5wucAbFxIetZP67ufcnY/3I9ZSjb+cHMQ2mWJwV/wYmRMQdu3WMU8yJYskUqzVSQwHKQjufGeWYv4XLsBvh1utlZIeV0h7s/A+ZsSYYhJGuR7aVa04WjweWVhyGXGSSkhC4IljIHHgHFCz7u6OGhlON15HWu8Gfp3A8llAEXpZ/s/AEw6fzz5nkRIT/1vFbK9EQ/hGrC+9TFHkkdVPD53RM+QJzQf7l99OmjZt8ygrwlAiGG/0bcDfakdqD3ZXMJJH4XHyUQhNhu7VPpaBjAfYecTUgNoeZzBfSPHV7EjS/rYm8V0Q== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Tue, Apr 29, 2025 at 05:49:44PM +0200, Oleg Nesterov wrote: > Damn, I am stupid. > > On 03/24, Oleg Nesterov wrote: > > > > 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 > > When I look at this code again, I think this race was not possible and thus > this patch (applied as af7bb0d2ca45) was not needed. > > Yes, begin_new_exec() can drop cred_guard_mutex on failure, but only after > de_thread() succeeds, when we can't race with another sub-thread. > > I hope this patch didn't make the things worse so we don't need to revert it. > Plus I think it makes this (confusing) logic a bit more clear. Just, unless > I am confused again, it wasn't really needed. > > ----------------------------------------------------------------------------- > But. I didn't read the original report from syzbot, > https://lore.kernel.org/all/67dc67f0.050a0220.25ae54.001f.GAE@google.com/#t > because I wasn't CC'ed. and then - sorry Kees!!! - I didn't bother to read > your first reply carefully. > > So yes, with or without this patch the "if (fs->in_exec)" check in copy_fs() > can obviously hit the 1 -> 0 transition. > > This is harmless, but should be probably fixed just to avoid another report > from KCSAN. > > I do not want to add another spin_lock(fs->lock). We can change copy_fs() to > use data_race(), but I'd prefer the patch below. Yes, it needs the additional > comment(s) to explain READ_ONCE(). > > What do you think? Did I miss somthing again??? Quite possibly... > > Mateusz, I hope you will cleanup this horror sooner or later ;) > > Oleg. > --- > > diff --git a/fs/exec.c b/fs/exec.c > index 5d1c0d2dc403..42a7f9b43911 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1495,7 +1495,7 @@ 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; > + WRITE_ONCE(current->fs->in_exec, 0); > mutex_unlock(¤t->signal->cred_guard_mutex); > abort_creds(bprm->cred); > } > diff --git a/kernel/fork.c b/kernel/fork.c > index 4c2df3816728..381af8c8ece8 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -1802,7 +1802,7 @@ static int copy_fs(unsigned long clone_flags, struct task_struct *tsk) > /* tsk->fs is already what we want */ > spin_lock(&fs->lock); > /* "users" and "in_exec" locked for check_unsafe_exec() */ > - if (fs->in_exec) { > + if (READ_ONCE(fs->in_exec)) { > spin_unlock(&fs->lock); > return -EAGAIN; > } > Yeah, this seems reasonable. -- Kees Cook