Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bluetooth: Controller: Fix missing conn update ind PDU validation #72608

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cvinayak
Copy link
Contributor

Fix missing validation of Connection Update Ind PDU. Ignore invalid connection update parameters and let the connection either procedure timeout or supervision timeout if the central switched to using invalid connection parameters.

@cvinayak
Copy link
Contributor Author

cvinayak commented May 11, 2024

@thoh-ot @erbr-ot @wopu-ot @kruithofa is this behavior to let procedure/supervision timeout an acceptable outcome?
Will have to add/update unit tests accordingly.

Fix missing validation of Connection Update Ind PDU. Ignore
invalid connection update parameters and let the connection
either procedure timeout or supervision timeout if the
central switched to using invalid connection parameters.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@cvinayak cvinayak force-pushed the github_conn_update_ind_param_check branch from 4992255 to f7f65af Compare May 12, 2024 03:36
@erbr-ot
Copy link
Collaborator

erbr-ot commented May 13, 2024

I tend to think we should be more pro-active in case of invalid params and disconnect with BT_HCI_ERR_INVALID_LL_PARAM. This could make recovery from a central mistake faster.

@thoh-ot
Copy link
Collaborator

thoh-ot commented May 14, 2024

Take at look at how invalid LL_PHY_UPDATE_IND are handled in

static void lp_pu_st_wait_rx_phy_update_ind(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t evt,
void *param)
{
switch (evt) {
case LP_PU_EVT_PHY_UPDATE_IND:
LL_ASSERT(conn->lll.role == BT_HCI_ROLE_PERIPHERAL);
llcp_rr_set_incompat(conn, INCOMPAT_RESERVED);
llcp_pdu_decode_phy_update_ind(ctx, (struct pdu_data *)param);
const uint8_t end_procedure = pu_check_update_ind(conn, ctx);
/* Mark RX node to NOT release */
llcp_rx_node_retain(ctx);
if (!end_procedure) {
if (ctx->data.pu.p_to_c_phy) {
/* If periph to central phy changes apply tx timing restriction */
pu_set_timing_restrict(conn, ctx->data.pu.p_to_c_phy);
}
/* Since at least one phy will change,
* stop the procedure response timeout
*/
llcp_lr_prt_stop(conn);
ctx->state = LP_PU_STATE_WAIT_INSTANT;
} else {
llcp_rr_set_incompat(conn, INCOMPAT_NO_COLLISION);
if (ctx->data.pu.error != BT_HCI_ERR_SUCCESS) {
/* Mark the connection for termination */
conn->llcp_terminate.reason_final = ctx->data.pu.error;
}
ctx->data.pu.ntf_pu = ctx->data.pu.host_initiated;
lp_pu_complete(conn, ctx, evt, param);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants