From 879edb36e9a953e453e7b19ac6c72ad3ba21655e Mon Sep 17 00:00:00 2001 From: mpi Date: Mon, 26 Feb 2018 13:33:25 +0000 Subject: [PATCH] Fix a TOCTOU race that causes signals to be delivered more than once. The race is only triggerable if one of the threads of a multi-threaded program is in the middle of a NOLOCK syscall when a signal is received. The problem is that `ps_sigact' is shared between threads so its access should be serialized. In the case of SA_RESETHAND, the handler is reset when a signal is delivered, so delivering the signal twice would put the process in an "impossible" state where some threads were stopped and some were waiting for the others to die. Serialize signal checking & processing with the KERNEL_LOCK() for now, and introduce postsig_done() gypped from FreeBSD, to make sure the lock is held when resetting the handler. Bug report from espie@, ok visa@ --- sys/kern/kern_sig.c | 75 +++++++++++++++++++++++++-------------------- sys/sys/signalvar.h | 3 +- 2 files changed, 42 insertions(+), 36 deletions(-) diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index cfa2df78e74..5e8153a90a6 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -1,4 +1,4 @@ -/* $OpenBSD: kern_sig.c,v 1.215 2018/02/19 08:59:52 mpi Exp $ */ +/* $OpenBSD: kern_sig.c,v 1.216 2018/02/26 13:33:25 mpi Exp $ */ /* $NetBSD: kern_sig.c,v 1.54 1996/04/22 01:38:32 christos Exp $ */ /* @@ -82,6 +82,7 @@ void proc_stop(struct proc *p, int); void proc_stop_sweep(void *); struct timeout proc_stop_to; +void postsig(struct proc *, int); int cansignal(struct proc *, struct process *, int); struct pool sigacts_pool; /* memory pool for sigacts structures */ @@ -746,6 +747,27 @@ pgsignal(struct pgrp *pgrp, int signum, int checkctty) prsignal(pr, signum); } +/* + * Recalculate the signal mask and reset the signal disposition after + * usermode frame for delivery is formed. + */ +void +postsig_done(struct proc *p, int signum, struct sigacts *ps) +{ + int mask = sigmask(signum); + + KERNEL_ASSERT_LOCKED(); + + p->p_ru.ru_nsignals++; + atomic_setbits_int(&p->p_sigmask, ps->ps_catchmask[signum]); + if ((ps->ps_sigreset & mask) != 0) { + ps->ps_sigcatch &= ~mask; + if (signum != SIGCONT && sigprop[signum] & SA_IGNORE) + ps->ps_sigignore |= mask; + ps->ps_sigact[signum] = SIG_DFL; + } +} + /* * Send a signal caused by a trap to the current thread * If it will be caught immediately, deliver it with correct code. @@ -780,16 +802,9 @@ trapsignal(struct proc *p, int signum, u_long trapno, int code, p->p_sigmask, code, &si); } #endif - p->p_ru.ru_nsignals++; (*pr->ps_emul->e_sendsig)(ps->ps_sigact[signum], signum, p->p_sigmask, trapno, code, sigval); - atomic_setbits_int(&p->p_sigmask, ps->ps_catchmask[signum]); - if ((ps->ps_sigreset & mask) != 0) { - ps->ps_sigcatch &= ~mask; - if (signum != SIGCONT && sigprop[signum] & SA_IGNORE) - ps->ps_sigignore |= mask; - ps->ps_sigact[signum] = SIG_DFL; - } + postsig_done(p, signum, ps); } else { p->p_sisig = signum; p->p_sitrapno = trapno; /* XXX for core dump/debugger */ @@ -1330,9 +1345,8 @@ proc_stop_sweep(void *v) * from the current set of pending signals. */ void -postsig(int signum) +postsig(struct proc *p, int signum) { - struct proc *p = curproc; struct process *pr = p->p_p; struct sigacts *ps = pr->ps_sigacts; sig_t action; @@ -1341,12 +1355,8 @@ postsig(int signum) union sigval sigval; int s, code; -#ifdef DIAGNOSTIC - if (signum == 0) - panic("postsig"); -#endif - - KERNEL_LOCK(); + KASSERT(signum != 0); + KERNEL_ASSERT_LOCKED(); mask = sigmask(signum); atomic_clearbits_int(&p->p_siglist, mask); @@ -1366,7 +1376,7 @@ postsig(int signum) #ifdef KTRACE if (KTRPOINT(p, KTR_PSIG)) { siginfo_t si; - + initsiginfo(&si, signum, trapno, code, sigval); ktrpsig(p, signum, action, p->p_flag & P_SIGSUSPEND ? p->p_oldmask : p->p_sigmask, code, &si); @@ -1407,15 +1417,6 @@ postsig(int signum) } else { returnmask = p->p_sigmask; } - atomic_setbits_int(&p->p_sigmask, ps->ps_catchmask[signum]); - if ((ps->ps_sigreset & mask) != 0) { - ps->ps_sigcatch &= ~mask; - if (signum != SIGCONT && sigprop[signum] & SA_IGNORE) - ps->ps_sigignore |= mask; - ps->ps_sigact[signum] = SIG_DFL; - } - splx(s); - p->p_ru.ru_nsignals++; if (p->p_sisig == signum) { p->p_sisig = 0; p->p_sitrapno = 0; @@ -1425,9 +1426,9 @@ postsig(int signum) (*pr->ps_emul->e_sendsig)(action, signum, returnmask, trapno, code, sigval); + postsig_done(p, signum, ps); + splx(s); } - - KERNEL_UNLOCK(); } /* @@ -1816,7 +1817,7 @@ filt_signal(struct knote *kn, long hint) void userret(struct proc *p) { - int sig; + int signum; /* send SIGPROF or SIGVTALRM if their timers interrupted this thread */ if (p->p_flag & P_PROFPEND) { @@ -1832,8 +1833,12 @@ userret(struct proc *p) KERNEL_UNLOCK(); } - while ((sig = CURSIG(p)) != 0) - postsig(sig); + if (CURSIG(p) != 0) { + KERNEL_LOCK(); + while ((signum = CURSIG(p)) != 0) + postsig(p, signum); + KERNEL_UNLOCK(); + } /* * If P_SIGSUSPEND is still set here, then we still need to restore @@ -1845,8 +1850,10 @@ userret(struct proc *p) atomic_clearbits_int(&p->p_flag, P_SIGSUSPEND); p->p_sigmask = p->p_oldmask; - while ((sig = CURSIG(p)) != 0) - postsig(sig); + KERNEL_LOCK(); + while ((signum = CURSIG(p)) != 0) + postsig(p, signum); + KERNEL_UNLOCK(); } if (p->p_flag & P_SUSPSINGLE) { diff --git a/sys/sys/signalvar.h b/sys/sys/signalvar.h index a38788bbd98..5b576e929b4 100644 --- a/sys/sys/signalvar.h +++ b/sys/sys/signalvar.h @@ -1,4 +1,4 @@ -/* $OpenBSD: signalvar.h,v 1.28 2015/05/05 02:13:46 guenther Exp $ */ +/* $OpenBSD: signalvar.h,v 1.29 2018/02/26 13:33:25 mpi Exp $ */ /* $NetBSD: signalvar.h,v 1.17 1996/04/22 01:23:31 christos Exp $ */ /* @@ -151,7 +151,6 @@ void gsignal(int pgid, int sig); void csignal(pid_t pgid, int signum, uid_t uid, uid_t euid); int issignal(struct proc *p); void pgsignal(struct pgrp *pgrp, int sig, int checkctty); -void postsig(int sig); void psignal(struct proc *p, int sig); void ptsignal(struct proc *p, int sig, enum signal_type type); #define prsignal(pr,sig) ptsignal((pr)->ps_mainproc, (sig), SPROCESS) -- 2.20.1