In tty_signal_sessleader(): if (tp->t_session != NULL && tp->t_session->s_leader != NULL) { p = tp->t_session->s_leader; PROC_LOCK(p); kern_psignal(p, sig); PROC_UNLOCK(p); } We're holding the tty lock, but not the session lock, so the s_leader may be changed to NULL right after the != NULL check by a concurrent invocation of killjobc() by the session leader. The compiler *might* optimize this and only read s_leader a single time, but that's far from guaranteed. I don't have a patch because I'm not sure what the right way to deal with this is. We could read s_leader a single time, like this: if (tp->t_session != NULL && (p = tp->t_session->s_leader) != NULL) { PROC_LOCK(p); kern_psignal(p, sig); PROC_UNLOCK(p); } ...but the compiler may in theory still output vulnerable code. I don't know what assumptions are made in FreeBSD about what compilers can and can't do.
Our atomic_load_ptr() guarantees that access occurs at the point where it is called. So you can do (p = atomic_load_ptr(&tp->t_session->s_leader)) != NULL and it cannot be unrolled by the optimizing compiler.
(In reply to Konstantin Belousov from comment #1) I see. This should solve the problem, provided the compiler doesn't perform store tearing on any assignments to s_leader (which is a reasonable assumption, since on most, if not all supported architectures pointers fit in a native word).
(In reply to Jakub Piecuch from comment #2) One of the fundamental assumptions of the FreeBSD kernel code is that plain stores and loads of fundamental types (integers up to long and pointers) are atomic on supported arches. This is explained in the very beginning of atomic(9).
Created attachment 220667 [details] tty: read s_leader exactly once in tty_signal_sessleader()
(In reply to Jakub Piecuch from comment #4) You talked about a race with zeroing s_leader, but comment now mention t_session. I reformulated the comment, and also read t_session into local, just to shorten the lines and make this fragment easier to read. Also, atomics come from sys/systm.h already. diff --git a/sys/kern/tty.c b/sys/kern/tty.c index 7526638b921..8d4d25a4ac0 100644 --- a/sys/kern/tty.c +++ b/sys/kern/tty.c @@ -1474,6 +1474,7 @@ void tty_signal_sessleader(struct tty *tp, int sig) { struct proc *p; + struct session *s; tty_assert_locked(tp); MPASS(sig >= 1 && sig < NSIG); @@ -1482,8 +1483,14 @@ tty_signal_sessleader(struct tty *tp, int sig) tp->t_flags &= ~TF_STOPPED; tp->t_termios.c_lflag &= ~FLUSHO; - if (tp->t_session != NULL && tp->t_session->s_leader != NULL) { - p = tp->t_session->s_leader; + /* + * Load s_leader exactly once to avoid race where s_leader is + * set to NULL by a concurrent invocation of killjobc() by the + * session leader. Note that we are not holding t_session's + * lock for the read. + */ + if ((s = tp->t_session) != NULL && + (p = atomic_load_ptr(&s->s_leader)) != NULL) { PROC_LOCK(p); kern_psignal(p, sig); PROC_UNLOCK(p);
(In reply to Konstantin Belousov from comment #5) I mention t_session in the comment because it contains the mutex that we should be holding in order to access t_session->s_leader. proctree_lock would also suffice for reading, but we're not holding that lock either. Thanks for reviewing and improving my patch. I'm a kernel newbie, so all feedback is appreciated :-)
A commit references this bug: Author: kib Date: Thu Dec 17 19:51:39 UTC 2020 New revision: 368735 URL: https://svnweb.freebsd.org/changeset/base/368735 Log: Fix a race in tty_signal_sessleader() with unlocked read of s_leader. Since we do not own the session lock, a parallel killjobc() might reset s_leader to NULL after we checked it. Read s_leader only once and ensure that compiler is not allowed to reload. While there, make access to t_session somewhat more pretty by using local variable. PR: 251915 Submitted by: Jakub Piecuch <j.piecuch96@gmail.com> MFC after: 1 week Changes: head/sys/kern/tty.c