On Fri, Mar 21, 2025 at 01:10:26AM -0700, Kees Cook wrote: > On Fri, Mar 21, 2025 at 01:44:23AM +0000, Al Viro wrote: > > On Thu, Mar 20, 2025 at 01:09:38PM -0700, Kees Cook wrote: > > > > > What I can imagine here is two failing execs racing a fork: > > > > > > A start execve > > > B fork with CLONE_FS > > > C start execve, reach check_unsafe_exec(), set fs->in_exec > > > A bprm_execve() failure, clear fs->in_exec > > > B copy_fs() increment fs->users. > > > C bprm_execve() failure, clear fs->in_exec > > > > > > But I don't think this is a "real" flaw, though, since the locking is to > > > protect a _successful_ execve from a fork (i.e. getting the user count > > > right). A successful execve will de_thread, and I don't see any wrong > > > counting of fs->users with regard to thread lifetime. > > > > > > Did I miss something in the analysis? Should we perform locking anyway, > > > or add data race annotations, or something else? > > > > Umm... What if C succeeds, ending up with suid sharing ->fs? > > I still can't quite construct it -- fs->users is always correct, I > think? > > Below would be the bad set of events, but it's wrong that "fs->users==1". > If A and C are both running with CLONE_FS then fs->users==2. A would need to > exit first, but it can't do that and also set fs->in_exec=0 > > A execve, reaches bprm_execve() failure path > B fork with CLONE_FS, reaches copy_fs() > C execve, reaches check_unsafe_exec() > C takes fs->lock, counts, finds safe fs->users==1, sets in_exec=1, unlocks > A sets fs->in_exec=0 > B takes fs->lock, sees in_exec==0, does fs->users++, unlocks > C goes setuid, sharing fs with unpriv B > > Something still feels very weird, though. Does fs->in_exec not matter at > all? Hmm, no, it stops fs->users++ happening after it was validated to be 1. This is a harmless data race afaict. See my other mail.
