d/freebsd-nq
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix a pair of races in SIGIO registration

Browse Source 
First, funsetownlst() list looks at the first element of the list to see
whether it's processing a process or a process group list.  Then it
acquires the global sigio lock and processes the list.  However, nothing
prevents the first sigio tracker from being freed by a concurrent
funsetown() before the sigio lock is acquired.

Fix this by acquiring the global sigio lock immediately after checking
whether the list is empty.  Callers of funsetownlst() ensure that new
sigio trackers cannot be added concurrently.

Second, fsetown() uses funsetown() to remove an existing sigio structure
from a file object.  However, funsetown() uses a racy check to avoid the
sigio lock, so two threads may call fsetown() on the same file object,
both observe that no sigio tracker is present, and enqueue two sigio
trackers for the same file object.  However, if the file object is
destroyed, funsetown() will only remove one sigio tracker, and
funsetownlst() may later trigger a use-after-free when it clears the
file object reference for each entry in the list.

Fix this by introducing funsetown_locked(), which avoids the racy check.

Reviewed by:	kib
Reported by:	pho
Tested by:	pho
MFC after:	1 week
Sponsored by:	The FreeBSD Foundation
Differential Revision:	https://reviews.freebsd.org/D27157
This commit is contained in:
Mark Johnston 2020-11-11 13:44:27 +00:00
parent 26007fe37c
commit f52979098d
4 changed files with 87 additions and 76 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

156
sys/kern/kern_descrip.c
View File

