From owner-dev-commits-src-all@freebsd.org Wed Aug 25 20:29:55 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 97AA565AC83; Wed, 25 Aug 2021 20:29:55 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4GvyJg3ppPz3vBf; Wed, 25 Aug 2021 20:29:55 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 6B0ED22D5F; Wed, 25 Aug 2021 20:29:55 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 17PKTtXR011162; Wed, 25 Aug 2021 20:29:55 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 17PKTtR1011161; Wed, 25 Aug 2021 20:29:55 GMT (envelope-from git) Date: Wed, 25 Aug 2021 20:29:55 GMT Message-Id: <202108252029.17PKTtR1011161@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Mark Johnston Subject: git: 1d874ba4f8ba - main - fsetown: Fix process lookup bugs MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: markj X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 1d874ba4f8ba58296cd9df611f5346dad8e91664 Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Aug 2021 20:29:55 -0000 The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=1d874ba4f8ba58296cd9df611f5346dad8e91664 commit 1d874ba4f8ba58296cd9df611f5346dad8e91664 Author: Mark Johnston AuthorDate: 2021-08-25 20:18:10 +0000 Commit: Mark Johnston 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);