Bluetooth: Controller: Fix missing conn update ind PDU validation

cvinayak · henrikbrixandersen · commit 9d03b8f1d308 · 2024-07-21T00:34:05.000+02:00
Fix missing validation of Connection Update Ind PDU. Ignore invalid connection update parameters and force a silent local connection termination. Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no> (cherry picked from commit 4b6d3f1) Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
diff --git a/subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c b/subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c
@@ -196,6 +196,22 @@ static bool cu_check_conn_parameters(struct ll_conn *conn, struct proc_ctx *ctx)
 }
 #endif /* CONFIG_BT_CTLR_CONN_PARAM_REQ */
 
+static bool cu_check_conn_ind_parameters(struct ll_conn *conn, struct proc_ctx *ctx)
+{
+	const uint16_t interval_max = ctx->data.cu.interval_max; /* unit 1.25ms */
+	const uint16_t timeout = ctx->data.cu.timeout; /* unit 10ms */
+	const uint16_t latency = ctx->data.cu.latency;
+
+	/* Valid conn_update_ind parameters */
+	return (interval_max >= CONN_INTERVAL_MIN(conn)) &&
+	       (interval_max <= CONN_UPDATE_CONN_INTV_4SEC) &&
+	       (latency <= CONN_UPDATE_LATENCY_MAX) &&
+	       (timeout >= CONN_UPDATE_TIMEOUT_100MS) &&
+	       (timeout <= CONN_UPDATE_TIMEOUT_32SEC) &&
+	       ((timeout * 4U) > /* *4U re. conn events is equivalent to *2U re. ms */
+		((latency + 1U) * interval_max));
+}
+
 static void cu_prepare_update_ind(struct ll_conn *conn, struct proc_ctx *ctx)
 {
 	ctx->data.cu.win_size = 1U;
@@ -585,8 +601,20 @@ static void lp_cu_st_wait_rx_conn_update_ind(struct ll_conn *conn, struct proc_c
 	switch (evt) {
 	case LP_CU_EVT_CONN_UPDATE_IND:
 		llcp_pdu_decode_conn_update_ind(ctx, param);
+
+		/* Invalid PDU, mark the connection for termination */
+		if (!cu_check_conn_ind_parameters(conn, ctx)) {
+			llcp_rr_set_incompat(conn, INCOMPAT_NO_COLLISION);
+			conn->llcp_terminate.reason_final = BT_HCI_ERR_INVALID_LL_PARAM;
+			lp_cu_complete(conn, ctx);
+			break;
+		}
+
+		llcp_rr_set_incompat(conn, INCOMPAT_RESERVED);
+
 		/* Keep RX node to use for NTF */
 		llcp_rx_node_retain(ctx);
+
 		ctx->state = LP_CU_STATE_WAIT_INSTANT;
 		break;
 	case LP_CU_EVT_UNKNOWN:
@@ -1206,19 +1234,27 @@ static void rp_cu_st_wait_rx_conn_update_ind(struct ll_conn *conn, struct proc_c
 		case BT_HCI_ROLE_PERIPHERAL:
 			llcp_pdu_decode_conn_update_ind(ctx, param);
 
-			if (is_instant_not_passed(ctx->data.cu.instant,
-						  ull_conn_event_counter(conn))) {
+			/* Valid PDU */
+			if (cu_check_conn_ind_parameters(conn, ctx)) {
+				if (is_instant_not_passed(ctx->data.cu.instant,
+							  ull_conn_event_counter(conn))) {
+					/* Keep RX node to use for NTF */
+					llcp_rx_node_retain(ctx);
 
-				llcp_rx_node_retain(ctx);
+					ctx->state = RP_CU_STATE_WAIT_INSTANT;
+
+					/* In case we only just received it in time */
+					rp_cu_check_instant(conn, ctx, evt, param);
+					break;
+				}
 
-				ctx->state = RP_CU_STATE_WAIT_INSTANT;
-				/* In case we only just received it in time */
-				rp_cu_check_instant(conn, ctx, evt, param);
-			} else {
 				conn->llcp_terminate.reason_final = BT_HCI_ERR_INSTANT_PASSED;
-				llcp_rr_complete(conn);
-				ctx->state = RP_CU_STATE_IDLE;
+			} else {
+				conn->llcp_terminate.reason_final = BT_HCI_ERR_INVALID_LL_PARAM;
 			}
+
+			llcp_rr_complete(conn);
+			ctx->state = RP_CU_STATE_IDLE;
 			break;
 		default:
 			/* Unknown role */
diff --git a/tests/bluetooth/controller/ctrl_conn_update/src/main.c b/tests/bluetooth/controller/ctrl_conn_update/src/main.c
@@ -4859,6 +4859,128 @@ ZTEST(periph_loc_no_param_req, test_conn_update_periph_loc_disallowed_no_param_r
 }
 #endif
 
+/*
+ * Central-initiated Connection Update procedure.
+ * Peripheral receives invalid Connection Update parameters.
+ *
+ * +-----+                    +-------+                    +-----+
+ * | UT  |                    | LL_P  |                    | LT  |
+ * +-----+                    +-------+                    +-----+
+ *    |                           |                           |
+ *    |                           |  LL_CONNECTION_UPDATE_IND |
+ *    |                           |<--------------------------|
+ *    |                           |                           |
+ *    ~~~~~~~~~~~~~~~~~~ TERMINATE CONNECTION ~~~~~~~~~~~~~~~~~
+ *    |                           |                           |
+ */
+ZTEST(periph_rem_invalid, test_conn_update_periph_rem_invalid_param)
+{
+	uint16_t interval;
+
+	/* Role */
+	test_set_role(&conn, BT_HCI_ROLE_PERIPHERAL);
+
+	/* Connect */
+	ull_cp_state_set(&conn, ULL_CP_CONNECTED);
+
+	/* Prepare */
+	event_prepare(&conn);
+
+	/* Rx */
+	interval = conn_update_ind.interval;
+	conn_update_ind.interval = 0U;
+	conn_update_ind.instant = event_counter(&conn) + 6U;
+	lt_tx(LL_CONNECTION_UPDATE_IND, &conn, &conn_update_ind);
+
+	/* Done */
+	event_done(&conn);
+
+	/* Termination 'triggered' */
+	zassert_equal(conn.llcp_terminate.reason_final, BT_HCI_ERR_INVALID_LL_PARAM,
+		      "Terminate reason %d", conn.llcp_terminate.reason_final);
+
+	/* Clear termination flag for subsequent test cycle */
+	conn.llcp_terminate.reason_final = 0;
+
+	/* Restore interval for other tests */
+	conn_update_ind.interval = interval;
+}
+
+#if defined(CONFIG_BT_CTLR_CONN_PARAM_REQ)
+/*
+ * Peripheral-initiated Connection Parameters Request procedure.
+ * Peripheral requests change in LE connection parameters, central’s Host accepts.
+ * Peripheral receives invalid Connection Update parameters.
+ *
+ * +-----+                    +-------+                    +-----+
+ * | UT  |                    | LL_P  |                    | LT  |
+ * +-----+                    +-------+                    +-----+
+ *    |                           |                           |
+ *    | LE Connection Update      |                           |
+ *    |-------------------------->|                           |
+ *    |                           | LL_CONNECTION_PARAM_REQ   |
+ *    |                           |-------------------------->|
+ *    |                           |                           |
+ *    |                           |  LL_CONNECTION_UPDATE_IND |
+ *    |                           |<--------------------------|
+ *    |                           |                           |
+ *    ~~~~~~~~~~~~~~~~~~ TERMINATE CONNECTION ~~~~~~~~~~~~~~~~~
+ *    |                           |                           |
+ */
+ZTEST(periph_rem_invalid, test_conn_param_req_periph_rem_invalid_param)
+{
+	struct node_tx *tx;
+	uint16_t interval;
+	uint8_t err;
+
+	/* Role */
+	test_set_role(&conn, BT_HCI_ROLE_PERIPHERAL);
+
+	/* Connect */
+	ull_cp_state_set(&conn, ULL_CP_CONNECTED);
+
+	/* Initiate a Connection Parameter Request Procedure */
+	err = ull_cp_conn_update(&conn, INTVL_MIN, INTVL_MAX, LATENCY, TIMEOUT, NULL);
+	zassert_equal(err, BT_HCI_ERR_SUCCESS);
+
+	/* Prepare */
+	event_prepare(&conn);
+	conn_param_req.reference_conn_event_count = event_counter(&conn);
+
+	/* Tx Queue should have one LL Control PDU */
+	lt_rx(LL_CONNECTION_PARAM_REQ, &conn, &tx, &conn_param_req);
+	lt_rx_q_is_empty(&conn);
+
+	/* Done */
+	event_done(&conn);
+
+	/* Release Tx */
+	ull_cp_release_tx(&conn, tx);
+
+	/* Prepare */
+	event_prepare(&conn);
+
+	/* Rx */
+	interval = conn_update_ind.interval;
+	conn_update_ind.interval = 0U;
+	conn_update_ind.instant = event_counter(&conn) + 6U;
+	lt_tx(LL_CONNECTION_UPDATE_IND, &conn, &conn_update_ind);
+
+	/* Done */
+	event_done(&conn);
+
+	/* Termination 'triggered' */
+	zassert_equal(conn.llcp_terminate.reason_final, BT_HCI_ERR_INVALID_LL_PARAM,
+		      "Terminate reason %d", conn.llcp_terminate.reason_final);
+
+	/* Clear termination flag for subsequent test cycle */
+	conn.llcp_terminate.reason_final = 0;
+
+	/* Restore interval for other tests */
+	conn_update_ind.interval = interval;
+}
+#endif /* CONFIG_BT_CTLR_CONN_PARAM_REQ */
+
 #if defined(CONFIG_BT_CTLR_CONN_PARAM_REQ)
 ZTEST_SUITE(central_loc, NULL, NULL, conn_update_setup, NULL, NULL);
 ZTEST_SUITE(central_rem, NULL, NULL, conn_update_setup, NULL, NULL);
@@ -4870,3 +4992,5 @@ ZTEST_SUITE(central_rem_no_param_req, NULL, NULL, conn_update_setup, NULL, NULL)
 ZTEST_SUITE(periph_loc_no_param_req, NULL, NULL, conn_update_setup, NULL, NULL);
 ZTEST_SUITE(periph_rem_no_param_req, NULL, NULL, conn_update_setup, NULL, NULL);
 #endif /* CONFIG_BT_CTLR_CONN_PARAM_REQ */
+
+ZTEST_SUITE(periph_rem_invalid, NULL, NULL, conn_update_setup, NULL, NULL);
