Bluetooth: Host: Add conn rsp param check #93174
Conversation
subsys/bluetooth/host/l2cap.c
Outdated
| */ | ||
| if (result == BT_L2CAP_LE_SUCCESS) { | ||
| if (!L2CAP_LE_CID_IS_DYN(dcid)) { | ||
| LOG_ERR("dcid 0x%04x is not dynamic", dcid); |
There was a problem hiding this comment.
We should use LOG_WRN for errors caused by the peer. LOG_ERR should be reserved for errors that the developer can do something about.
subsys/bluetooth/host/l2cap.c
Outdated
| if (result == BT_L2CAP_LE_SUCCESS) { | ||
| if (!L2CAP_LE_CID_IS_DYN(dcid)) { | ||
| LOG_ERR("dcid 0x%04x is not dynamic", dcid); | ||
| result = BT_L2CAP_LE_ERR_UNACCEPT_PARAMS; |
There was a problem hiding this comment.
Pretending that the remote peer answered BT_L2CAP_LE_ERR_UNACCEPT_PARAMS may not be optimal.
I'm tempted to say that it would be better to disconnect the ACL connection since there has been a protocol error on the L2CAP signalling channel.
If we don't disconnect, then the peer may believe the L2CAP connection is successful, while our side has closed this channel. But you can argue that the peer invoked undefined behavior so we can leave it hanging.
But telling our application or upper layers (ecred_cb->ecred_conn_rsp) that the peer responded with something different than it actually did is something we should do very carefully. It can make debugging harder.
Even if we didn't propagate result anywhere, clearly this is a just a clever trick to reuse existing code. I think we should avoid clever code if possible. I think it may be a better engineering decision to add new values for result. It may infringe on reserved values in the spec, but I would argue that that is a lesser evil. Or we could make result be a 32-bit integer instead, giving us room for custom codes. But both of these options may be API breaking to a certain extent. I would like some discussion on this.
Since the correct way to handle this case is not obvious, I would like a comment that explains the choice we make here and why it's fine.
Also, if we are to pretend the peer responded with something, then BT_L2CAP_LE_ERR_INVALID_PARAMS would be slightly more appropriate.
Change requested: Please at least use BT_L2CAP_LE_ERR_INVALID_PARAMS and write a comment documenting the engineering decisions. (Alternatively let's continue this discussion.)
There was a problem hiding this comment.
Good points!
What if we instead, if any of the checks fail run bt_l2cap_chan_remove(), bt_l2cap_chan_del() and bt_l2cap_chan_disconnect(). Should we/do we need to run l2cap_remove_ident and cancel chan->rtx_work as well?
There was a problem hiding this comment.
I changed it to instead return early/do its own disconnecting, which seems fine according to Core 1.E.2.7 (What to do regarding invalid behavior).
Although Core 3.A.4.6 says
Terminating an L2CAP channel requires that an L2CAP_DISCONNECTION_REQ packet be sent and acknowledged by an L2CAP_DISCONNECTION_RSP packet
I'm not quite sure which has precedence (would think the one regarding invalid behavior)?
zephyrbot
added
area: Bluetooth Host
zephyrbot
requested review from
cvinayak,
hermabe,
jhedberg,
rugeGerritsen,
sjanc and
Thalley
Check whether the connection response parameters both with and without ECRED are within the valid ranges from the Bluetooth Core Specification (part 3.A.4 v6.0). Signed-off-by: Håvard Reierstad <[email protected]>
HaavardRei
force-pushed
the
conn_rsp_validation
branch
from
57f5b75 to
86ec0c2
Compare
|
Check whether the connection response parameters both with and without ECRED are within the valid ranges from the Bluetooth Core Specification (part 3.A.4 v6.0).