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 28542C35FFC for ; Sat, 22 Mar 2025 06:26:11 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5F7F2280002; Sat, 22 Mar 2025 02:26:08 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5A867280001; Sat, 22 Mar 2025 02:26:08 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 447CB280002; Sat, 22 Mar 2025 02:26:08 -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 25C8E280001 for ; Sat, 22 Mar 2025 02:26:08 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 64EE5120F3E for ; Sat, 22 Mar 2025 06:26:09 +0000 (UTC) X-FDA: 83248202058.05.AB96932 Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf10.hostedemail.com (Postfix) with ESMTP id AA70AC0007 for ; Sat, 22 Mar 2025 06:26:07 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=WtYFib+Q; spf=pass (imf10.hostedemail.com: domain of kees@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=kees@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1742624767; 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=PGeQuYrFyBVeuIgBtIpsRNh5DCkhfT7uF92+SbedXMI=; b=6xg4/PIRpHMqJQMN3Ye1BsyG/gY7mPf+09yWF9BL/Gss8p7hNXondnVIoeuGw1MRmwW5Ck aTHvuNhcB0wINOHUlHOS0WdXJ3sIcvT00hMZG10A6cfT1qDFWC2JMVySOa/jAvRbvPxJ9D USxbTQql4ZQKuYQMh6AhrrvSbAO9Oas= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=WtYFib+Q; spf=pass (imf10.hostedemail.com: domain of kees@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=kees@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1742624767; a=rsa-sha256; cv=none; b=CSej1k3zScEki1kg4UXXbRwYiBlYSvr6hFS2G0XWbutyAaB1nsXhy3xKDfyjVgSD3AyszU G7kd4tx6f5awrasiqp3ifVuxIoeI0iuPVgLKnPbFP9PNocicpGnbYlXoZ3NgewHciqaVDV Ez0/VfEV/X+7HqH7i3cAVFq0RLRcrvA= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 6C55AA4A17E; Sat, 22 Mar 2025 06:20:37 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id EFD14C4CEDD; Sat, 22 Mar 2025 06:26:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1742624766; bh=f2Dru0mOp1GeU7ZUiZtTDdgdJ+Abue+hSj07w9O/LT8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=WtYFib+Q3DoREtr2IJNNacGcq8XdyyIPv3ygvWEamXjhiMnaR6w04f8o2ZHJC9v/r uIBmKS7CZQAZCuPJg73/vaNO1Wx005GyVa7KHJZ/Zkv+/lMdvSDiL+QvOEIcg+Ib9p kEmICSBUowT0M4TxBdIPswgMJLgJ0I9X5XchezrAtxkEFM1jTfWSkR4aK7Mf2HLyEm gPTQuCY9no07QXktpCazE+TrMGW7ZSSTOHD3Ir6UB3mAgbtWEgBui2+UhGE1RnuklS VEGEWdR3OgSUkzRUgyiJfwK7bNZGCgBxCmWawrazP2dUWpXqWZY7YxKR8V76SnTSP+ acdtDH6GJTTmA== Date: Fri, 21 Mar 2025 23:26:03 -0700 From: Kees Cook To: Al Viro Cc: Christian Brauner , Oleg Nesterov , jack@suse.cz, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, syzkaller-bugs@googlegroups.com, syzbot Subject: Re: [syzbot] [fs?] [mm?] KCSAN: data-race in bprm_execve / copy_fs (4) Message-ID: <202503212313.1E55652@keescook> References: <67dc67f0.050a0220.25ae54.001f.GAE@google.com> <202503201225.92C5F5FB1@keescook> <20250321-abdecken-infomaterial-2f373f8e3b3c@brauner> <20250322010008.GG2023217@ZenIV> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250322010008.GG2023217@ZenIV> X-Rspam-User: X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: AA70AC0007 X-Stat-Signature: 1donhpie795ocaurfp47oimj6e5tdn6c X-HE-Tag: 1742624767-574004 X-HE-Meta: U2FsdGVkX1/SgI502J9aTeVFymtTb2FIfTHogQy0H0S+u5yHoiYlwoSyhTUu2H68I+m9D/n0wprlWhMqW+6tAk8WK/WgDzd8e/P0DsjvY09A8ERZWeexkyM54W3dOBSfIFBsCEq998ew7tqRZPPbTCbAnCNLSql9w8DqV4MAGFz1FoCDMcsJGerrsoyiYOdxNHZvfo+sentzbCqEuBg1roj1EaHYxN2g3836pV1OoUGrvfHAZ1oeF6Vx5OOS5tbqAvCmG1vuADEXieVCwWo39IEubaRkACAl2XIYo4VcyuOriasch9oG530t5VE/6BjjqA3l/aeaKoiinZ/IKN6tgGAGOiPcmHcGx6lvKAzy+LvaLL2YA0EkpvMqmAnfYQtoOIrDR9qXKuY/CWcmzzPq9Xs1M7htDJ6oRKkSg/Yn9UjD/KcO7R8JMdqorHolaZia59TVxfHhdX+rwETqemKCC1sekAd9Y9OpqCD4zB7PmvNKewn9VAKP/SNjNOaaEeeqnUwJ4axCYd17zV/f7XtLJL9M/4r5Jqqls5LHFt4VupV9yPVreGGOKtUK5c7l5J6iaGEoLqYfPjcpE1dULTpLXDCQrQphePGQhQYqFI1PSeLw8vAFkhFuJPe+s69Kk3quQAiB/8GoJioJIT/84WcuVU2TWkvU/8zuC6ikrIcDDkZmQVU2aNGqGh9eCDb0TULThszZ+DvmzlAXM9YK/9DrlrvVW3unGQ7Qt0IgM9eyS+qk5mwoOiSoOOLXoiyZBECwSs7gtm+OATsO0P6hnEtU8lwtU+cnPZ4imKTrDcxeoiqtle59JlmxEH3fMbMqEYBnq/e3nXcj5gwBQm5CFVsfd4tMdtBYAAgPBUFnbeshhH3sUnN5ONhNmy9kagu6mYSrwRi1TyckuBMcFhjLUGJY2sXZsPfyWt+H7omR0nOwv9Eo0698RjGQwVjfO5FOxqxcYMMiFxH0fNiuqL35ps1 hAgIzYZS AUO0UqdKI3EwEd+5s057sGleGnTOY6+i4TH2yKf/A7bksNajVt/Yb6/2qQiTx2iqYOlwOuhZzino7kO0+67JjbMwEGjfaBUDRdcE4PbJroyu2Ml2vLMy0YihMn/E8rmPMVG9MLdku6+835yE3tsjUi4Wb1FIQYDPEyENMvatoTMoijAZYkSXaKNFjnU+Zbqj1RUvUQ8wDNMulaJgc6W0dlLVH+bGLy2yHcaa5S1JUGoV525bLpneXzoKgCF2LfqilIMe4ps/MVBV8gSLaGPtm+TNuoGk3YhfQnaVc93/cISr6IS2y4faWWxKZqNTCU418PZyaUBaj492pk4swGn7jUE8eo/tsfwBHbvvB4RMiDVbSnRDETS66KczU6pd/qY74isr+BeoHCyrYEpW9HEpKQAPWr05p0kRrRneW5B9UKcLvUroTzTHAKRLPJQ== 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 Sat, Mar 22, 2025 at 01:00:08AM +0000, Al Viro wrote: > On Fri, Mar 21, 2025 at 09:45:39AM +0100, Christian Brauner wrote: > > > Afaict, the only way this data race can happen is if we jump to the > > cleanup label and then reset current->fs->in_exec. If the execve was > > successful there's no one to race us with CLONE_FS obviously because we > > took down all other threads. > > Not really. Yeah, you found it. Thank you! > 1) A enters check_unsafe_execve(), sets ->in_exec to 1 > 2) B enters check_unsafe_execve(), sets ->in_exec to 1 With 3 threads A, B, and C already running, fs->users == 3, so steps (1) and (2) happily pass. > 3) A calls exec_binprm(), fails (bad binary) > 4) A clears ->in_exec > 5) C calls clone(2) with CLONE_FS and spawns D - ->in_exec is 0 D's creation bumps fs->users == 4. > 6) B gets through exec_binprm(), kills A and C, but not D. > 7) B clears ->in_exec, returns > > Result: B and D share ->fs, B runs suid binary. > > Had (5) happened prior to (2), (2) wouldn't have set ->in_exec; > had (5) happened prior to (4), clone() would've failed; had > (5) been delayed past (6), there wouldn't have been a thread > to call clone(). > > But in the window between (4) and (6), clone() doesn't see > execve() in progress and check_unsafe_execve() has already > been done, so it hadn't seen the extra thread. > > IOW, it really is racy. It's a counter, not a flag. Yeah, I would agree. Totally untested patch: diff --git a/fs/exec.c b/fs/exec.c index 506cd411f4ac..988b8621c079 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1632,7 +1632,7 @@ static void check_unsafe_exec(struct linux_binprm *bprm) if (p->fs->users > n_fs) bprm->unsafe |= LSM_UNSAFE_SHARE; else - p->fs->in_exec = 1; + refcount_inc(&p->fs->in_exec); spin_unlock(&p->fs->lock); } @@ -1862,7 +1862,7 @@ static int bprm_execve(struct linux_binprm *bprm) sched_mm_cid_after_execve(current); /* execve succeeded */ - current->fs->in_exec = 0; + refcount_dec(¤t->fs->in_exec); current->in_execve = 0; rseq_execve(current); user_events_execve(current); @@ -1881,7 +1881,7 @@ static int bprm_execve(struct linux_binprm *bprm) force_fatal_sig(SIGSEGV); sched_mm_cid_after_execve(current); - current->fs->in_exec = 0; + refcount_dec(¤t->fs->in_exec); current->in_execve = 0; return retval; diff --git a/fs/fs_struct.c b/fs/fs_struct.c index 64c2d0814ed6..df46b873c425 100644 --- a/fs/fs_struct.c +++ b/fs/fs_struct.c @@ -115,7 +115,7 @@ struct fs_struct *copy_fs_struct(struct fs_struct *old) /* We don't need to lock fs - think why ;-) */ if (fs) { fs->users = 1; - fs->in_exec = 0; + fs->in_exec = REFCOUNT_INIT(0); spin_lock_init(&fs->lock); seqcount_spinlock_init(&fs->seq, &fs->lock); fs->umask = old->umask; diff --git a/include/linux/fs_struct.h b/include/linux/fs_struct.h index 783b48dedb72..aebc0b7aedb9 100644 --- a/include/linux/fs_struct.h +++ b/include/linux/fs_struct.h @@ -11,7 +11,7 @@ struct fs_struct { spinlock_t lock; seqcount_spinlock_t seq; int umask; - int in_exec; + refcount_t in_exec; struct path root, pwd; } __randomize_layout; diff --git a/kernel/fork.c b/kernel/fork.c index 735405a9c5f3..8b427045fd86 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1767,7 +1767,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 (refcount_read(&fs->in_exec)) { spin_unlock(&fs->lock); return -EAGAIN; } -- Kees Cook