From 04e5d64c454ec53103fa1f4b7f3634bb61a65d0f Mon Sep 17 00:00:00 2001 From: Marco Varlese Date: Fri, 23 Feb 2018 17:43:06 +0100 Subject: [PATCH 1/1] SCTP: fix connection memory corruption A bug was found when multiple SCTP connections were being opened to the same SCTP server. This patch addresses that problem, removing the use of the 'parent' pointer approach for sub-connection and saving instead within the sub-connection itself the ID representing its position. That facilitates pointer-arithmetic to be computed in the get_connection_from_transport(). Change-Id: Iaa1f4efc501590be1c93e42fd6fe3d6e02f635eb Signed-off-by: Marco Varlese --- src/vnet/sctp/sctp.c | 14 +++++++++----- src/vnet/sctp/sctp.h | 10 +++++++--- src/vnet/sctp/sctp_input.c | 9 +++++---- src/vnet/sctp/sctp_output.c | 4 ++-- test/test_sctp.py | 5 +++-- 5 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/vnet/sctp/sctp.c b/src/vnet/sctp/sctp.c index 4643e8e900a..9a0f47b599f 100644 --- a/src/vnet/sctp/sctp.c +++ b/src/vnet/sctp/sctp.c @@ -27,7 +27,8 @@ sctp_connection_bind (u32 session_index, transport_endpoint_t * tep) pool_get (tm->listener_pool, listener); memset (listener, 0, sizeof (*listener)); - listener->sub_conn[MAIN_SCTP_SUB_CONN_IDX].parent = listener; + listener->sub_conn[MAIN_SCTP_SUB_CONN_IDX].subconn_idx = + MAIN_SCTP_SUB_CONN_IDX; listener->sub_conn[MAIN_SCTP_SUB_CONN_IDX].c_c_index = listener - tm->listener_pool; listener->sub_conn[MAIN_SCTP_SUB_CONN_IDX].connection.lcl_port = tep->port; @@ -273,7 +274,8 @@ sctp_sub_connection_add (u8 thread_index) sctp_conn->sub_conn[MAIN_SCTP_SUB_CONN_IDX].connection.c_index; sctp_conn->sub_conn[sctp_conn->next_avail_sub_conn]. connection.thread_index = thread_index; - sctp_conn->sub_conn[sctp_conn->next_avail_sub_conn].parent = sctp_conn; + sctp_conn->sub_conn[sctp_conn->next_avail_sub_conn].subconn_idx = + sctp_conn->next_avail_sub_conn; sctp_conn->next_avail_sub_conn += 1; @@ -310,7 +312,8 @@ sctp_connection_new (u8 thread_index) pool_get (sctp_main->connections[thread_index], sctp_conn); memset (sctp_conn, 0, sizeof (*sctp_conn)); - sctp_conn->sub_conn[MAIN_SCTP_SUB_CONN_IDX].parent = sctp_conn; + sctp_conn->sub_conn[MAIN_SCTP_SUB_CONN_IDX].subconn_idx = + MAIN_SCTP_SUB_CONN_IDX; sctp_conn->sub_conn[MAIN_SCTP_SUB_CONN_IDX].c_c_index = sctp_conn - sctp_main->connections[thread_index]; sctp_conn->sub_conn[MAIN_SCTP_SUB_CONN_IDX].c_thread_index = thread_index; @@ -330,7 +333,8 @@ sctp_half_open_connection_new (u8 thread_index) memset (sctp_conn, 0, sizeof (*sctp_conn)); sctp_conn->sub_conn[MAIN_SCTP_SUB_CONN_IDX].c_c_index = sctp_conn - tm->half_open_connections; - sctp_conn->sub_conn[MAIN_SCTP_SUB_CONN_IDX].parent = sctp_conn; + sctp_conn->sub_conn[MAIN_SCTP_SUB_CONN_IDX].subconn_idx = + MAIN_SCTP_SUB_CONN_IDX; return sctp_conn; } @@ -374,7 +378,7 @@ sctp_connection_open (transport_endpoint_t * rmt) transport_connection_t *trans_conn = &sctp_conn->sub_conn[idx].connection; ip_copy (&trans_conn->rmt_ip, &rmt->ip, rmt->is_ip4); ip_copy (&trans_conn->lcl_ip, &lcl_addr, rmt->is_ip4); - sctp_conn->sub_conn[idx].parent = sctp_conn; + sctp_conn->sub_conn[idx].subconn_idx = idx; trans_conn->rmt_port = rmt->port; trans_conn->lcl_port = clib_host_to_net_u16 (lcl_port); trans_conn->is_ip4 = rmt->is_ip4; diff --git a/src/vnet/sctp/sctp.h b/src/vnet/sctp/sctp.h index fd9d8da5741..048d153ac55 100644 --- a/src/vnet/sctp/sctp.h +++ b/src/vnet/sctp/sctp.h @@ -100,8 +100,8 @@ enum _sctp_subconn_state typedef struct _sctp_sub_connection { transport_connection_t connection; /**< Common transport data. First! */ - void *parent; /**< Link to the parent-super connection */ + u8 subconn_idx; /**< This indicates the position of this sub-connection in the super-set container of connections pool */ u32 error_count; /**< The current error count for this destination. */ u32 error_threshold; /**< Current error threshold for this destination, i.e. what value marks the destination down if error count reaches this value. */ @@ -512,7 +512,7 @@ sctp_half_open_connection_get (u32 conn_index) clib_spinlock_lock_if_init (&sctp_main.half_open_lock); if (!pool_is_free_index (sctp_main.half_open_connections, conn_index)) tc = pool_elt_at_index (sctp_main.half_open_connections, conn_index); - tc->sub_conn[MAIN_SCTP_SUB_CONN_IDX].parent = tc; + tc->sub_conn[MAIN_SCTP_SUB_CONN_IDX].subconn_idx = MAIN_SCTP_SUB_CONN_IDX; clib_spinlock_unlock_if_init (&sctp_main.half_open_lock); return tc; } @@ -609,7 +609,11 @@ sctp_get_connection_from_transport (transport_connection_t * tconn) if (sub->parent == NULL) SCTP_ADV_DBG ("sub->parent == NULL"); #endif - return (sctp_connection_t *) sub->parent; + if (sub->subconn_idx > 0) + return (sctp_connection_t *) sub - + (sizeof (sctp_sub_connection_t) * (sub->subconn_idx - 1)); + + return (sctp_connection_t *) sub; } always_inline u32 diff --git a/src/vnet/sctp/sctp_input.c b/src/vnet/sctp/sctp_input.c index d3e69c68ba0..615cdefd134 100644 --- a/src/vnet/sctp/sctp_input.c +++ b/src/vnet/sctp/sctp_input.c @@ -909,7 +909,7 @@ sctp46_rcv_phase_inline (vlib_main_t * vm, vlib_node_runtime_t * node, idx = sctp_sub_conn_id_via_ip6h (sctp_conn, ip6_hdr); } - sctp_conn->sub_conn[idx].parent = sctp_conn; + sctp_conn->sub_conn[idx].subconn_idx = idx; sctp_full_hdr_t *full_hdr = (sctp_full_hdr_t *) sctp_hdr; sctp_chunk_hdr = @@ -938,7 +938,7 @@ sctp46_rcv_phase_inline (vlib_main_t * vm, vlib_node_runtime_t * node, my_thread_index; new_sctp_conn->sub_conn[idx].PMTU = sctp_conn->sub_conn[idx].PMTU; - new_sctp_conn->sub_conn[idx].parent = new_sctp_conn; + new_sctp_conn->sub_conn[idx].subconn_idx = idx; if (sctp_half_open_connection_cleanup (sctp_conn)) { @@ -1563,7 +1563,8 @@ sctp46_listen_process_inline (vlib_main_t * vm, /* Create child session and send SYN-ACK */ child_conn = sctp_connection_new (my_thread_index); - child_conn->sub_conn[MAIN_SCTP_SUB_CONN_IDX].parent = child_conn; + child_conn->sub_conn[MAIN_SCTP_SUB_CONN_IDX].subconn_idx = + MAIN_SCTP_SUB_CONN_IDX; child_conn->sub_conn[MAIN_SCTP_SUB_CONN_IDX].c_lcl_port = sctp_hdr->dst_port; child_conn->sub_conn[MAIN_SCTP_SUB_CONN_IDX].c_rmt_port = @@ -1748,7 +1749,7 @@ sctp46_established_phase_inline (vlib_main_t * vm, vlib_node_runtime_t * node, idx = sctp_sub_conn_id_via_ip6h (sctp_conn, ip6_hdr); } - sctp_conn->sub_conn[idx].parent = sctp_conn; + sctp_conn->sub_conn[idx].subconn_idx = idx; sctp_full_hdr_t *full_hdr = (sctp_full_hdr_t *) sctp_hdr; sctp_chunk_hdr = diff --git a/src/vnet/sctp/sctp_output.c b/src/vnet/sctp/sctp_output.c index 39e5e75ea57..a9b2417e0c9 100644 --- a/src/vnet/sctp/sctp_output.c +++ b/src/vnet/sctp/sctp_output.c @@ -1094,7 +1094,7 @@ sctp_push_hdr_i (sctp_connection_t * sctp_conn, vlib_buffer_t * b, u8 idx = sctp_data_subconn_select (sctp_conn); SCTP_DBG_OUTPUT - ("SCTP_CONN = %p, IDX = %u, S_INDEX = %u, C_INDEX = %u, LCL_PORT = %u, RMT_PORT = %u", + ("SCTP_CONN = %p, IDX = %u, S_INDEX = %u, C_INDEX = %u, sctp_conn->[...].LCL_PORT = %u, sctp_conn->[...].RMT_PORT = %u", sctp_conn, idx, sctp_conn->sub_conn[idx].connection.s_index, sctp_conn->sub_conn[idx].connection.c_index, sctp_conn->sub_conn[idx].connection.lcl_port, @@ -1149,7 +1149,7 @@ sctp_push_header (transport_connection_t * trans_conn, vlib_buffer_t * b) SCTP_DBG_OUTPUT ("TRANS_CONN = %p, SCTP_CONN = %p, " "S_INDEX = %u, C_INDEX = %u," - "LCL_PORT = %u, RMT_PORT = %u", + "trans_conn->LCL_PORT = %u, trans_conn->RMT_PORT = %u", trans_conn, sctp_conn, trans_conn->s_index, diff --git a/test/test_sctp.py b/test/test_sctp.py index a2f5e6a45da..32068abf76e 100644 --- a/test/test_sctp.py +++ b/test/test_sctp.py @@ -68,9 +68,10 @@ class TestSCTP(VppTestCase): self.logger.critical(error) self.assertEqual(error.find("failed"), -1) - error = self.vapi.cli("test echo client mbytes 10 appns 1" + + error = self.vapi.cli("test echo client nclients 2 mbytes 100" + + " appns 1" + " fifo-size 4" + - " no-output test-bytes syn-timeout 20 " + + " no-output test-bytes syn-timeout 3" + " uri " + uri) if error: self.logger.critical(error) -- 2.16.6