KCSAN: data-race in bprm_execve / copy_fs (4)

==================================================================

BUG: KCSAN: data-race in bprm_execve / copy_fs

write to 0xffff8881044f8250 of 4 bytes by task 13692 on cpu 0:
bprm_execve+0x748/0x9c0 fs/exec.c:1884

 do_execveat_common+0x769/0x7e0 fs/exec.c:1966
 do_execveat fs/exec.c:2051 [inline]
 __do_sys_execveat fs/exec.c:2125 [inline]
 __se_sys_execveat fs/exec.c:2119 [inline]
 __x64_sys_execveat+0x75/0x90 fs/exec.c:2119
 x64_sys_call+0x291e/0x2dc0 arch/x86/include/generated/asm/syscalls_64.h:323
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xc9/0x1c0 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

read to 0xffff8881044f8250 of 4 bytes by task 13686 on cpu 1:
copy_fs+0x95/0xf0 kernel/fork.c:1770

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?

Quoted 14 lines · Kees Cook

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.

Quoted 2 lines

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?

Quoted 16 lines · Al Viro

Quoted 14 lines · Kees Cook

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.

Quoted 4 lines

Quoted 2 lines

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.

Hey look another threaded exec bug. :|

Quoted 5 lines · syzbot

==================================================================

BUG: KCSAN: data-race in bprm_execve / copy_fs

write to 0xffff8881044f8250 of 4 bytes by task 13692 on cpu 0:
bprm_execve+0x748/0x9c0 fs/exec.c:1884

This is:

        current->fs->in_exec = 0;

And is part of the execve failure path:

out:

	...
        if (bprm->point_of_no_return && !fatal_signal_pending(current))
                force_fatal_sig(SIGSEGV);

        sched_mm_cid_after_execve(current);
        current->fs->in_exec = 0;
        current->in_execve = 0;

        return retval;
}

Quoted 10 lines

 do_execveat_common+0x769/0x7e0 fs/exec.c:1966
 do_execveat fs/exec.c:2051 [inline]
 __do_sys_execveat fs/exec.c:2125 [inline]
 __se_sys_execveat fs/exec.c:2119 [inline]
 __x64_sys_execveat+0x75/0x90 fs/exec.c:2119
 x64_sys_call+0x291e/0x2dc0 arch/x86/include/generated/asm/syscalls_64.h:323
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xc9/0x1c0 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Quoted 2 lines

read to 0xffff8881044f8250 of 4 bytes by task 13686 on cpu 1:
copy_fs+0x95/0xf0 kernel/fork.c:1770

