| Message ID | 436acf8a-ea5f-a308-0e3e-fc7c6ffde7aa@ugent.be |
|---|---|
| State | Accepted |
| Commit | 728abc0151b06b3cb1d084ca2ee7418c3fad89e2 |
| Headers | show |
| Series | Bluetooth: hci_event: Add missing locking on hdev in hci_le_ext_adv_term_evt | expand |
Hi Niels, > Both hci_find_adv_instance and hci_remove_adv_instance have a comment > above their function definition saying that these two functions require > the caller to hold the hdev->lock lock. However, hci_le_ext_adv_term_evt > does not acquire that lock and neither does its caller hci_le_meta_evt > (hci_le_meta_evt calls hci_le_ext_adv_term_evt via an indirect function > call because of the lookup in hci_le_ev_table). > > The other event handlers all acquire and release the hdev->lock and they > follow the rule that hci_find_adv_instance and hci_remove_adv_instance > must be called while holding the hdev->lock lock. > > The solution is to make sure hci_le_ext_adv_term_evt also acquires and > releases the hdev->lock lock. The check on ev->status which logs a > warning and does an early return is not covered by the lock because > other functions also access ev->status without holding the lock. > > Signed-off-by: Niels Dossche <niels.dossche@ugent.be> > --- > net/bluetooth/hci_event.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) patch has been applied to bluetooth-next tree. Regards Marcel
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index fc30f4c03d29..3bf048d0df37 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -5670,8 +5670,6 @@ static void hci_le_ext_adv_term_evt(struct hci_dev *hdev, void *data, bt_dev_dbg(hdev, "status 0x%2.2x", ev->status); - adv = hci_find_adv_instance(hdev, ev->handle); - /* The Bluetooth Core 5.3 specification clearly states that this event * shall not be sent when the Host disables the advertising set. So in * case of HCI_ERROR_CANCELLED_BY_HOST, just ignore the event. @@ -5684,9 +5682,13 @@ static void hci_le_ext_adv_term_evt(struct hci_dev *hdev, void *data, return; } + hci_dev_lock(hdev); + + adv = hci_find_adv_instance(hdev, ev->handle); + if (ev->status) { if (!adv) - return; + goto unlock; /* Remove advertising as it has been terminated */ hci_remove_adv_instance(hdev, ev->handle); @@ -5694,12 +5696,12 @@ static void hci_le_ext_adv_term_evt(struct hci_dev *hdev, void *data, list_for_each_entry_safe(adv, n, &hdev->adv_instances, list) { if (adv->enabled) - return; + goto unlock; } /* We are no longer advertising, clear HCI_LE_ADV */ hci_dev_clear_flag(hdev, HCI_LE_ADV); - return; + goto unlock; } if (adv) @@ -5714,16 +5716,19 @@ static void hci_le_ext_adv_term_evt(struct hci_dev *hdev, void *data, if (hdev->adv_addr_type != ADDR_LE_DEV_RANDOM || bacmp(&conn->resp_addr, BDADDR_ANY)) - return; + goto unlock; if (!ev->handle) { bacpy(&conn->resp_addr, &hdev->random_addr); - return; + goto unlock; } if (adv) bacpy(&conn->resp_addr, &adv->random_addr); } + +unlock: + hci_dev_unlock(hdev); } static void hci_le_conn_update_complete_evt(struct hci_dev *hdev, void *data,
Both hci_find_adv_instance and hci_remove_adv_instance have a comment above their function definition saying that these two functions require the caller to hold the hdev->lock lock. However, hci_le_ext_adv_term_evt does not acquire that lock and neither does its caller hci_le_meta_evt (hci_le_meta_evt calls hci_le_ext_adv_term_evt via an indirect function call because of the lookup in hci_le_ev_table). The other event handlers all acquire and release the hdev->lock and they follow the rule that hci_find_adv_instance and hci_remove_adv_instance must be called while holding the hdev->lock lock. The solution is to make sure hci_le_ext_adv_term_evt also acquires and releases the hdev->lock lock. The check on ev->status which logs a warning and does an early return is not covered by the lock because other functions also access ev->status without holding the lock. Signed-off-by: Niels Dossche <niels.dossche@ugent.be> --- net/bluetooth/hci_event.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)