Bluetooth: Host: Fix bt_conn reservation leak for extended advertising

This fixes a leak for the following scenario:
A bonded peer connects via undirected adv set while directed connectable
extended advertisement runs on the same identity.
The enhanced connection complete is cached, not yet processed and the
undirected set is terminated, which clears the `BT_ADV_ENABLED` flag.
During processing of the cached enhanced connection complete event, the
`find_pending_connect()` function then finds the directed set instead of
the undirected set (as until now there was no distinction of the
advertising set type in the lookup function). This leads to a
reservation leak, as the directed set is not terminated (but the
`BT_ADV_ENABLED` flag is cleared).

This commits adds the advertising set context to the
`bt_hci_le_enh_conn_complete()` and `find_pending_connect()` functions,
so that the correct advertising set is used for the lookup.

Note: This change has been locally tested with a test case created by AI
that reproduces the above scenario.

Signed-off-by: Kyra Lengfeld <kyra.lengfeld@nordicsemi.no>

Author: KyraLengfeld

subsys/bluetooth/host/adv.c

@@ -2159,14 +2159,15 @@ void bt_hci_le_adv_set_terminated(struct net_buf *buf)
 		if (bt_dev.cached_conn_complete[i].valid &&
 		    bt_dev.cached_conn_complete[i].evt.handle == evt->conn_handle) {
 			if (was_adv_enabled) {
-				/* Process the cached connection complete event
-				 * now that the corresponding advertising set is known.
+				/* Process the cached connection complete event with the
+				 * advertising set context.
 				 *
 				 * If the advertiser has been stopped before the connection
 				 * complete event has been raised to the application, we
 				 * discard the event.
 				 */
-				bt_hci_le_enh_conn_complete(&bt_dev.cached_conn_complete[i].evt);
+				bt_hci_le_enh_conn_complete(&bt_dev.cached_conn_complete[i].evt,
+							    adv);
 			}
 			bt_dev.cached_conn_complete[i].valid = false;
 		}

subsys/bluetooth/host/hci_core.c

@@ -1258,7 +1258,8 @@ int bt_le_set_phy(struct bt_conn *conn, uint8_t all_phys,
 	return bt_hci_cmd_send_sync(BT_HCI_OP_LE_SET_PHY, buf, NULL);
 }
 
