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.

Sign up for GitHub

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 div-by-zero with BT_CONN_PARAM_ANY #84154

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
+21 −2
Draft

Bluetooth: Controller: Fix div-by-zero with BT_CONN_PARAM_ANY #84154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
65b4f76
Bluetooth: Controller: Fix div-by-zero with BT_CONN_PARAM_ANY
cvinayak Jan 17, 2025
124104e
Bluetooth: Controller: Disallow invalid connection interval value
cvinayak Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions subsys/bluetooth/controller/ll_sw/ull_central.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,13 @@ uint8_t ll_create_connection(uint16_t scan_interval, uint16_t scan_window,
uint8_t hop;
int err;

/* Disallow invalid connection interval */
if (!IS_ENABLED(CONFIG_BT_CTLR_CONN_INTERVAL_LOW_LATENCY) &&
!IN_RANGE(interval, BT_HCI_LE_INTERVAL_MIN, BT_HCI_LE_INTERVAL_MAX)) {
return BT_HCI_ERR_INVALID_PARAM;
}

/* Concurrent scanning and initiating not supported */
scan = ull_scan_is_disabled_get(SCAN_HANDLE_1M);
if (!scan) {
return BT_HCI_ERR_CMD_DISALLOWED;
Expand Down
9 changes: 8 additions & 1 deletion subsys/bluetooth/controller/ll_sw/ull_llcp_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,14 @@ static void lp_comm_tx(struct ll_conn *conn, struct proc_ctx *ctx)
* NOTE: As the supervision timeout is at most 32s the normal procedure response
* timeout of 40s will never come into play for the ACL Termination procedure.
*/
const uint32_t conn_interval_us = conn->lll.interval * CONN_INT_UNIT_US;
uint32_t conn_interval_us;

if (conn->lll.interval >= BT_HCI_LE_INTERVAL_MIN) {
conn_interval_us = conn->lll.interval * CONN_INT_UNIT_US;
} else {
conn_interval_us = (conn->lll.interval + 1U) * CONN_LOW_LAT_INT_UNIT_US;
}
Comment on lines +238 to +244
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can conn->lll.interval end up being < BT_HCI_LE_INTERVAL_MIN?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CONFIG_BT_CONN_PARAM_ANY=y allows connection interval values less than BT_HCI_LE_INTERVAL_MIN and greater than BT_HCI_LE_INTERVAL_MAX allowing some vendor implementation to use proprietary connection interval timings represented outside the defined min/max. In the Zephyr Controller, values 0 to 5 are used to stress the scheduling design/implementation for overlapping connection role under babblesim CI testing; a value of 1 respresents 1 ms interval (where in 0 is 500 us, and 5 is 3 ms).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

So conn->lll.interval == 0 is a valid value?

I count 15 places wher conn->lll.interval is multiplied by something else, making all those expression end up as 0. While I believe this PR is fixing one such specific case for a good reason, I have my doubts that this is a proper solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other places are related to Connected ISO, and these non-standard values use (CONFIG_BT_CONN_PARAM_ANY=y) would likely misbehave. Hence, not worth the effort to have added branching in Connected ISO use cases.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other places are related to Connected ISO

There are several places in non-ISO that also multiply by conn->lll.interval.

So conn->lll.interval == 0 is a valid value?

Missed this question I think :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several places in non-ISO that also multiply by conn->lll.interval.

Those are in connection establishment implementation, initiator/central and peripheral connection establishment. Support for initial connection establishment with these non-standard value in not supported/planned or covered in bsim test either. This will have to be a separate task outside the scope of this PR.

So conn->lll.interval == 0 is a valid value?

Missed this question I think :)

yes, Host permits a value of 0 as connection interval when CONFIG_BT_CONN_PARAM_ANY=y, but is not supported in Zephyr Controller for connection establishment, only supporting it in LL Connection Update and LL Connection Parameter Request Procedures, refer to c3f557c

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will have to be a separate task outside the scope of this PR.

Will you create a GH issue or something for it then? If we allow conn->lll.interval == 0 then that seems to be a fairly high risk of being misused as it is not allowed to be 0 by the core spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems its easier to fix the occurrences to help aid future feature development; putting PR back to draft until I get around to fixing it and updating a bsim coverage.

Comment on lines +242 to +244
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should a comment stating why/how/when if (conn->lll.interval >= BT_HCI_LE_INTERVAL_MIN) is true, and why, when it is true, the calculation changes from the intuitive conn_interval_us = conn->lll.interval * CONN_INT_UNIT_US; to something that is seemingly completely different

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These change are base dependencies for now adopted Frame Space Update procedure in PR: #82324

I am hoping the non-standard CONFIG_BT_CONN_PARAM_ANY can be deprecated in the future. I can discuss offline on the details with you.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, this code is non-standard (not spec-compliant) and pretty undocumented. We should add comment stating why this is here, and why it's interval + 1U and why CONN_LOW_LAT_INT_UNIT_US is used instead of CONN_INT_UNIT_US


const uint16_t sto_reload = RADIO_CONN_EVENTS(
(conn->supervision_timeout * 10U * 1000U),
conn_interval_us);
Expand Down
7 changes: 6 additions & 1 deletion subsys/bluetooth/controller/ll_sw/ull_peripheral.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,15 @@ void ull_periph_setup(struct node_rx_pdu *rx, struct node_rx_ftr *ftr,
sizeof(lll->data_chan_map));
lll->data_chan_hop = pdu_adv->connect_ind.hop;
lll->interval = sys_le16_to_cpu(pdu_adv->connect_ind.interval);

/* Check for minimum required channels, hop count and disallow invalid
* connection interval value.
*/
if ((lll->data_chan_count < CHM_USED_COUNT_MIN) ||
(lll->data_chan_hop < CHM_HOP_COUNT_MIN) ||
(lll->data_chan_hop > CHM_HOP_COUNT_MAX) ||
!lll->interval) {
(!IS_ENABLED(CONFIG_BT_CTLR_CONN_INTERVAL_LOW_LATENCY) &&
!IN_RANGE(lll->interval, BT_HCI_LE_INTERVAL_MIN, BT_HCI_LE_INTERVAL_MAX)) {
invalid_release(&adv->ull, lll, link, rx);

return;
Expand Down
Loading