Add reference counts for inp/stcb/net when timers are running.

This avoids a use-after-free reported for the userland stack.
Thanks to Taylor Brandstetter for suggesting a patch for
the userland stack.

MFC after:		1 week

sys/netinet/sctp_os_bsd.h

@@ -269,12 +269,17 @@ typedef struct callout sctp_os_timer_t;
 
 
 #define SCTP_OS_TIMER_INIT(tmr)	callout_init(tmr, 1)
-#define SCTP_OS_TIMER_START	callout_reset
+/*
-#define SCTP_OS_TIMER_STOP	callout_stop
+ * NOTE: The next two shouldn't be called directly outside of sctp_timer_start()
-#define SCTP_OS_TIMER_STOP_DRAIN callout_drain
+ * and sctp_timer_stop(), since they don't handle incrementing/decrementing
-#define SCTP_OS_TIMER_PENDING	callout_pending
+ * relevant reference counts.
-#define SCTP_OS_TIMER_ACTIVE	callout_active
+ */
-#define SCTP_OS_TIMER_DEACTIVATE callout_deactivate
+#define SCTP_OS_TIMER_START		callout_reset
+#define SCTP_OS_TIMER_STOP		callout_stop
+#define SCTP_OS_TIMER_STOP_DRAIN	callout_drain
+#define SCTP_OS_TIMER_PENDING		callout_pending
+#define SCTP_OS_TIMER_ACTIVE		callout_active
+#define SCTP_OS_TIMER_DEACTIVATE	callout_deactivate
 
 #define sctp_get_tick_count() (ticks)
 

sys/netinet/sctp_pcb.c

@@ -2739,23 +2739,54 @@ sctp_move_pcb_and_assoc(struct sctp_inpcb *old_inp, struct sctp_inpcb *new_inp,
 			}
 		}
 	}
-	/*
+	/* Now any running timers need to be adjusted. */
-	 * Now any running timers need to be adjusted since we really don't
+	if (stcb->asoc.dack_timer.ep == old_inp) {
-	 * care if they are running or not just blast in the new_inp into
+		SCTP_INP_DECR_REF(old_inp);
-	 * all of them.
+		stcb->asoc.dack_timer.ep = new_inp;
-	 */
+		SCTP_INP_INCR_REF(new_inp);
-
+	}
-	stcb->asoc.dack_timer.ep = (void *)new_inp;
+	if (stcb->asoc.asconf_timer.ep == old_inp) {
-	stcb->asoc.asconf_timer.ep = (void *)new_inp;
+		SCTP_INP_DECR_REF(old_inp);
-	stcb->asoc.strreset_timer.ep = (void *)new_inp;
+		stcb->asoc.asconf_timer.ep = new_inp;
-	stcb->asoc.shut_guard_timer.ep = (void *)new_inp;
+		SCTP_INP_INCR_REF(new_inp);
-	stcb->asoc.autoclose_timer.ep = (void *)new_inp;
+	}
-	stcb->asoc.delete_prim_timer.ep = (void *)new_inp;
+	if (stcb->asoc.strreset_timer.ep == old_inp) {
+		SCTP_INP_DECR_REF(old_inp);
+		stcb->asoc.strreset_timer.ep = new_inp;
+		SCTP_INP_INCR_REF(new_inp);
+	}
+	if (stcb->asoc.shut_guard_timer.ep == old_inp) {
+		SCTP_INP_DECR_REF(old_inp);
+		stcb->asoc.shut_guard_timer.ep = new_inp;
+		SCTP_INP_INCR_REF(new_inp);
+	}
+	if (stcb->asoc.autoclose_timer.ep == old_inp) {
+		SCTP_INP_DECR_REF(old_inp);
+		stcb->asoc.autoclose_timer.ep = new_inp;
+		SCTP_INP_INCR_REF(new_inp);
+	}
+	if (stcb->asoc.delete_prim_timer.ep == old_inp) {
+		SCTP_INP_DECR_REF(old_inp);
+		stcb->asoc.delete_prim_timer.ep = new_inp;
+		SCTP_INP_INCR_REF(new_inp);
+	}
 	/* now what about the nets? */
 	TAILQ_FOREACH(net, &stcb->asoc.nets, sctp_next) {
-		net->pmtu_timer.ep = (void *)new_inp;
+		if (net->pmtu_timer.ep == old_inp) {
-		net->hb_timer.ep = (void *)new_inp;
+			SCTP_INP_DECR_REF(old_inp);
-		net->rxt_timer.ep = (void *)new_inp;
+			net->pmtu_timer.ep = new_inp;
+			SCTP_INP_INCR_REF(new_inp);
+		}
+		if (net->hb_timer.ep == old_inp) {
+			SCTP_INP_DECR_REF(old_inp);
+			net->hb_timer.ep = new_inp;
+			SCTP_INP_INCR_REF(new_inp);
+		}
+		if (net->rxt_timer.ep == old_inp) {
+			SCTP_INP_DECR_REF(old_inp);
+			net->rxt_timer.ep = new_inp;
+			SCTP_INP_INCR_REF(new_inp);
+		}
 	}
 	SCTP_INP_WUNLOCK(new_inp);
 	SCTP_INP_WUNLOCK(old_inp);
