From owner-svn-src-head@freebsd.org Sun Oct 15 19:33:32 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 2E19BE49719; Sun, 15 Oct 2017 19:33:32 +0000 (UTC) (envelope-from tuexen@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id D408484164; Sun, 15 Oct 2017 19:33:31 +0000 (UTC) (envelope-from tuexen@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id v9FJXVs9080843; Sun, 15 Oct 2017 19:33:31 GMT (envelope-from tuexen@FreeBSD.org) Received: (from tuexen@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id v9FJXVcA080842; Sun, 15 Oct 2017 19:33:31 GMT (envelope-from tuexen@FreeBSD.org) Message-Id: <201710151933.v9FJXVcA080842@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: tuexen set sender to tuexen@FreeBSD.org using -f From: Michael Tuexen Date: Sun, 15 Oct 2017 19:33:30 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r324638 - head/sys/netinet X-SVN-Group: head X-SVN-Commit-Author: tuexen X-SVN-Commit-Paths: head/sys/netinet X-SVN-Commit-Revision: 324638 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 15 Oct 2017 19:33:32 -0000 Author: tuexen Date: Sun Oct 15 19:33:30 2017 New Revision: 324638 URL: https://svnweb.freebsd.org/changeset/base/324638 Log: Fix the handling of parital and too short chunks. Ensure that the current behaviour is consistent: stop processing of the chunk, but finish the processing of the previous chunks. This behaviour might be changed in a later commit to ABORT the assoication due to a protocol violation, but changing this is a separate issue. MFC after: 3 days Modified: head/sys/netinet/sctp_input.c Modified: head/sys/netinet/sctp_input.c ============================================================================== --- head/sys/netinet/sctp_input.c Sun Oct 15 19:28:14 2017 (r324637) +++ head/sys/netinet/sctp_input.c Sun Oct 15 19:33:30 2017 (r324638) @@ -4518,7 +4518,6 @@ sctp_process_control(struct mbuf *m, int iphlen, int * * until we get into jumbo grams and such.. */ uint8_t chunk_buf[SCTP_CHUNK_BUFFER_SIZE]; - struct sctp_tcb *locked_tcb = stcb; int got_auth = 0; uint32_t auth_offset = 0, auth_len = 0; int auth_skipped = 0; @@ -4530,31 +4529,28 @@ sctp_process_control(struct mbuf *m, int iphlen, int * SCTPDBG(SCTP_DEBUG_INPUT1, "sctp_process_control: iphlen=%u, offset=%u, length=%u stcb:%p\n", iphlen, *offset, length, (void *)stcb); + if (stcb) { + SCTP_TCB_LOCK_ASSERT(stcb); + } /* validate chunk header length... */ if (ntohs(ch->chunk_length) < sizeof(*ch)) { SCTPDBG(SCTP_DEBUG_INPUT1, "Invalid header length %d\n", ntohs(ch->chunk_length)); - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } - return (NULL); + return (stcb); } /* * validate the verification tag */ vtag_in = ntohl(sh->v_tag); - if (locked_tcb) { - SCTP_TCB_LOCK_ASSERT(locked_tcb); - } if (ch->chunk_type == SCTP_INITIATION) { SCTPDBG(SCTP_DEBUG_INPUT1, "Its an INIT of len:%d vtag:%x\n", ntohs(ch->chunk_length), vtag_in); if (vtag_in != 0) { /* protocol error- silently discard... */ SCTP_STAT_INCR(sctps_badvtag); - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); + if (stcb != NULL) { + SCTP_TCB_UNLOCK(stcb); } return (NULL); } @@ -4577,9 +4573,6 @@ sctp_process_control(struct mbuf *m, int iphlen, int * if (*offset >= length) { /* no more data left in the mbuf chain */ *offset = length; - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } return (NULL); } ch = (struct sctp_chunkhdr *)sctp_m_getptr(m, *offset, @@ -4588,10 +4581,7 @@ sctp_process_control(struct mbuf *m, int iphlen, int * if (ch == NULL) { /* Help */ *offset = length; - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } - return (NULL); + return (stcb); } if (ch->chunk_type == SCTP_COOKIE_ECHO) { goto process_control_chunks; @@ -4628,10 +4618,7 @@ sctp_process_control(struct mbuf *m, int iphlen, int * * sctp_findassociation_ep_asconf(). */ SCTP_INP_DECR_REF(inp); - } else { - locked_tcb = stcb; } - /* now go back and verify any auth chunk to be sure */ if (auth_skipped && (stcb != NULL)) { struct sctp_auth_chunk *auth; @@ -4645,10 +4632,7 @@ sctp_process_control(struct mbuf *m, int iphlen, int * auth_offset)) { /* auth HMAC failed so dump it */ *offset = length; - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } - return (NULL); + return (stcb); } else { /* remaining chunks are HMAC checked */ stcb->asoc.authenticated = 1; @@ -4664,9 +4648,6 @@ sctp_process_control(struct mbuf *m, int iphlen, int * mflowtype, mflowid, inp->fibnum, vrf_id, port); *offset = length; - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } return (NULL); } asoc = &stcb->asoc; @@ -4684,8 +4665,8 @@ sctp_process_control(struct mbuf *m, int iphlen, int * } else { /* drop this packet... */ SCTP_STAT_INCR(sctps_badvtag); - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); + if (stcb != NULL) { + SCTP_TCB_UNLOCK(stcb); } return (NULL); } @@ -4698,8 +4679,8 @@ sctp_process_control(struct mbuf *m, int iphlen, int * * but it won't complete until the shutdown * is completed */ - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); + if (stcb != NULL) { + SCTP_TCB_UNLOCK(stcb); } snprintf(msg, sizeof(msg), "OOTB, %s:%d at %s", __FILE__, __LINE__, __func__); op_err = sctp_generate_cause(SCTP_BASE_SYSCTL(sctp_diag_info_code), @@ -4718,8 +4699,8 @@ sctp_process_control(struct mbuf *m, int iphlen, int * "invalid vtag: %xh, expect %xh\n", vtag_in, asoc->my_vtag); SCTP_STAT_INCR(sctps_badvtag); - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); + if (stcb != NULL) { + SCTP_TCB_UNLOCK(stcb); } *offset = length; return (NULL); @@ -4755,10 +4736,7 @@ process_control_chunks: if (chk_length < sizeof(*ch) || (*offset + (int)chk_length) > length) { *offset = length; - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } - return (NULL); + return (stcb); } SCTP_STAT_INCR_COUNTER64(sctps_incontrolchunks); /* @@ -4773,8 +4751,8 @@ process_control_chunks: sizeof(struct sctp_init_ack_chunk), chunk_buf); if (ch == NULL) { *offset = length; - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); + if (stcb != NULL) { + SCTP_TCB_UNLOCK(stcb); } return (NULL); } @@ -4796,8 +4774,8 @@ process_control_chunks: chunk_buf); if (ch == NULL) { *offset = length; - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); + if (stcb != NULL) { + SCTP_TCB_UNLOCK(stcb); } return (NULL); } @@ -4808,8 +4786,8 @@ process_control_chunks: if (ch == NULL) { SCTP_PRINTF("sctp_process_control: Can't get the all data....\n"); *offset = length; - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); + if (stcb != NULL) { + SCTP_TCB_UNLOCK(stcb); } return (NULL); } @@ -4849,8 +4827,8 @@ process_control_chunks: (length - *offset > (int)SCTP_SIZE32(chk_length))) { /* RFC 4960 requires that no ABORT is sent */ *offset = length; - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); + if (stcb != NULL) { + SCTP_TCB_UNLOCK(stcb); } return (NULL); } @@ -4870,8 +4848,8 @@ process_control_chunks: mflowtype, mflowid, vrf_id, port); *offset = length; - if ((!abort_no_unlock) && (locked_tcb)) { - SCTP_TCB_UNLOCK(locked_tcb); + if ((!abort_no_unlock) && (stcb != NULL)) { + SCTP_TCB_UNLOCK(stcb); } return (NULL); break; @@ -4881,15 +4859,14 @@ process_control_chunks: SCTPDBG(SCTP_DEBUG_INPUT3, "SCTP_INIT-ACK\n"); if (inp->sctp_flags & SCTP_PCB_FLAGS_SOCKET_GONE) { /* We are not interested anymore */ - if ((stcb) && (stcb->asoc.total_output_queue_size)) { + if ((stcb != NULL) && (stcb->asoc.total_output_queue_size)) { ; } else { - if ((locked_tcb != NULL) && (locked_tcb != stcb)) { - /* Very unlikely */ - SCTP_TCB_UNLOCK(locked_tcb); + if (stcb != NULL) { + SCTP_TCB_UNLOCK(stcb); } *offset = length; - if (stcb) { + if (stcb != NULL) { #if defined(__APPLE__) || defined(SCTP_SO_LOCK_TESTING) so = SCTP_INP_SO(inp); atomic_add_int(&stcb->asoc.refcnt, 1); @@ -4911,10 +4888,7 @@ process_control_chunks: if ((num_chunks > 1) || (length - *offset > (int)SCTP_SIZE32(chk_length))) { *offset = length; - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } - return (NULL); + return (stcb); } if ((netp) && (*netp)) { ret = sctp_handle_init_ack(m, iphlen, *offset, @@ -4938,10 +4912,7 @@ process_control_chunks: if ((stcb != NULL) && (ret == 0)) { sctp_chunk_output(stcb->sctp_ep, stcb, SCTP_OUTPUT_FROM_CONTROL_PROC, SCTP_SO_NOT_LOCKED); } - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } - return (NULL); + return (stcb); break; case SCTP_SELECTIVE_ACK: { @@ -5133,10 +5104,7 @@ process_control_chunks: if ((stcb == NULL) || (chk_length != sizeof(struct sctp_heartbeat_chunk))) { /* Its not ours */ *offset = length; - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } - return (NULL); + return (stcb); } /* He's alive so give him credit */ if (SCTP_BASE_SYSCTL(sctp_logging_level) & SCTP_THRESHOLD_LOGGING) { @@ -5166,10 +5134,7 @@ process_control_chunks: (void *)stcb); if ((stcb == NULL) || (chk_length != sizeof(struct sctp_shutdown_chunk))) { *offset = length; - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } - return (NULL); + return (stcb); } if (netp && *netp) { int abort_flag = 0; @@ -5206,7 +5171,7 @@ process_control_chunks: if (inp->sctp_flags & SCTP_PCB_FLAGS_SOCKET_GONE) { /* We are not interested anymore */ abend: - if (stcb) { + if (stcb != NULL) { SCTP_TCB_UNLOCK(stcb); } *offset = length; @@ -5263,6 +5228,9 @@ process_control_chunks: } } if (netp) { + struct sctp_tcb *locked_stcb; + + locked_stcb = stcb; ret_buf = sctp_handle_cookie_echo(m, iphlen, *offset, @@ -5273,11 +5241,17 @@ process_control_chunks: auth_skipped, auth_offset, auth_len, - &locked_tcb, + &locked_stcb, mflowtype, mflowid, vrf_id, port); + if ((locked_stcb != NULL) && (locked_stcb != stcb)) { + SCTP_TCB_UNLOCK(locked_stcb); + } + if (stcb != NULL) { + SCTP_TCB_LOCK_ASSERT(stcb); + } } else { ret_buf = NULL; } @@ -5285,8 +5259,8 @@ process_control_chunks: SCTP_ASOC_CREATE_UNLOCK(linp); } if (ret_buf == NULL) { - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); + if (stcb != NULL) { + SCTP_TCB_UNLOCK(stcb); } SCTPDBG(SCTP_DEBUG_INPUT3, "GAK, null buffer\n"); @@ -5313,10 +5287,7 @@ process_control_chunks: case SCTP_COOKIE_ACK: SCTPDBG(SCTP_DEBUG_INPUT3, "SCTP_COOKIE-ACK, stcb %p\n", (void *)stcb); if ((stcb == NULL) || chk_length != sizeof(struct sctp_cookie_ack_chunk)) { - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } - return (NULL); + return (stcb); } if (inp->sctp_flags & SCTP_PCB_FLAGS_SOCKET_GONE) { /* We are not interested anymore */ @@ -5358,11 +5329,8 @@ process_control_chunks: /* He's alive so give him credit */ if ((stcb == NULL) || (chk_length != sizeof(struct sctp_ecne_chunk))) { /* Its not ours */ - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } *offset = length; - return (NULL); + return (stcb); } if (stcb) { if (stcb->asoc.ecn_supported == 0) { @@ -5385,12 +5353,8 @@ process_control_chunks: SCTPDBG(SCTP_DEBUG_INPUT3, "SCTP_ECN-CWR\n"); /* He's alive so give him credit */ if ((stcb == NULL) || (chk_length != sizeof(struct sctp_cwr_chunk))) { - /* Its not ours */ - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } *offset = length; - return (NULL); + return (stcb); } if (stcb) { if (stcb->asoc.ecn_supported == 0) { @@ -5413,10 +5377,7 @@ process_control_chunks: if ((num_chunks > 1) || (length - *offset > (int)SCTP_SIZE32(chk_length))) { *offset = length; - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } - return (NULL); + return (stcb); } if ((stcb) && netp && *netp) { sctp_handle_shutdown_complete((struct sctp_shutdown_complete_chunk *)ch, @@ -5449,11 +5410,8 @@ process_control_chunks: SCTPDBG(SCTP_DEBUG_INPUT3, "SCTP_ASCONF-ACK\n"); if (chk_length < sizeof(struct sctp_asconf_ack_chunk)) { /* Its not ours */ - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } *offset = length; - return (NULL); + return (stcb); } if ((stcb) && netp && *netp) { if (stcb->asoc.asconf_supported == 0) { @@ -5479,11 +5437,8 @@ process_control_chunks: SCTPDBG(SCTP_DEBUG_INPUT3, "SCTP_FWD-TSN\n"); if (chk_length < sizeof(struct sctp_forward_tsn_chunk)) { /* Its not ours */ - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } *offset = length; - return (NULL); + return (stcb); } /* He's alive so give him credit */ if (stcb) { @@ -5546,11 +5501,8 @@ process_control_chunks: SCTPDBG(SCTP_DEBUG_INPUT3, "SCTP_STREAM_RESET\n"); if (((stcb == NULL) || (ch == NULL) || (chk_length < sizeof(struct sctp_stream_reset_tsn_req)))) { /* Its not ours */ - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } *offset = length; - return (NULL); + return (stcb); } if (stcb->asoc.reconfig_supported == 0) { goto unknown_chunk; @@ -5566,11 +5518,8 @@ process_control_chunks: /* re-get it all please */ if (chk_length < sizeof(struct sctp_pktdrop_chunk)) { /* Its not ours */ - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } *offset = length; - return (NULL); + return (stcb); } if (ch && (stcb) && netp && (*netp)) { if (stcb->asoc.pktdrop_supported == 0) { @@ -5579,7 +5528,6 @@ process_control_chunks: sctp_handle_packet_dropped((struct sctp_pktdrop_chunk *)ch, stcb, *netp, min(chk_length, (sizeof(chunk_buf) - 4))); - } break; case SCTP_AUTHENTICATION: @@ -5601,11 +5549,8 @@ process_control_chunks: (chk_length > (sizeof(struct sctp_auth_chunk) + SCTP_AUTH_DIGEST_LEN_MAX))) { /* Its not ours */ - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } *offset = length; - return (NULL); + return (stcb); } if (got_auth == 1) { /* skip this chunk... it's already auth'd */ @@ -5670,11 +5615,8 @@ next_chunk: ch = (struct sctp_chunkhdr *)sctp_m_getptr(m, *offset, sizeof(struct sctp_chunkhdr), chunk_buf); if (ch == NULL) { - if (locked_tcb) { - SCTP_TCB_UNLOCK(locked_tcb); - } *offset = length; - return (NULL); + return (stcb); } } /* while */