Bug 251915 - TOCTOU race between tty_signal_sessleader() and killjobc()
Summary: TOCTOU race between tty_signal_sessleader() and killjobc()
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Konstantin Belousov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-17 10:04 UTC by Jakub Piecuch
Modified: 2021-12-02 12:01 UTC (History)
3 users (show)

See Also:


Attachments
tty: read s_leader exactly once in tty_signal_sessleader() (834 bytes, patch)
2020-12-17 17:09 UTC, Jakub Piecuch
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Piecuch 2020-12-17 10:04:16 UTC
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.
Comment 1 Konstantin Belousov freebsd_committer 2020-12-17 16:29:52 UTC
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.
Comment 2 Jakub Piecuch 2020-12-17 16:50:17 UTC
(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).
Comment 3 Konstantin Belousov freebsd_committer 2020-12-17 17:03:23 UTC
(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).
Comment 4 Jakub Piecuch 2020-12-17 17:09:58 UTC
Created attachment 220667 [details]
tty: read s_leader exactly once in tty_signal_sessleader()
Comment 5 Konstantin Belousov freebsd_committer 2020-12-17 19:22:20 UTC
(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);
Comment 6 Jakub Piecuch 2020-12-17 19:39:49 UTC
(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 :-)
Comment 7 commit-hook freebsd_committer 2020-12-17 19:51:43 UTC
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