@ -1001,6 +1001,40 @@ kern_dup(struct thread *td, u_int mode, int flags, int old, int new)
return (error);
}
static void
sigiofree(struct sigio *sigio)
{
crfree(sigio->sio_ucred);
free(sigio, M_SIGIO);
}
static struct sigio *
funsetown_locked(struct sigio *sigio)
{
struct proc *p;
struct pgrp *pg;
SIGIO_ASSERT_LOCKED();
if (sigio == NULL)
return (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);
PGRP_UNLOCK(pg);
} else {
p = sigio->sio_proc;
PROC_LOCK(p);
SLIST_REMOVE(&sigio->sio_proc->p_sigiolst, sigio,
sigio, sio_pgsigio);
PROC_UNLOCK(p);
}
return (sigio);
}
/*
* If sigio is on the list associated with a process or process group,
* disable signalling from the device, remove sigio from the list and
@ -1011,92 +1045,82 @@ funsetown(struct sigio **sigiop)
{
struct sigio *sigio;
/* Racy check, consumers must provide synchronization. */
if (*sigiop == NULL)
return;
SIGIO_LOCK();
sigio = *sigiop;
if (sigio == NULL) {
SIGIO_UNLOCK();
return;
}
*(sigio->sio_myref) = NULL;
if ((sigio)->sio_pgid < 0) {
struct pgrp *pg = (sigio)->sio_pgrp;
PGRP_LOCK(pg);
SLIST_REMOVE(&sigio->sio_pgrp->pg_sigiolst, sigio,
sigio, sio_pgsigio);
PGRP_UNLOCK(pg);
} else {
struct proc *p = (sigio)->sio_proc;
PROC_LOCK(p);
SLIST_REMOVE(&sigio->sio_proc->p_sigiolst, sigio,
sigio, sio_pgsigio);
PROC_UNLOCK(p);
}
sigio = funsetown_locked(*sigiop);
SIGIO_UNLOCK();
crfree(sigio->sio_ucred);
free(sigio, M_SIGIO);
if (sigio != NULL)
sigiofree(sigio);
}
/*
* Free a list of sigio structures.
* We only need to lock the SIGIO_LOCK because we have made ourselves
* inaccessible to callers of fsetown and therefore do not need to lock
* the proc or pgrp struct for the list manipulation.
* Free a list of sigio structures. The caller must ensure that new sigio
* structures cannot be added after this point. For process groups this is
* guaranteed using the proctree lock; for processes, the P_WEXIT flag serves
* as an interlock.
*/
void
funsetownlst(struct sigiolst *sigiolst)
{
struct proc *p;
struct pgrp *pg;
struct sigio *sigio;
struct sigio *sigio, *tmp;
/* Racy check. */
sigio = SLIST_FIRST(sigiolst);
if (sigio == NULL)
return;
p = NULL;
pg = NULL;
SIGIO_LOCK();
sigio = SLIST_FIRST(sigiolst);
if (sigio == NULL) {
SIGIO_UNLOCK();
return;
}
/*
* Every entry of the list should belong
* to a single proc or pgrp.
* Every entry of the list should belong to a single proc or pgrp.
*/
if (sigio->sio_pgid < 0) {
pg = sigio->sio_pgrp;
PGRP_LOCK_ASSERT(pg, MA_NOTOWNED);
sx_assert(&proctree_lock, SX_XLOCKED);
PGRP_LOCK(pg);
} else /* if (sigio->sio_pgid > 0) */ {
p = sigio->sio_proc;
PROC_LOCK_ASSERT(p, MA_NOTOWNED);
PROC_LOCK(p);
KASSERT((p->p_flag & P_WEXIT) != 0,
("%s: process %p is not exiting", __func__, p));
}
SIGIO_LOCK();
while ((sigio = SLIST_FIRST(sigiolst)) != NULL) {
*(sigio->sio_myref) = NULL;
SLIST_FOREACH(sigio, sigiolst, sio_pgsigio) {
*sigio->sio_myref = NULL;
if (pg != NULL) {
KASSERT(sigio->sio_pgid < 0,
("Proc sigio in pgrp sigio list"));
KASSERT(sigio->sio_pgrp == pg,
("Bogus pgrp in sigio list"));
PGRP_LOCK(pg);
SLIST_REMOVE(&pg->pg_sigiolst, sigio, sigio,
sio_pgsigio);
PGRP_UNLOCK(pg);
} else /* if (p != NULL) */ {
KASSERT(sigio->sio_pgid > 0,
("Pgrp sigio in proc sigio list"));
KASSERT(sigio->sio_proc == p,
("Bogus proc in sigio list"));
PROC_LOCK(p);
SLIST_REMOVE(&p->p_sigiolst, sigio, sigio,
sio_pgsigio);
PROC_UNLOCK(p);
}
SIGIO_UNLOCK();
crfree(sigio->sio_ucred);
free(sigio, M_SIGIO);
SIGIO_LOCK();
}
if (pg != NULL)
PGRP_UNLOCK(pg);
else
PROC_UNLOCK(p);
SIGIO_UNLOCK();
SLIST_FOREACH_SAFE(sigio, sigiolst, sio_pgsigio, tmp)
sigiofree(sigio);
}
/*
@ -1110,7 +1134,7 @@ fsetown(pid_t pgid, struct sigio **sigiop)
{
struct proc *proc;
struct pgrp *pgrp;
struct sigio *sigio;
struct sigio *osigio, *sigio;
int ret;
if (pgid == 0) {
@ -1120,13 +1144,14 @@ fsetown(pid_t pgid, struct sigio **sigiop)
ret = 0;
/* Allocate and fill in the new sigio out of locks. */
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) {
@ -1142,20 +1167,21 @@ fsetown(pid_t pgid, struct sigio **sigiop)
* restrict FSETOWN to the current process or process
* group for maximum safety.
*/
PROC_UNLOCK(proc);
if (proc->p_session != curthread->td_proc->p_session) {
PROC_UNLOCK(proc);
ret = EPERM;
goto fail;
}
pgrp = NULL;
sigio->sio_proc = proc;
SLIST_INSERT_HEAD(&proc->p_sigiolst, sigio, sio_pgsigio);
PROC_UNLOCK(proc);
} else /* if (pgid < 0) */ {
pgrp = pgfind(-pgid);
if (pgrp == NULL) {
ret = ESRCH;
goto fail;
}
PGRP_UNLOCK(pgrp);
/*
* Policy - Don't allow a process to FSETOWN a process
@ -1166,44 +1192,28 @@ fsetown(pid_t pgid, struct sigio **sigiop)
* group for maximum safety.
*/
if (pgrp->pg_session != curthread->td_proc->p_session) {
PGRP_UNLOCK(pgrp);
ret = EPERM;
goto fail;
}
proc = NULL;
}
funsetown(sigiop);
if (pgid > 0) {
PROC_LOCK(proc);
/*
* Since funsetownlst() is called without the proctree
* locked, we need to check for P_WEXIT.
* XXX: is ESRCH correct?
*/
if ((proc->p_flag & P_WEXIT) != 0) {
PROC_UNLOCK(proc);
ret = ESRCH;
goto fail;
}
SLIST_INSERT_HEAD(&proc->p_sigiolst, sigio, sio_pgsigio);
sigio->sio_proc = proc;
PROC_UNLOCK(proc);
} else {
PGRP_LOCK(pgrp);
SLIST_INSERT_HEAD(&pgrp->pg_sigiolst, sigio, sio_pgsigio);
sigio->sio_pgrp = pgrp;
PGRP_UNLOCK(pgrp);
}
sx_sunlock(&proctree_lock);
SIGIO_LOCK();
*sigiop = sigio;
SIGIO_UNLOCK();
if (osigio != NULL)
sigiofree(osigio);
return (0);
fail:
SIGIO_UNLOCK();
sx_sunlock(&proctree_lock);
crfree(sigio->sio_ucred);
free(sigio, M_SIGIO);
sigiofree(sigio);
if (osigio != NULL)
sigiofree(osigio);
return (ret);
}

2
sys/kern/kern_exit.c
View File

@ -358,7 +358,7 @@ exit1(struct thread *td, int rval, int signo)
/*
* Reset any sigio structures pointing to us as a result of
* F_SETOWN with our pid.
* F_SETOWN with our pid. The P_WEXIT flag interlocks with fsetown().
*/
funsetownlst(&p->p_sigiolst);

3
sys/kern/kern_proc.c
View File

@ -774,7 +774,8 @@ pgdelete(struct pgrp *pgrp)
/*
* Reset any sigio structures pointing to us as a result of
* F_SETOWN with our pgid.
* F_SETOWN with our pgid. The proctree lock ensures that
* new sigio structures will not be added after this point.
*/
funsetownlst(&pgrp->pg_sigiolst);

2
sys/sys/signalvar.h
View File

@ -337,7 +337,7 @@ struct thread;
#define SIGIO_TRYLOCK() mtx_trylock(&sigio_lock)
#define SIGIO_UNLOCK() mtx_unlock(&sigio_lock)
#define SIGIO_LOCKED() mtx_owned(&sigio_lock)
#define SIGIO_ASSERT(type) mtx_assert(&sigio_lock, type)
#define SIGIO_ASSERT_LOCKED(type) mtx_assert(&sigio_lock, MA_OWNED)
extern struct mtx sigio_lock;