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 <marco.varlese@suse.com>

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)