From 4b6d3f1e16ea8ccccbf85f1eaf7efd0caea11766 Mon Sep 17 00:00:00 2001 From: Vinayak Kariappa Chettimada Date: Sat, 11 May 2024 06:35:19 +0200 Subject: [PATCH] Bluetooth: Controller: Fix missing conn update ind PDU validation 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 --- .../controller/ll_sw/ull_llcp_conn_upd.c | 53 ++++++-- .../controller/ctrl_conn_update/src/main.c | 124 ++++++++++++++++++ 2 files changed, 168 insertions(+), 9 deletions(-) diff --git a/subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c b/subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c index eb1f692e55f278..db0d2711748b30 100644 --- 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,9 +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: @@ -1202,19 +1229,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 index 937f71833346b2..635e36c4ce1ebe 100644 --- 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);