diff mbox series

Bluetooth: hci_event: Add missing locking on hdev in hci_le_ext_adv_term_evt

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

Commit Message

Niels Dossche Feb. 9, 2022, 2:08 p.m. UTC
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(-)

Comments

Marcel Holtmann March 4, 2022, 3:30 p.m. UTC | #1
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 mbox series

Patch

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,