git: 1d874ba4f8ba - main - fsetown: Fix process lookup bugs

[Mark Johnston]

The branch main has been updated by markj:

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

commit 1d874ba4f8ba58296cd9df611f5346dad8e91664
Author:     Mark Johnston <markj at FreeBSD.org>
AuthorDate: 2021-08-25 20:18:10 +0000
Commit:     Mark Johnston <markj at FreeBSD.org>
CommitDate: 2021-08-25 20:18:10 +0000

    fsetown: Fix process lookup bugs
    
    - pget()/pfind() will acquire the PID hash bucket locks, which are
      sleepable sx locks, but this means that the sigio mutex cannot be held
      while calling these functions.  Instead, use pget() to hold the
      process, after which we lock the sigio and proc locks, respectively.
    - funsetownlst() assumes that processes cannot be registered for SIGIO
      once they have P_WEXIT set.  However, pfind() will happily return
      exiting processes, breaking the invariant.  Add an explicit check for
      P_WEXIT in fsetown() to fix this. [1]
    
    Fixes:  f52979098d3c ("Fix a pair of races in SIGIO registration")
    Reported by:    syzkaller [1]
    Reviewed by:    kib
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D31661
---
 sys/kern/kern_descrip.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index c7269e4b33a9..e6a6a36801e4 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -1031,18 +1031,16 @@ funsetown_locked(struct sigio *sigio)
 
 	if (sigio == NULL)
 		return (NULL);
-	*(sigio->sio_myref) = NULL;
+	*sigio->sio_myref = NULL;
 	if (sigio->sio_pgid < 0) {
 		pg = sigio->sio_pgrp;
 		PGRP_LOCK(pg);
-		SLIST_REMOVE(&sigio->sio_pgrp->pg_sigiolst, sigio,
-		    sigio, sio_pgsigio);
+		SLIST_REMOVE(&pg->pg_sigiolst, sigio, sigio, sio_pgsigio);
 		PGRP_UNLOCK(pg);
 	} else {
 		p = sigio->sio_proc;
 		PROC_LOCK(p);
-		SLIST_REMOVE(&sigio->sio_proc->p_sigiolst, sigio,
-		    sigio, sio_pgsigio);
+		SLIST_REMOVE(&p->p_sigiolst, sigio, sigio, sio_pgsigio);
 		PROC_UNLOCK(p);
 	}
 	return (sigio);
@@ -1156,18 +1154,25 @@ fsetown(pid_t pgid, struct sigio **sigiop)
 	}
 
 	ret = 0;
+	osigio = NULL;
 
 	sigio = malloc(sizeof(struct sigio), M_SIGIO, M_WAITOK);
 	sigio->sio_pgid = pgid;
 	sigio->sio_ucred = crhold(curthread->td_ucred);
 	sigio->sio_myref = sigiop;
 
-	sx_slock(&proctree_lock);
-	SIGIO_LOCK();
-	osigio = funsetown_locked(*sigiop);
 	if (pgid > 0) {
-		proc = pfind(pgid);
-		if (proc == NULL) {
+		ret = pget(pgid, PGET_NOTWEXIT | PGET_NOTID | PGET_HOLD, &proc);
+		SIGIO_LOCK();
+		if (ret != 0)
+			goto fail;
+
+		osigio = funsetown_locked(*sigiop);
+
+		PROC_LOCK(proc);
+		_PRELE(proc);
+		if ((proc->p_flag & P_WEXIT) != 0) {
+			PROC_UNLOCK(proc);
 			ret = ESRCH;
 			goto fail;
 		}
@@ -1190,12 +1195,17 @@ fsetown(pid_t pgid, struct sigio **sigiop)
 		SLIST_INSERT_HEAD(&proc->p_sigiolst, sigio, sio_pgsigio);
 		PROC_UNLOCK(proc);
 	} else /* if (pgid < 0) */ {
+		sx_slock(&proctree_lock);
+		SIGIO_LOCK();
 		pgrp = pgfind(-pgid);
 		if (pgrp == NULL) {
+			sx_sunlock(&proctree_lock);
 			ret = ESRCH;
 			goto fail;
 		}
 
+		osigio = funsetown_locked(*sigiop);
+
 		/*
 		 * Policy - Don't allow a process to FSETOWN a process
 		 * in another session.
@@ -1205,16 +1215,17 @@ fsetown(pid_t pgid, struct sigio **sigiop)
 		 * group for maximum safety.
 		 */
 		if (pgrp->pg_session != curthread->td_proc->p_session) {
+			sx_sunlock(&proctree_lock);
 			PGRP_UNLOCK(pgrp);
 			ret = EPERM;
 			goto fail;
 		}
 
-		SLIST_INSERT_HEAD(&pgrp->pg_sigiolst, sigio, sio_pgsigio);
 		sigio->sio_pgrp = pgrp;
+		SLIST_INSERT_HEAD(&pgrp->pg_sigiolst, sigio, sio_pgsigio);
 		PGRP_UNLOCK(pgrp);
+		sx_sunlock(&proctree_lock);
 	}
-	sx_sunlock(&proctree_lock);
 	*sigiop = sigio;
 	SIGIO_UNLOCK();
 	if (osigio != NULL)
@@ -1223,7 +1234,6 @@ fsetown(pid_t pgid, struct sigio **sigiop)
 
 fail:
 	SIGIO_UNLOCK();
-	sx_sunlock(&proctree_lock);
 	sigiofree(sigio);
 	if (osigio != NULL)
 		sigiofree(osigio);