This is:

                if (fs->in_exec) {

Which is under lock:

        struct fs_struct *fs = current->fs;
        if (clone_flags & CLONE_FS) {
                /* tsk->fs is already what we want */
                spin_lock(&fs->lock);
                /* "users" and "in_exec" locked for check_unsafe_exec() * */
                if (fs->in_exec) {
                        spin_unlock(&fs->lock);
                        return -EAGAIN;
                }
                fs->users++;
                spin_unlock(&fs->lock);

Does execve need to be taking this lock? The other thing touching it is check_unsafe_exec(), which takes the lock. It looks like the bprm_execve() lock was removed in commit 8c652f96d385 ("do_execve() must not clear fs->in_exec if it was set by another thread") which used the return value from check_unsafe_exec():

    When do_execve() succeeds, it is safe to clear ->in_exec unconditionally.
    It can be set only if we don't share ->fs with another process, and since
    we already killed all sub-threads either ->in_exec == 0 or we are the
    only user of this ->fs.

    Also, we do not need fs->lock to clear fs->in_exec.

This logic was updated in commit 9e00cdb091b0 ("exec:check_unsafe_exec:
kill the dead -EAGAIN and clear_in_exec logic"), which includes this rationale:

            2. "out_unmark:" in do_execve_common() is either called
               under ->cred_guard_mutex, or after de_thread() which
               kills other threads, so we can't race with sub-thread
               which could set ->in_exec. And if ->fs is shared with
               another process ->in_exec should be false anyway.

The de_thread() is part of the "point of no return" in exec_binprm(), called via exec_binprm(). But the bprm_execve() error path is reachable from many paths prior to the point of no return.

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?

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.

Quoted 4 lines · Christian Brauner

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.

1) A enters check_unsafe_execve(), sets ->in_exec to 1
2) B enters check_unsafe_execve(), sets ->in_exec to 1

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

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.

Quoted 8 lines · Al Viro

Quoted 4 lines · Christian Brauner

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!

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 10 lines

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().

Quoted 5 lines

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:

fs/exec.c +2 -2

index 506cd411f4ac..988b8621c079 100644
@@ -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(&current->fs->in_exec);
 	current->in_execve = 0;
 	rseq_execve(current);
 	user_events_execve(current);

Quoted 65 lines · Kees Cook

Quoted 8 lines · Al Viro

Quoted 4 lines · Christian Brauner

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!

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 10 lines

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().

Quoted 5 lines

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:

fs/exec.c +2 -2

index 506cd411f4ac..988b8621c079 100644
@@ -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(&current->fs->in_exec);
 	current->in_execve = 0;
 	rseq_execve(current);
 	user_events_execve(current);

fs/exec.c static int bprm_execve(struct linux_binprm *bprm) · line 1887

Quoted 8 lines

fs/exec.c +1 -1

@@ -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(&current->fs->in_exec);
 	current->in_execve = 0;
 
 	return retval;

The bump is conditional and with this patch you may be issuing refcount_dec when you declined to refcount_inc.

A special case where there are others to worry about and which proceeds with an exec without leaving in any indicators is imo sketchy.

I would argue it would make the most sense to serialize these execs.

Vast majority of programs are single-threaded when they exec with an unshared ->fs, so they don't need to bear any overhead nor complexity modulo a branch.

For any fucky case you can park yourself waiting for any pending exec to finish.

Quoted 4 lines · Christian Brauner

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.

1) A enters check_unsafe_execve(), sets ->in_exec to 1
2) B enters check_unsafe_execve(), sets ->in_exec to 1
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
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.

Not really.

1) A enters check_unsafe_execve(), sets ->in_exec to 1
2) B enters check_unsafe_execve(), sets ->in_exec to 1

3) A calls exec_binprm(), fails (bad binary)
4) A clears ->in_exec

No, check_unsafe_execve() is called with cred_guard_mutex held, see prepare_bprm_creds()

Quoted 2 lines

3) A calls exec_binprm(), fails (bad binary)
4) A clears ->in_exec

So (2) can only happen after A fails and drops cred_guard_mutex.

And this means that we just need to ensure that ->in_exec is cleared before this mutex is dropped, no? Something like below?

Quoted 2 lines · Oleg Nesterov

And this means that we just need to ensure that ->in_exec is cleared before this mutex is dropped, no? Something like below?

Probably should work, but I wonder if it would be cleaner to have ->in_exec replaced with pointer to task_struct responsible. Not "somebody with that fs_struct for ->fs is trying to do execve(), has verified that nothing outside of their threads is using this and had been holding ->signal->cred_guard_mutex ever since then", but "this is the thread that..."

Quoted 6 lines · Al Viro

Quoted 2 lines · Oleg Nesterov

And this means that we just need to ensure that ->in_exec is cleared before this mutex is dropped, no? Something like below?

Quoted 6 lines

Probably should work, but I wonder if it would be cleaner to have ->in_exec replaced with pointer to task_struct responsible. Not "somebody with that fs_struct for ->fs is trying to do execve(), has verified that nothing outside of their threads is using this and had been holding ->signal->cred_guard_mutex ever since then", but "this is the thread that..."

perhaps... or something else to make this "not immediately obvious" fs->in_exec more clear.

But I guess we need something simple for -stable, so will you agree with this fix for now? Apart from changelog/comments.

	retval = de_thread(me);

(quoted diff) +1 -0

+	current->fs->in_exec = 0;

	if (retval)
		current->fs->in_exec = 0;

is correct but looks confusing. See "V2" below, it clears fs->in_exec after the "if (retval)" check.

syzbot says:

Unfortunately, I don't have any reproducer for this issue yet.

so I guess "#syz test: " is pointless right now...