-static struct bt_conn *find_pending_connect(uint8_t role, bt_addr_le_t *peer_addr)
+static struct bt_conn *find_pending_connect(uint8_t role, const bt_addr_le_t *peer_addr,
+					    const struct bt_le_ext_adv *ext_adv)
 {
 	struct bt_conn *conn;
 
@@ -1279,6 +1280,33 @@ static struct bt_conn *find_pending_connect(uint8_t role, bt_addr_le_t *peer_add
 	}
 
 	if (IS_ENABLED(CONFIG_BT_PERIPHERAL) && role == BT_HCI_ROLE_PERIPHERAL) {
+		/* Do not fall back between directed and undirected pending connections
+		 * when the terminating advertising set is known. Such a fallback can
+		 * consume a reservation belonging to another active set.
+		 */
+		if (ext_adv != NULL) {
+			if (bt_addr_le_eq(&ext_adv->target_addr, BT_ADDR_LE_ANY)) {
+				/* Having multiple same-identity undirected reservations and
+				 * finding the first one that might not have been the one that
+				 * was used to initiate the connection is not a problem.
+				 * Undirected reservations have no advertising-set association
+				 * or per-set state, so any matching reservation can
+				 * be consumed; one remains for each other enabled set.
+				 */
+				return bt_conn_lookup_state_le(ext_adv->id, BT_ADDR_LE_NONE,
+							       BT_CONN_ADV_CONNECTABLE);
+			}
+
+			return bt_conn_lookup_state_le(ext_adv->id, &ext_adv->target_addr,
+						       BT_CONN_ADV_DIR_CONNECTABLE);
+		}
+
+		/* In case there is no advertising handle, there can be at most one
+		 * relevant peripheral advertiser. This is the case for legacy
+		 * advertising, or when the controller does not support extended
+		 * advertising. In this case, we can fall back to the legacy lookup
+		 * behaviour.
+		 */
 		conn = bt_conn_lookup_state_le(bt_dev.adv_conn_id, peer_addr,
 					       BT_CONN_ADV_DIR_CONNECTABLE);
 		if (!conn) {
@@ -1303,7 +1331,7 @@ static void le_conn_complete_cancel(uint8_t err)
 	 * There is no need to check ID address as only one
 	 * connection in central role can be in pending state.
 	 */
-	conn = find_pending_connect(BT_HCI_ROLE_CENTRAL, NULL);
+	conn = find_pending_connect(BT_HCI_ROLE_CENTRAL, NULL, NULL);
 	if (!conn) {
 		LOG_ERR("No pending central connection");
 		return;
@@ -1363,7 +1391,7 @@ static void le_conn_complete_adv_timeout(void)
 		/* There is no need to check ID address as only one
 		 * connection in peripheral role can be in pending state.
 		 */
-		conn = find_pending_connect(BT_HCI_ROLE_PERIPHERAL, NULL);
+		conn = find_pending_connect(BT_HCI_ROLE_PERIPHERAL, NULL, NULL);
 		if (!conn) {
 			LOG_ERR("No pending peripheral connection");
 			return;
@@ -1404,7 +1432,7 @@ static void enh_conn_complete(struct bt_hci_evt_le_enh_conn_complete *evt)
 		return;
 	}
 #endif
-	bt_hci_le_enh_conn_complete(evt);
+	bt_hci_le_enh_conn_complete(evt, NULL);
 }
 
 static void translate_addrs(bt_addr_le_t *peer_addr, bt_addr_le_t *id_addr,
@@ -1442,7 +1470,8 @@ static void update_conn(struct bt_conn *conn, const bt_addr_le_t *id_addr,
 #endif
 }
 
-void bt_hci_le_enh_conn_complete(struct bt_hci_evt_le_enh_conn_complete *evt)
+void bt_hci_le_enh_conn_complete(struct bt_hci_evt_le_enh_conn_complete *evt,
+				 const struct bt_le_ext_adv *ext_adv)
 {
 	__ASSERT_NO_MSG(evt->status == BT_HCI_ERR_SUCCESS);
 
@@ -1461,10 +1490,13 @@ void bt_hci_le_enh_conn_complete(struct bt_hci_evt_le_enh_conn_complete *evt)
 	bt_id_pending_keys_update();
 #endif
 
-	id = evt->role == BT_HCI_ROLE_PERIPHERAL ? bt_dev.adv_conn_id : BT_ID_DEFAULT;
+	id = BT_ID_DEFAULT;
+	if (evt->role == BT_HCI_ROLE_PERIPHERAL) {
+		id = ext_adv != NULL ? ext_adv->id : bt_dev.adv_conn_id;
+	}
 	translate_addrs(&peer_addr, &id_addr, evt, id);
 
-	conn = find_pending_connect(evt->role, &id_addr);
+	conn = find_pending_connect(evt->role, &id_addr, ext_adv);
 
 	if (IS_ENABLED(CONFIG_BT_CENTRAL) &&
 	    evt->role == BT_HCI_ROLE_CENTRAL) {

subsys/bluetooth/host/hci_core.h

@@ -342,14 +342,15 @@ struct bt_dev {
 	/* Pointer to reserved advertising set */
 	struct bt_le_ext_adv    *adv;
 #if defined(CONFIG_BT_CONN) && (CONFIG_BT_EXT_ADV_MAX_ADV_SET > 1)
-	/* When supporting multiple concurrent connectable advertising sets
-	 * with multiple identities, we need to know the identity of
-	 * the terminating advertising set to identify the connection object.
-	 * The identity of the advertising set is determined by its
-	 * advertising handle, which is part of the
-	 * LE Set Advertising Set Terminated event which is always sent
-	 * _after_ the LE Enhanced Connection complete event.
-	 * Therefore we need cache this event until its identity is known.
+	/* When supporting multiple concurrent connectable advertising sets,
+	 * we need to know the identity of the terminating advertising set to
+	 * identify the connection object. If multiple sets share an identity,
+	 * we also need to know whether the terminating set is directed or
+	 * undirected to select the corresponding connection reservation. The
+	 * advertising set is identified by its advertising handle, which is part
+	 * of the LE Advertising Set Terminated event which is always sent _after_
+	 * the LE Enhanced Connection Complete event. Therefore we need to cache
+	 * this event until its advertising set is known.
 	 */
 	struct {
 		bool valid;
@@ -516,7 +517,8 @@ void bt_hci_user_passkey_req(struct net_buf *buf);
 void bt_hci_auth_complete(struct net_buf *buf);
 
 /* Common HCI event handlers */
-void bt_hci_le_enh_conn_complete(struct bt_hci_evt_le_enh_conn_complete *evt);
+void bt_hci_le_enh_conn_complete(struct bt_hci_evt_le_enh_conn_complete *evt,
+				 const struct bt_le_ext_adv *ext_adv);
 
 /* Scan HCI event handlers */
 void bt_hci_le_adv_report(struct net_buf *buf);