git: f3851b235b23 - main - ktrace: Fix a race with fork()

[Mark Johnston]

The branch main has been updated by markj:

URL: https://cgit.FreeBSD.org/src/commit/?id=f3851b235b23d9220ace31bbc89b1fe0a78fc75c

commit f3851b235b23d9220ace31bbc89b1fe0a78fc75c
Author:     Mark Johnston <markj at FreeBSD.org>
AuthorDate: 2021-05-27 19:49:59 +0000
Commit:     Mark Johnston <markj at FreeBSD.org>
CommitDate: 2021-05-27 19:52:20 +0000

    ktrace: Fix a race with fork()
    
    ktrace(2) may toggle trace points in any of
    1. a single process
    2. all members of a process group
    3. all descendents of the processes in 1 or 2
    
    In the first two cases, we do not permit the operation if the process is
    being forked or not visible. However, in case 3 we did not enforce this
    restriction for descendents. As a result, the assertions about the child
    in ktrprocfork() may be violated.
    
    Move these checks into ktrops() so that they are applied consistently.
    
    Allow KTROP_CLEAR for nascent processes. Otherwise, there is a window
    where we cannot clear trace points for a nascent child if they are
    inherited from the parent.
    
    Reported by:    syzbot+d96676592978f137e05c at syzkaller.appspotmail.com
    Reported by:    syzbot+7c98fcf84a4439f2817f at syzkaller.appspotmail.com
    Reviewed by:    kib
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D30481
---
 sys/kern/kern_ktrace.c | 43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/sys/kern/kern_ktrace.c b/sys/kern/kern_ktrace.c
index dc064d9ebd67..4a2dad20b035 100644
--- a/sys/kern/kern_ktrace.c
+++ b/sys/kern/kern_ktrace.c
@@ -1006,7 +1006,7 @@ sys_ktrace(struct thread *td, struct ktrace_args *uap)
 	int facs = uap->facs & ~KTRFAC_ROOT;
 	int ops = KTROP(uap->ops);
 	int descend = uap->ops & KTRFLAG_DESCEND;
-	int nfound, ret = 0;
+	int ret = 0;
 	int flags, error = 0;
 	struct nameidata nd;
 	struct ktr_io_params *kiop, *old_kiop;
@@ -1080,42 +1080,31 @@ restart:
 			error = ESRCH;
 			goto done;
 		}
+
 		/*
 		 * ktrops() may call vrele(). Lock pg_members
 		 * by the proctree_lock rather than pg_mtx.
 		 */
 		PGRP_UNLOCK(pg);
-		nfound = 0;
+		if (LIST_EMPTY(&pg->pg_members)) {
+			sx_sunlock(&proctree_lock);
+			error = ESRCH;
+			goto done;
+		}
 		LIST_FOREACH(p, &pg->pg_members, p_pglist) {
 			PROC_LOCK(p);
-			if (p->p_state == PRS_NEW ||
-			    p_cansee(td, p) != 0) {
-				PROC_UNLOCK(p); 
-				continue;
-			}
-			nfound++;
 			if (descend)
 				ret |= ktrsetchildren(td, p, ops, facs, kiop);
 			else
 				ret |= ktrops(td, p, ops, facs, kiop);
 		}
-		if (nfound == 0) {
-			sx_sunlock(&proctree_lock);
-			error = ESRCH;
-			goto done;
-		}
 	} else {
 		/*
 		 * by pid
 		 */
 		p = pfind(uap->pid);
-		if (p == NULL)
+		if (p == NULL) {
 			error = ESRCH;
-		else
-			error = p_cansee(td, p);
-		if (error) {
-			if (p != NULL)
-				PROC_UNLOCK(p);
 			sx_sunlock(&proctree_lock);
 			goto done;
 		}
@@ -1187,8 +1176,20 @@ ktrops(struct thread *td, struct proc *p, int ops, int facs,
 		PROC_UNLOCK(p);
 		return (0);
 	}
-	if (p->p_flag & P_WEXIT) {
-		/* If the process is exiting, just ignore it. */
+	if ((ops == KTROP_SET && p->p_state == PRS_NEW) || !p_cansee(td, p)) {
+		/*
+		 * Disallow setting trace points if the process is being born.
+		 * This avoids races with trace point inheritance in
+		 * ktrprocfork().
+		 */
+		PROC_UNLOCK(p);
+		return (0);
+	}
+	if ((p->p_flag & P_WEXIT) != 0) {
+		/*
+		 * There's nothing to do if the process is exiting, but avoid
+		 * signaling an error.
+		 */
 		PROC_UNLOCK(p);
 		return (1);
 	}