Bluetooth: Host: Add missing buffer length check #74639
Conversation
zephyrbot
requested review from
alwa-nordic,
hermabe,
jhedberg,
jori-nordic,
sjanc,
Thalley and
theob-pro
jhedberg
dismissed
their stale review
Waiting for valid discovered issue to be resolved.
ekleezg
force-pushed
the
bt_host_length_check
branch
from
119b820 to
22c2261
Compare
ekleezg
requested review from
ssekar15,
MulinChao,
ChiHuaL,
sylvioalves,
yperess,
sjg20 and
jackrosenthal
as code owners
ekleezg
force-pushed
the
bt_host_length_check
branch
from
9c03965 to
081a7cb
Compare
Modified to check the length of the remaining data in buffer before processing the next report. The length check is missing in the cont routine. Signed-off-by: Eunkyu Lee <mochaccino.00.00@gmail.com>
ekleezg
force-pushed
the
bt_host_length_check
branch
from
959dc16 to
cfa51db
Compare
| if (evt_type & BT_HCI_LE_ADV_EVT_TYPE_LEGACY) { | ||
| return; | ||
| } | ||
|
|
||
| /* Start discarding irrespective of the `more_to_come` flag. We | ||
| * assume we may have lost a partial adv report in the truncated | ||
| * data. | ||
| */ | ||
| reassembling_advertiser.state = FRAG_ADV_DISCARDING; | ||
|
|
||
| return; |
There was a problem hiding this comment.
| if (evt_type & BT_HCI_LE_ADV_EVT_TYPE_LEGACY) { | |
| return; | |
| } | |
| /* Start discarding irrespective of the `more_to_come` flag. We | |
| * assume we may have lost a partial adv report in the truncated | |
| * data. | |
| */ | |
| reassembling_advertiser.state = FRAG_ADV_DISCARDING; | |
| return; | |
| if ((evt_type & BT_HCI_LE_ADV_EVT_TYPE_LEGACY) == 0) { | |
| /* Start discarding irrespective of the `more_to_come` flag. We | |
| * assume we may have lost a partial adv report in the truncated | |
| * data. | |
| */ | |
| reassembling_advertiser.state = FRAG_ADV_DISCARDING; | |
| } | |
| return; |
This would reduce the number of return statements to 1
|
@jhedberg should this get merged for 3.7? |
| if (evt_type & BT_HCI_LE_ADV_EVT_TYPE_LEGACY) { | ||
| return; | ||
| } | ||
|
|
||
| /* Start discarding irrespective of the `more_to_come` flag. We | ||
| * assume we may have lost a partial adv report in the truncated | ||
| * data. | ||
| */ | ||
| reassembling_advertiser.state = FRAG_ADV_DISCARDING; |
There was a problem hiding this comment.
I think the early return may cause us to produce garbled advertisement reports.
Start discarding irrespective of the
more_to_comeflag. We assume we may have lost a partial adv report in the truncated data.
The above comment is there because we are in a while-loop over reports. By returning early, we may be skipping over one or more reports. This can lead to a bad reassembly.
A bad reassembly is not a huge problem. The application has to tolerate malicious advertisements anyway. But when we did discussed this last time, we decided to prefer dropping data.
There was a problem hiding this comment.
can we come up with a test case for this somehow? I agree it's better to drop data instead of making a bad reassembly.
There was a problem hiding this comment.
but in this particular case, it's the controller that's malicious, no? so likely not a problem for "normal" use-cases. This early-return is more for protecting the host (and its CPU / resources I guess). My opinion is that we should just shut down bluetooth when that happens. It indicates something is wrong, either HCI driver synchronization or that the controller is very buggy or compromised.
There was a problem hiding this comment.
Yes. This is only relevant for HCI protocol violation. Either the controller did bad, or there was data corruption during HCI transport. It's not an exposed attack surface.
It gets to be merged :) |
After zephyrproject-rtos#74639 If we receive corrupted adv report, will set state to discarding, but if next adv report is new advertiser, will lead ASSERT. Signed-off-by: Lingao Meng <menglingao@xiaomi.com>
Modified to check the length of the remaining data in buffer before processing the next report. The length check is missing in the cont routine.