Author: tuexen
Date: Sun Jul 19 12:34:19 2020
New Revision: 363323
URL: https://svnweb.freebsd.org/changeset/base/363323
Log:
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
Modified:
head/sys/netinet/sctp_os_bsd.h
head/sys/netinet/sctp_pcb.c
head/sys/netinet/sctp_var.h
head/sys/netinet/sctputil.c
head/sys/netinet/sctputil.h
Modified: head/sys/netinet/sctp_os_bsd.h
==============================================================================
--- head/sys/netinet/sctp_os_bsd.h Sun Jul 19 12:25:03 2020
(r363322)
+++ head/sys/netinet/sctp_os_bsd.h Sun Jul 19 12:34:19 2020
(r363323)
@@ -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
-#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
+/*
+ * NOTE: The next two shouldn't be called directly outside of
sctp_timer_start()
+ * and sctp_timer_stop(), since they don't handle incrementing/decrementing
+ * relevant reference counts.
+ */
+#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)
Modified: head/sys/netinet/sctp_pcb.c
==============================================================================
--- head/sys/netinet/sctp_pcb.c Sun Jul 19 12:25:03 2020 (r363322)
+++ head/sys/netinet/sctp_pcb.c Sun Jul 19 12:34:19 2020 (r363323)
@@ -2739,23 +2739,54 @@ sctp_move_pcb_and_assoc(struct sctp_inpcb *old_inp, st
}
}
}
- /*
- * Now any running timers need to be adjusted since we really don't
- * care if they are running or not just blast in the new_inp into
- * all of them.
- */
-
- stcb->asoc.dack_timer.ep = (void *)new_inp;
- stcb->asoc.asconf_timer.ep = (void *)new_inp;
- stcb->asoc.strreset_timer.ep = (void *)new_inp;
- stcb->asoc.shut_guard_timer.ep = (void *)new_inp;
- stcb->asoc.autoclose_timer.ep = (void *)new_inp;
- stcb->asoc.delete_prim_timer.ep = (void *)new_inp;
+ /* Now any running timers need to be adjusted. */
+ if (stcb->asoc.dack_timer.ep == old_inp) {
+ SCTP_INP_DECR_REF(old_inp);
+ stcb->asoc.dack_timer.ep = new_inp;
+ SCTP_INP_INCR_REF(new_inp);
+ }
+ if (stcb->asoc.asconf_timer.ep == old_inp) {
+ SCTP_INP_DECR_REF(old_inp);
+ stcb->asoc.asconf_timer.ep = new_inp;
+ SCTP_INP_INCR_REF(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;
- net->hb_timer.ep = (void *)new_inp;
- net->rxt_timer.ep = (void *)new_inp;
+ if (net->pmtu_timer.ep == old_inp) {
+ SCTP_INP_DECR_REF(old_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,
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,
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);
Modified: head/sys/netinet/sctp_var.h
==============================================================================
--- head/sys/netinet/sctp_var.h Sun Jul 19 12:25:03 2020 (r363322)
+++ head/sys/netinet/sctp_var.h Sun Jul 19 12:34:19 2020 (r363323)
@@ -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); \
Modified: head/sys/netinet/sctputil.c
==============================================================================
--- head/sys/netinet/sctputil.c Sun Jul 19 12:25:03 2020 (r363322)
+++ head/sys/netinet/sctputil.c Sun Jul 19 12:34:19 2020 (r363323)
@@ -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) {
- atomic_add_int(&stcb->asoc.refcnt, 1);
- if (stcb->asoc.state == 0) {
- atomic_add_int(&stcb->asoc.refcnt, -1);
- if (inp) {
- SCTP_INP_DECR_REF(inp);
- }
- SCTPDBG(SCTP_DEBUG_TIMER2,
- "Timer type %d handler exiting due to CLOSED
association.\n",
- type);
- CURVNET_RESTORE();
- return;
- }
+ if ((stcb != NULL) && (stcb->asoc.state == SCTP_STATE_EMPTY)) {
+ SCTPDBG(SCTP_DEBUG_TIMER2,
+ "Timer type %d handler exiting due to CLOSED
association.\n",
+ type);
+ goto out_decr;
}
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();
- return;
+ goto out_decr;
}
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();
- return;
+ goto out;
}
} 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 = 0;
+ did_output = false;
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:
- if (stcb) {
+out:
+ if (stcb != NULL) {
SCTP_TCB_UNLOCK(stcb);
} else if (inp != NULL) {
SCTP_INP_WUNLOCK(inp);
@@ -2101,10 +2103,16 @@ get_out:
}
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, s
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, st
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;
- tmr->net = NULL;
+ /*
+ * If the timer was actually stopped, decrement reference
+ * 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",
Modified: head/sys/netinet/sctputil.h
==============================================================================
--- head/sys/netinet/sctputil.h Sun Jul 19 12:25:03 2020 (r363322)
+++ head/sys/netinet/sctputil.h Sun Jul 19 12:34:19 2020 (r363323)
@@ -90,6 +90,14 @@ sctp_notify_stream_reset_add(struct sctp_tcb *stcb, ui
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 *);
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
