Skip site navigation (1)Skip section navigation (2) 
Date:      Thu, 7 May 2020 01:37:43 +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-11@freebsd.org
Subject:   svn commit: r360736 - stable/11/sys/netinet
Message-ID:  <202005070137.0471bhIO061498@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: tuexen
Date: Thu May  7 01:37:42 2020
New Revision: 360736
URL: https://svnweb.freebsd.org/changeset/base/360736

Log:
  MFC r352594: Improve SCTP locking
  
  Don't hold the info lock when calling sctp_select_a_tag().
  
  This avoids a double lock bug in the NAT colliding state processing
  of SCTP. Thanks to Felix Weinrank for finding and reporting this issue in
  https://github.com/sctplab/usrsctp/issues/374
  He found this bug using fuzz testing.

Modified:
  stable/11/sys/netinet/sctp_input.c
Directory Properties:
  stable/11/   (props changed)

Modified: stable/11/sys/netinet/sctp_input.c
==============================================================================
--- stable/11/sys/netinet/sctp_input.c	Thu May  7 01:34:41 2020	(r360735)
+++ stable/11/sys/netinet/sctp_input.c	Thu May  7 01:37:42 2020	(r360736)
@@ -702,34 +702,37 @@ static int
 sctp_handle_nat_colliding_state(struct sctp_tcb *stcb)
 {
 	/*
-	 * return 0 means we want you to proceed with the abort non-zero
-	 * means no abort processing
+	 * Return 0 means we want you to proceed with the abort non-zero
+	 * means no abort processing.
 	 */
+	uint32_t new_vtag;
 	struct sctpasochead *head;
 
 	if ((SCTP_GET_STATE(stcb) == SCTP_STATE_COOKIE_WAIT) ||
 	    (SCTP_GET_STATE(stcb) == SCTP_STATE_COOKIE_ECHOED)) {
+		new_vtag = sctp_select_a_tag(stcb->sctp_ep, stcb->sctp_ep->sctp_lport, stcb->rport, 1);
 		atomic_add_int(&stcb->asoc.refcnt, 1);
 		SCTP_TCB_UNLOCK(stcb);
 		SCTP_INP_INFO_WLOCK();
 		SCTP_TCB_LOCK(stcb);
 		atomic_subtract_int(&stcb->asoc.refcnt, 1);
+	} else {
+		return (0);
 	}
 	if (SCTP_GET_STATE(stcb) == SCTP_STATE_COOKIE_WAIT) {
 		/* generate a new vtag and send init */
 		LIST_REMOVE(stcb, sctp_asocs);
-		stcb->asoc.my_vtag = sctp_select_a_tag(stcb->sctp_ep, stcb->sctp_ep->sctp_lport, stcb->rport, 1);
+		stcb->asoc.my_vtag = new_vtag;
 		head = &SCTP_BASE_INFO(sctp_asochash)[SCTP_PCBHASH_ASOC(stcb->asoc.my_vtag, SCTP_BASE_INFO(hashasocmark))];
 		/*
 		 * put it in the bucket in the vtag hash of assoc's for the
 		 * system
 		 */
 		LIST_INSERT_HEAD(head, stcb, sctp_asocs);
-		sctp_send_initiate(stcb->sctp_ep, stcb, SCTP_SO_NOT_LOCKED);
 		SCTP_INP_INFO_WUNLOCK();
+		sctp_send_initiate(stcb->sctp_ep, stcb, SCTP_SO_NOT_LOCKED);
 		return (1);
-	}
-	if (SCTP_GET_STATE(stcb) == SCTP_STATE_COOKIE_ECHOED) {
+	} else {
 		/*
 		 * treat like a case where the cookie expired i.e.: - dump
 		 * current cookie. - generate a new vtag. - resend init.
@@ -739,15 +742,15 @@ sctp_handle_nat_colliding_state(struct sctp_tcb *stcb)
 		SCTP_SET_STATE(stcb, SCTP_STATE_COOKIE_WAIT);
 		sctp_stop_all_cookie_timers(stcb);
 		sctp_toss_old_cookies(stcb, &stcb->asoc);
-		stcb->asoc.my_vtag = sctp_select_a_tag(stcb->sctp_ep, stcb->sctp_ep->sctp_lport, stcb->rport, 1);
+		stcb->asoc.my_vtag = new_vtag;
 		head = &SCTP_BASE_INFO(sctp_asochash)[SCTP_PCBHASH_ASOC(stcb->asoc.my_vtag, SCTP_BASE_INFO(hashasocmark))];
 		/*
 		 * put it in the bucket in the vtag hash of assoc's for the
 		 * system
 		 */
 		LIST_INSERT_HEAD(head, stcb, sctp_asocs);
-		sctp_send_initiate(stcb->sctp_ep, stcb, SCTP_SO_NOT_LOCKED);
 		SCTP_INP_INFO_WUNLOCK();
+		sctp_send_initiate(stcb->sctp_ep, stcb, SCTP_SO_NOT_LOCKED);
 		return (1);
 	}
 	return (0);

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202005070137.0471bhIO061498>