Oleg.
---

fs/exec.c +3 -2

index 506cd411f4ac..02e8824fc9cd 100644
@@ -1236,6 +1236,7 @@ int begin_new_exec(struct linux_binprm * bprm)
 	if (retval)
 		goto out;
 
+	current->fs->in_exec = 0;
 	/*
 	 * Cancel any io_uring activity across execve
 	 */

@@ -1497,6 +1498,8 @@ static void free_bprm(struct linux_binprm *bprm)
 	}
 	free_arg_pages(bprm);
 	if (bprm->cred) {
+		// for the case exec fails before de_thread()
+		current->fs->in_exec = 0;
 		mutex_unlock(&current->signal->cred_guard_mutex);
 		abort_creds(bprm->cred);
 	}

@@ -1862,7 +1865,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 +1883,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;

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.

I had cursory glances at this code earlier and the more I try to understand it the more confused I am.

Per my other e-mail the obvious scheme would serialize all execs sharing ->fs and make copy_fs do a killable wait for execs to finish. Arguably this would also improve userspace-visible behavior as a transient -EBUSY would be eliminated.

is there a problem getting this done even for stable kernels? I understand it would be harder to backport churn-wise, but should be much easier to reason about?

I won't argue with another solution. But this problem is quite old,

unless I am totally confused this logic was wrong from the very beginning when fs->in_exec was introduced by 498052bba55ec.

So to me it would be better to have the trivial fix for stable, exactly because it is trivially backportable. Then cleanup/simplify this logic on top of it.

I won't argue with another solution. But this problem is quite old, unless I am totally confused this logic was wrong from the very beginning when fs->in_exec was introduced by 498052bba55ec.

So to me it would be better to have the trivial fix for stable, exactly because it is trivially backportable. Then cleanup/simplify this logic on top of it.

Quoted 4 lines · Oleg Nesterov

So to me it would be better to have the trivial fix for stable, exactly because it is trivially backportable. Then cleanup/simplify this logic on top of it.

So I got myself a crap testcase with a CLONE_FS'ed task which can execve and sanity-checked that suid is indeed not honored as expected.

Anyhow, the plan would be to serialize on the bit, synchronized with the current spin lock. copy_fs would call a helper to wait for it to clear, would still bump ->users under the spin lock.

This would decouple the handling from cred_mutex and avoid weirdness like clearing the ->in_exec flag when we never set it.

Quoted 3 lines · Mateusz Guzik

Quoted 1 line · Oleg Nesterov

Quoted 6 lines

Quoted 3 lines

So to me it would be better to have the trivial fix for stable, exactly because it is trivially backportable. Then cleanup/simplify this logic on top of it.

So I got myself a crap testcase with a CLONE_FS'ed task which can execve and sanity-checked that suid is indeed not honored as expected.

So you mean my patch can't fix the problem?

Quoted 7 lines

Anyhow, the plan would be to serialize on the bit, synchronized with the current spin lock. copy_fs would call a helper to wait for it to clear, would still bump ->users under the spin lock.

This would decouple the handling from cred_mutex and avoid weirdness like clearing the ->in_exec flag when we never set it.

I don't really understand the idea, but as I said I won't argue with another solution.

Quoted 9 lines · Oleg Nesterov

Quoted 7 lines · Mateusz Guzik

Quoted 4 lines · Oleg Nesterov

So to me it would be better to have the trivial fix for stable, exactly because it is trivially backportable. Then cleanup/simplify this logic on top of it.

Quoted 4 lines

Quoted 2 lines

So I got myself a crap testcase with a CLONE_FS'ed task which can execve and sanity-checked that suid is indeed not honored as expected.

So you mean my patch can't fix the problem?

No, I think the patch works.

I am saying the current scheme is avoidably hard to reason about.

Quoted 14 lines · Mateusz Guzik

Quoted 12 lines · Oleg Nesterov

Quoted 9 lines · Mateusz Guzik

Quoted 4 lines · Oleg Nesterov

So to me it would be better to have the trivial fix for stable, exactly because it is trivially backportable. Then cleanup/simplify this logic on top of it.