@@ -3562,7 +3593,7 @@ sctp_inpcb_free(struct sctp_inpcb *inp, int immediate, int from)
 		being_refed++;
 	if (SCTP_ASOC_CREATE_LOCK_CONTENDED(inp))
 		being_refed++;
-
+	/* NOTE: 0 refcount also means no timers are referencing us. */
 	if ((inp->refcount) ||
 	    (being_refed) ||
 	    (inp->sctp_flags & SCTP_PCB_FLAGS_CLOSE_IP)) {
@@ -3584,16 +3615,6 @@ sctp_inpcb_free(struct sctp_inpcb *inp, int immediate, int from)
 	SCTP_INP_WUNLOCK(inp);
 	SCTP_ASOC_CREATE_UNLOCK(inp);
 	SCTP_INP_INFO_WUNLOCK();
-	/*
-	 * Now we release all locks. Since this INP cannot be found anymore
-	 * except possibly by the kill timer that might be running. We call
-	 * the drain function here. It should hit the case were it sees the
-	 * ACTIVE flag cleared and exit out freeing us to proceed and
-	 * destroy everything.
-	 */
-	if (from != SCTP_CALLED_FROM_INPKILL_TIMER) {
-		(void)SCTP_OS_TIMER_STOP_DRAIN(&inp->sctp_ep.signature_change.timer);
-	}
 
 #ifdef SCTP_LOG_CLOSING
 	sctp_log_closing(inp, NULL, 5);

sys/netinet/sctp_var.h

@@ -186,7 +186,6 @@ extern struct pr_usrreqs sctp_usrreqs;
 #define sctp_free_remote_addr(__net) { \
 	if ((__net)) {  \
 		if (SCTP_DECREMENT_AND_CHECK_REFCOUNT(&(__net)->ref_count)) { \
-			(void)SCTP_OS_TIMER_STOP(&(__net)->rxt_timer.timer); \
 			RO_NHFREE(&(__net)->ro); \
 			if ((__net)->src_addr_selected) { \
 				sctp_free_ifa((__net)->ro._s_addr); \

sys/netinet/sctputil.c

@@ -1713,16 +1713,22 @@ sctp_timeout_handler(void *t)
 	struct sctp_nets *net;
 	struct sctp_timer *tmr;
 	struct mbuf *op_err;
-	int did_output;
 	int type;
 	int i, secret;
+	bool did_output, released_asoc_reference;
 
+	/*
+	 * If inp, stcb or net are not NULL, then references to these were
+	 * added when the timer was started, and must be released before
+	 * this function returns.
+	 */
 	tmr = (struct sctp_timer *)t;
 	inp = (struct sctp_inpcb *)tmr->ep;
 	stcb = (struct sctp_tcb *)tmr->tcb;
 	net = (struct sctp_nets *)tmr->net;
 	CURVNET_SET((struct vnet *)tmr->vnet);
 	did_output = 1;
+	released_asoc_reference = false;
 
 #ifdef SCTP_AUDITING_ENABLED
 	sctp_audit_log(0xF0, (uint8_t)tmr->type);
@@ -1738,56 +1744,39 @@ sctp_timeout_handler(void *t)
 	KASSERT(stcb == NULL || stcb->sctp_ep == inp,
 	    ("sctp_timeout_handler of type %d: inp = %p, stcb->sctp_ep %p",
 	    type, stcb, stcb->sctp_ep));
-	if (inp) {
-		SCTP_INP_INCR_REF(inp);
-	}
 	tmr->stopped_from = 0xa001;
-	if (stcb) {
+	if ((stcb != NULL) && (stcb->asoc.state == SCTP_STATE_EMPTY)) {
-		atomic_add_int(&stcb->asoc.refcnt, 1);
+		SCTPDBG(SCTP_DEBUG_TIMER2,
-		if (stcb->asoc.state == 0) {
+		    "Timer type %d handler exiting due to CLOSED association.\n",
-			atomic_add_int(&stcb->asoc.refcnt, -1);
+		    type);
-			if (inp) {
+		goto out_decr;
-				SCTP_INP_DECR_REF(inp);
-			}
-			SCTPDBG(SCTP_DEBUG_TIMER2,
-			    "Timer type %d handler exiting due to CLOSED association.\n",
-			    type);
-			CURVNET_RESTORE();
-			return;
-		}
 	}
 	tmr->stopped_from = 0xa002;
 	SCTPDBG(SCTP_DEBUG_TIMER2, "Timer type %d goes off.\n", type);
 	if (!SCTP_OS_TIMER_ACTIVE(&tmr->timer)) {
-		if (inp) {
-			SCTP_INP_DECR_REF(inp);
-		}
-		if (stcb) {
-			atomic_add_int(&stcb->asoc.refcnt, -1);
-		}
 		SCTPDBG(SCTP_DEBUG_TIMER2,
 		    "Timer type %d handler exiting due to not being active.\n",
 		    type);
-		CURVNET_RESTORE();
+		goto out_decr;
-		return;
 	}
 
 	tmr->stopped_from = 0xa003;
 	if (stcb) {
 		SCTP_TCB_LOCK(stcb);
+		/*
+		 * Release reference so that association can be freed if
+		 * necessary below. This is safe now that we have acquired
+		 * the lock.
+		 */
 		atomic_add_int(&stcb->asoc.refcnt, -1);
+		released_asoc_reference = true;
 		if ((type != SCTP_TIMER_TYPE_ASOCKILL) &&
-		    ((stcb->asoc.state == 0) ||
+		    ((stcb->asoc.state == SCTP_STATE_EMPTY) ||
 		    (stcb->asoc.state & SCTP_STATE_ABOUT_TO_BE_FREED))) {
-			SCTP_TCB_UNLOCK(stcb);
-			if (inp) {
-				SCTP_INP_DECR_REF(inp);
-			}
 			SCTPDBG(SCTP_DEBUG_TIMER2,
 			    "Timer type %d handler exiting due to CLOSED association.\n",
 			    type);
-			CURVNET_RESTORE();
+			goto out;
-			return;
 		}
 	} else if (inp != NULL) {
 		SCTP_INP_WLOCK(inp);
@@ -1803,13 +1792,13 @@ sctp_timeout_handler(void *t)
 		/*
 		 * Callout has been rescheduled.
 		 */
-		goto get_out;
+		goto out;
 	}
 	if (!SCTP_OS_TIMER_ACTIVE(&tmr->timer)) {
 		/*
 		 * Not active, so no action.
 		 */
-		goto get_out;
+		goto out;
 	}
 	SCTP_OS_TIMER_DEACTIVATE(&tmr->timer);
 
@@ -1836,6 +1825,7 @@ sctp_timeout_handler(void *t)
 		sctp_auditing(4, inp, stcb, net);
 #endif
 		sctp_chunk_output(inp, stcb, SCTP_OUTPUT_FROM_T3, SCTP_SO_NOT_LOCKED);
+		did_output = true;
 		if ((stcb->asoc.num_send_timers_up == 0) &&
 		    (stcb->asoc.sent_queue_cnt > 0)) {
 			struct sctp_tmit_chunk *chk;
@@ -1866,8 +1856,7 @@ sctp_timeout_handler(void *t)
 			/* no need to unlock on tcb its gone */
 			goto out_decr;
 		}
-		/* We do output but not here */
+		did_output = false;
-		did_output = 0;
 		break;
 	case SCTP_TIMER_TYPE_RECV:
 		KASSERT(inp != NULL && stcb != NULL && net == NULL,
@@ -1880,6 +1869,7 @@ sctp_timeout_handler(void *t)
 		sctp_auditing(4, inp, stcb, NULL);
 #endif
 		sctp_chunk_output(inp, stcb, SCTP_OUTPUT_FROM_SACK_TMR, SCTP_SO_NOT_LOCKED);
+		did_output = true;
 		break;
 	case SCTP_TIMER_TYPE_SHUTDOWN:
 		KASSERT(inp != NULL && stcb != NULL && net != NULL,
@@ -1895,6 +1885,7 @@ sctp_timeout_handler(void *t)
 		sctp_auditing(4, inp, stcb, net);
 #endif
 		sctp_chunk_output(inp, stcb, SCTP_OUTPUT_FROM_SHUT_TMR, SCTP_SO_NOT_LOCKED);
+		did_output = true;
 		break;
 	case SCTP_TIMER_TYPE_HEARTBEAT:
 		KASSERT(inp != NULL && stcb != NULL && net != NULL,
@@ -1912,6 +1903,9 @@ sctp_timeout_handler(void *t)
 		if (!(net->dest_state & SCTP_ADDR_NOHB)) {
 			sctp_timer_start(SCTP_TIMER_TYPE_HEARTBEAT, inp, stcb, net);
 			sctp_chunk_output(inp, stcb, SCTP_OUTPUT_FROM_HB_TMR, SCTP_SO_NOT_LOCKED);
+			did_output = true;
+		} else {
+			did_output = false;
 		}
 		break;
 	case SCTP_TIMER_TYPE_COOKIE:
@@ -1932,6 +1926,7 @@ sctp_timeout_handler(void *t)
 		 * respect to where from in chunk_output.
 		 */
 		sctp_chunk_output(inp, stcb, SCTP_OUTPUT_FROM_T3, SCTP_SO_NOT_LOCKED);
+		did_output = true;
 		break;
 	case SCTP_TIMER_TYPE_NEWCOOKIE:
 		KASSERT(inp != NULL && stcb == NULL && net == NULL,
@@ -1953,7 +1948,7 @@ sctp_timeout_handler(void *t)
 			    sctp_select_initial_TSN(&inp->sctp_ep);
 		}
 		sctp_timer_start(SCTP_TIMER_TYPE_NEWCOOKIE, inp, NULL, NULL);
-		did_output = 0;
+		did_output = false;
 		break;
 	case SCTP_TIMER_TYPE_PATHMTURAISE:
 		KASSERT(inp != NULL && stcb != NULL && net != NULL,
@@ -1961,7 +1956,7 @@ sctp_timeout_handler(void *t)
 		    type, inp, stcb, net));
 		SCTP_STAT_INCR(sctps_timopathmtu);
 		sctp_pathmtu_timer(inp, stcb, net);
-		did_output = 0;
+		did_output = false;
 		break;
 	case SCTP_TIMER_TYPE_SHUTDOWNACK:
 		KASSERT(inp != NULL && stcb != NULL && net != NULL,
@@ -1977,6 +1972,7 @@ sctp_timeout_handler(void *t)
 		sctp_auditing(4, inp, stcb, net);
 #endif
 		sctp_chunk_output(inp, stcb, SCTP_OUTPUT_FROM_SHUT_ACK_TMR, SCTP_SO_NOT_LOCKED);
+		did_output = true;
 		break;
 	case SCTP_TIMER_TYPE_ASCONF:
 		KASSERT(inp != NULL && stcb != NULL && net != NULL,
@@ -1991,6 +1987,7 @@ sctp_timeout_handler(void *t)
 		sctp_auditing(4, inp, stcb, net);
 #endif
 		sctp_chunk_output(inp, stcb, SCTP_OUTPUT_FROM_ASCONF_TMR, SCTP_SO_NOT_LOCKED);
+		did_output = true;
 		break;
 	case SCTP_TIMER_TYPE_SHUTDOWNGUARD:
 		KASSERT(inp != NULL && stcb != NULL && net == NULL,
@@ -2000,6 +1997,7 @@ sctp_timeout_handler(void *t)
 		op_err = sctp_generate_cause(SCTP_BASE_SYSCTL(sctp_diag_info_code),
 		    "Shutdown guard timer expired");
 		sctp_abort_an_association(inp, stcb, op_err, SCTP_SO_NOT_LOCKED);
+		did_output = true;
 		/* no need to unlock on tcb its gone */
 		goto out_decr;
 	case SCTP_TIMER_TYPE_AUTOCLOSE:
@@ -2009,7 +2007,7 @@ sctp_timeout_handler(void *t)
 		SCTP_STAT_INCR(sctps_timoautoclose);
 		sctp_autoclose_timer(inp, stcb);
 		sctp_chunk_output(inp, stcb, SCTP_OUTPUT_FROM_AUTOCLOSE_TMR, SCTP_SO_NOT_LOCKED);
-		did_output = 0;
+		did_output = true;
 		break;
 	case SCTP_TIMER_TYPE_STRRESET:
 		KASSERT(inp != NULL && stcb != NULL && net == NULL,
@@ -2021,6 +2019,7 @@ sctp_timeout_handler(void *t)
 			goto out_decr;
 		}
 		sctp_chunk_output(inp, stcb, SCTP_OUTPUT_FROM_STRRST_TMR, SCTP_SO_NOT_LOCKED);
+		did_output = true;
 		break;
 	case SCTP_TIMER_TYPE_INPKILL:
 		KASSERT(inp != NULL && stcb == NULL && net == NULL,
@@ -2061,6 +2060,7 @@ sctp_timeout_handler(void *t)
 		    ("timeout of type %d: inp = %p, stcb = %p, net = %p",
 		    type, inp, stcb, net));
 		sctp_handle_addr_wq();
+		did_output = true;
 		break;
 	case SCTP_TIMER_TYPE_PRIM_DELETED:
 		KASSERT(inp != NULL && stcb != NULL && net == NULL,
@@ -2068,20 +2068,22 @@ sctp_timeout_handler(void *t)
 		    type, inp, stcb, net));
 		SCTP_STAT_INCR(sctps_timodelprim);
 		sctp_delete_prim_timer(inp, stcb);
+		did_output = false;
 		break;
 	default:
 #ifdef INVARIANTS
 		panic("Unknown timer type %d", type);
 #else
-		goto get_out;
+		did_output = false;
+		goto out;
 #endif
 	}
 #ifdef SCTP_AUDITING_ENABLED
 	sctp_audit_log(0xF1, (uint8_t)type);
-	if (inp)
+	if (inp != NULL)
 		sctp_auditing(5, inp, stcb, net);
 #endif
-	if ((did_output) && stcb) {
+	if (did_output && (stcb != NULL)) {
 		/*
 		 * Now we need to clean up the control chunk chain if an
 		 * ECNE is on it. It must be marked as UNSENT again so next
@@ -2091,8 +2093,8 @@ sctp_timeout_handler(void *t)
 		 */
 		sctp_fix_ecn_echo(&stcb->asoc);
 	}
-get_out:
+out:
-	if (stcb) {
+	if (stcb != NULL) {
 		SCTP_TCB_UNLOCK(stcb);
 	} else if (inp != NULL) {
 		SCTP_INP_WUNLOCK(inp);
@@ -2101,10 +2103,16 @@ sctp_timeout_handler(void *t)
 	}
 
 out_decr:
-	if (inp) {
+	/* These reference counts were incremented in sctp_timer_start(). */
+	if (inp != NULL) {
 		SCTP_INP_DECR_REF(inp);
 	}
-
+	if ((stcb != NULL) && !released_asoc_reference) {
+		atomic_add_int(&stcb->asoc.refcnt, -1);
+	}
+	if (net != NULL) {
+		sctp_free_remote_addr(net);
+	}
 out_no_decr:
 	SCTPDBG(SCTP_DEBUG_TIMER2, "Timer type %d handler finished.\n", type);
 	CURVNET_RESTORE();
@@ -2538,6 +2546,19 @@ sctp_timer_start(int t_type, struct sctp_inpcb *inp, struct sctp_tcb *stcb,
 		SCTPDBG(SCTP_DEBUG_TIMER2,
 		    "Timer type %d started: ticks=%u, inp=%p, stcb=%p, net=%p.\n",
 		    t_type, to_ticks, inp, stcb, net);
+		/*
+		 * If this is a newly scheduled callout, as opposed to a
+		 * rescheduled one, increment relevant reference counts.
+		 */
+		if (tmr->ep != NULL) {
+			SCTP_INP_INCR_REF(inp);
+		}
+		if (tmr->tcb != NULL) {
+			atomic_add_int(&stcb->asoc.refcnt, 1);
+		}
+		if (tmr->net != NULL) {
+			atomic_add_int(&net->ref_count, 1);
+		}
 	} else {
 		/*
 		 * This should not happen, since we checked for pending
@@ -2830,9 +2851,26 @@ sctp_timer_stop(int t_type, struct sctp_inpcb *inp, struct sctp_tcb *stcb,
 		SCTPDBG(SCTP_DEBUG_TIMER2,
 		    "Timer type %d stopped: inp=%p, stcb=%p, net=%p.\n",
 		    t_type, inp, stcb, net);
-		tmr->ep = NULL;
+		/*
-		tmr->tcb = NULL;
+		 * If the timer was actually stopped, decrement reference
-		tmr->net = NULL;
+		 * counts that were incremented in sctp_timer_start().
+		 */
+		if (tmr->ep != NULL) {
+			SCTP_INP_DECR_REF(inp);
+			tmr->ep = NULL;
+		}
+		if (tmr->tcb != NULL) {
+			atomic_add_int(&stcb->asoc.refcnt, -1);
+			tmr->tcb = NULL;
+		}
+		if (tmr->net != NULL) {
+			/*
+			 * Can't use net, since it doesn't work for
+			 * SCTP_TIMER_TYPE_ASCONF.
+			 */
+			sctp_free_remote_addr((struct sctp_nets *)tmr->net);
+			tmr->net = NULL;
+		}
 	} else {
 		SCTPDBG(SCTP_DEBUG_TIMER2,
 		    "Timer type %d not stopped: inp=%p, stcb=%p, net=%p.\n",

sys/netinet/sctputil.h

@@ -90,6 +90,14 @@ sctp_notify_stream_reset_add(struct sctp_tcb *stcb, uint16_t numberin,
 void
      sctp_notify_stream_reset_tsn(struct sctp_tcb *stcb, uint32_t sending_tsn, uint32_t recv_tsn, int flag);
 
+/*
+ * NOTE: sctp_timer_start() will increment the reference count of any relevant
+ * structure the timer is referencing, in order to prevent a race condition
+ * between the timer executing and the structure being freed.
+ *
+ * When the timer fires or sctp_timer_stop() is called, these references are
+ * removed.
+ */
 void
 sctp_timer_start(int, struct sctp_inpcb *, struct sctp_tcb *,
     struct sctp_nets *);