Bluetooth: Controller: Fix div-by-zero with BT_CONN_PARAM_ANY #84154
Conversation
zephyrbot
requested review from
carlescufi,
erbr-ot,
mtpr-ot,
thoh-ot and
wopu-ot
| 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; | ||
| } |
There was a problem hiding this comment.
When can conn->lll.interval end up being < BT_HCI_LE_INTERVAL_MIN?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| } else { | ||
| conn_interval_us = (conn->lll.interval + 1U) * CONN_LOW_LAT_INT_UNIT_US; | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Fix div-by-zero usage fault when BT_CONN_PARAM_ANY is enabled and a value of 0 is used as connection interval value. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Disallow invalid connection interval value use in connection establishment. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
cvinayak
force-pushed
the
github_conn_interval_div_by_zero_fix
branch
from
bac6789 to
124104e
Compare
Fix div-by-zero usage fault when BT_CONN_PARAM_ANY is enabled and a value of 0 is used as connection interval value.