Date: Mon, 24 Aug 2020 09:13:06 +0000 (UTC) From: Michael Tuexen <tuexen@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: svn commit: r364641 - stable/12/sys/netinet Message-ID: <202008240913.07O9D60D003191@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: tuexen Date: Mon Aug 24 09:13:06 2020 New Revision: 364641 URL: https://svnweb.freebsd.org/changeset/base/364641 Log: MFC r363323: 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. Modified: stable/12/sys/netinet/sctp_os_bsd.h stable/12/sys/netinet/sctp_pcb.c stable/12/sys/netinet/sctp_var.h stable/12/sys/netinet/sctputil.c stable/12/sys/netinet/sctputil.h Directory Properties: stable/12/ (props changed) Modified: stable/12/sys/netinet/sctp_os_bsd.h ============================================================================== --- stable/12/sys/netinet/sctp_os_bsd.h Mon Aug 24 09:11:37 2020 (r364640) +++ stable/12/sys/netinet/sctp_os_bsd.h Mon Aug 24 09:13:06 2020 (r364641) @@ -266,12 +266,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: stable/12/sys/netinet/sctp_pcb.c ============================================================================== --- stable/12/sys/netinet/sctp_pcb.c Mon Aug 24 09:11:37 2020 (r364640) +++ stable/12/sys/netinet/sctp_pcb.c Mon Aug 24 09:13:06 2020 (r364641) @@ -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: stable/12/sys/netinet/sctp_var.h ============================================================================== --- stable/12/sys/netinet/sctp_var.h Mon Aug 24 09:11:37 2020 (r364640) +++ stable/12/sys/netinet/sctp_var.h Mon Aug 24 09:13:06 2020 (r364641) @@ -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); \ if ((__net)->ro.ro_rt) { \ RTFREE((__net)->ro.ro_rt); \ (__net)->ro.ro_rt = NULL; \ Modified: stable/12/sys/netinet/sctputil.c ============================================================================== --- stable/12/sys/netinet/sctputil.c Mon Aug 24 09:11:37 2020 (r364640) +++ stable/12/sys/netinet/sctputil.c Mon Aug 24 09:13:06 2020 (r364641) @@ -1709,16 +1709,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); @@ -1734,56 +1740,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); @@ -1798,13 +1787,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); @@ -1831,6 +1820,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; @@ -1861,8 +1851,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, @@ -1875,6 +1864,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, @@ -1890,6 +1880,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, @@ -1907,6 +1898,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: @@ -1927,6 +1921,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, @@ -1948,7 +1943,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, @@ -1956,7 +1951,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, @@ -1972,6 +1967,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, @@ -1986,6 +1982,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, @@ -1995,6 +1992,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; @@ -2005,7 +2003,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, @@ -2017,6 +2015,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, @@ -2057,6 +2056,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, @@ -2064,20 +2064,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 @@ -2087,8 +2089,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); @@ -2097,10 +2099,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(); @@ -2533,6 +2541,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 @@ -2825,9 +2846,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: stable/12/sys/netinet/sctputil.h ============================================================================== --- stable/12/sys/netinet/sctputil.h Mon Aug 24 09:11:37 2020 (r364640) +++ stable/12/sys/netinet/sctputil.h Mon Aug 24 09:13:06 2020 (r364641) @@ -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 *);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202008240913.07O9D60D003191>