So I got myself a crap testcase with a CLONE_FS'ed task which can execve and sanity-checked that suid is indeed not honored as expected.

Quoted 5 lines

Quoted 1 line

So you mean my patch can't fix the problem?

No, I think the patch works.

I am saying the current scheme is avoidably hard to reason about.

Ah, OK, thanks. Then I still think it makes more sense to do the cleanups you propose on top of this fix.

Quoted 19 lines · Oleg Nesterov

Quoted 18 lines · Mateusz Guzik

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Mateusz Guzik

Quoted 1 line · Oleg Nesterov

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

So to me it would be better to have the trivial fix for stable, exactly because it is trivially backportable. Then cleanup/simplify this logic on top of it.

So I got myself a crap testcase with a CLONE_FS'ed task which can execve and sanity-checked that suid is indeed not honored as expected.

So you mean my patch can't fix the problem?

No, I think the patch works.

Quoted 4 lines

Quoted 1 line

I am saying the current scheme is avoidably hard to reason about.

Ah, OK, thanks. Then I still think it makes more sense to do the cleanups you propose on top of this fix.

I agree. We should go with Oleg's fix that in the old scheme and use that. And then @Mateusz your cleanup should please go on top!

Quoted 23 lines · Christian Brauner

Quoted 21 lines · Oleg Nesterov

Quoted 19 lines · Mateusz Guzik

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Mateusz Guzik

Quoted 1 line · Oleg Nesterov

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

So to me it would be better to have the trivial fix for stable, exactly because it is trivially backportable. Then cleanup/simplify this logic on top of it.

So I got myself a crap testcase with a CLONE_FS'ed task which can execve and sanity-checked that suid is indeed not honored as expected.

So you mean my patch can't fix the problem?

No, I think the patch works.

I am saying the current scheme is avoidably hard to reason about.

Quoted 5 lines

Quoted 2 lines

Ah, OK, thanks. Then I still think it makes more sense to do the cleanups you propose on top of this fix.

I agree. We should go with Oleg's fix that in the old scheme and use that. And then @Mateusz your cleanup should please go on top!

Ok, in that case I'm gonna ship when I'm gonna ship(tm), maybe later this week.

Quoted 27 lines · Mateusz Guzik

Quoted 26 lines · Christian Brauner

Quoted 23 lines · Oleg Nesterov

Quoted 11 lines · Mateusz Guzik

Quoted 9 lines · Oleg Nesterov

Quoted 7 lines · Mateusz Guzik

Quoted 4 lines · Oleg Nesterov

So to me it would be better to have the trivial fix for stable, exactly because it is trivially backportable. Then cleanup/simplify this logic on top of it.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

So I got myself a crap testcase with a CLONE_FS'ed task which can execve and sanity-checked that suid is indeed not honored as expected.

So you mean my patch can't fix the problem?

No, I think the patch works.

I am saying the current scheme is avoidably hard to reason about.

Ah, OK, thanks. Then I still think it makes more sense to do the cleanups you propose on top of this fix.

Quoted 4 lines

Quoted 2 lines

I agree. We should go with Oleg's fix that in the old scheme and use that. And then @Mateusz your cleanup should please go on top!

Ok, in that case I'm gonna ship when I'm gonna ship(tm), maybe later this week.

Ok, I've taken the patch as I've got a first round of fixes to send already.

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

Damn, I am stupid.

Quoted 10 lines · Oleg Nesterov

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/[email protected]/#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.
---

fs/exec.c +1 -1

index 5d1c0d2dc403..42a7f9b43911 100644
@@ -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(&current->signal->cred_guard_mutex);
 		abort_creds(bprm->cred);
 	}

kernel/fork.c +0 -0

index 4c2df3816728..381af8c8ece8 100644

Damn, I am stupid.

Quoted 10 lines · Oleg Nesterov

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/[email protected]/#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.
---

fs/exec.c +1 -1

index 5d1c0d2dc403..42a7f9b43911 100644
@@ -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(&current->signal->cred_guard_mutex);
                abort_creds(bprm->cred);
        }

kernel/fork.c +0 -0

index 4c2df3816728..381af8c8ece8 100644