[PATCH 00/11] Bluetooth: ISO: fix HUP on socket release/shutdown + UAF/locking fixes

[PATCH 00/11] Bluetooth: ISO: fix HUP on socket release/shutdown + UAF/locking fixes

[Pauli Virtanen]

The retracted patch in commit e824c0bbe0ec9 ("Bluetooth: ISO: clear
iso_data always when detaching conn from hcon") merged in
bluetooth-next/master broke ISO socket transition to BT_CLOSED so they
don't send POLLHUP properly any more, as seen in "ISO Disconnect -
Success" test.

This is rebased version of and supercedes
https://lore.kernel.org/linux-bluetooth/fbd9dd573bb1ce5f38370128dd35447e2ddfed9c.1784625576.git.pav@iki.fi/

The first commit in this series fixes that, and subsequent mostly
independent commits other minor issues found looking at the ISO socket
code.

The last three fix UAF issues. The locking/lifetime of iso_conn
complicates the code here and the last three patches can be replaced by
somewhat simpler solution with bigger diff that removes iso_conn
https://github.com/pv/linux/commit/e4b460b5f7aaba060b958229aaec6b2217642178

Fixed issues aside BT_CONNECT, these appear to be pre-existing in
several previous kernel releases:

- sk is always leaked on socket release

- timeout_work may deadlock under certain conditions

- correctness of iso_conn_del() vs UAF requires somewhat too complex
  reasoning about race conditions, and it's not quite correct

        [Task 1]                      [Task hdev->workqueue]
        iso_sock_timeout              iso_conn_del
          iso_conn_hold_unless_zero     iso_chan_del
                           `------------> iso_conn_put
                                      caller frees hcon
          iso_conn_put
            iso_conn_free
              conn->hcon->iso_data = NULL; /* UAF */

- attempt to free iso_conn in iso_conn_del() races with iso_conn_del
  with potential UAF

- iso_sock_ready() gets conn->sk without requisite locks, this should
  be done like in sco.c

- iso_sock_getname, iso_connect_ind are missing lock_sock and NULL
  checks

- kref_get_unless_zero(&((struct iso_conn *)hcon->iso_data)->ref)
  requires synchronization primitive.

Tested vs iso-tester, Pipewire Qemu ucast/bcast audio tests, and real
ucast audio streaming, which pass without KASAN/locking splats. Also
checked iso-tester produces balanced iso_sock_init / iso_sock_destruct
kprints.

Pauli Virtanen (11):
  Bluetooth: ISO: fix CONNECTED -> CLOSED transition on shutdown/release
  Bluetooth: ISO: lock sk in iso_sock_getname
  Bluetooth: ISO: lock sk in iso_connect_ind
  Bluetooth: ISO: fix timeout vs sync_timeout typo in check_bcast_qos
  Bluetooth: ISO: validate sockaddr_iso first in iso_sock_rebind_bis()
  Bluetooth: ISO: hold sk properly in iso_conn_ready
  Bluetooth: ISO: fix leaking sk after socket release
  Bluetooth: ISO: avoid deadlocks in iso_sock_timeout
  Bluetooth: ISO: ensure no dangling hcon references in iso_conn
  Bluetooth: ISO: fix refcounting of iso_conn
  Bluetooth: ISO: fix race of kfree vs kref_get_unless_zero

 include/net/bluetooth/hci_core.h |   4 +-
 net/bluetooth/hci_conn.c         |   2 +
 net/bluetooth/iso.c              | 274 ++++++++++++++++++++++---------
 3 files changed, 197 insertions(+), 83 deletions(-)

-- 
2.55.0

[PATCH 01/11] Bluetooth: ISO: fix CONNECTED -> CLOSED transition on shutdown/release

[Pauli Virtanen]

Commit e824c0bbe0ec ("Bluetooth: ISO: clear iso_data always when detaching conn from hcon")
merged a version of the UAF fix that breaks releasing connected
ISO sockets. Since hci_conn::iso_data is set to NULL, iso_chan_del() won't
be called when the hci_conn disconnects, and the ISO socket does not emit
POLLHUP correctly.

Fix by retaining full hci_conn <-> iso_conn association while in
BT_DISCONNECT state, so that local disconnect via shutdown() follows
similar ISO socket code path as remote disconnect.  Use a separate flag
to track whether hci_conn_drop() is needed, instead of setting
iso_conn::hcon = NULL

In iso_sock_ready(), disallow disconnecting socket going BT_CONNECTED,
in case hcon connects while its drop is pending.

Fixes: e824c0bbe0ec ("Bluetooth: ISO: clear iso_data always when detaching conn from hcon")
Fixes: fbdc4bc47268 ("Bluetooth: ISO: Use defer setup to separate PA sync and BIG sync")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    This is rebased version of and supercedes
    
    https://lore.kernel.org/linux-bluetooth/fbd9dd573bb1ce5f38370128dd35447e2ddfed9c.1784625576.git.pav@iki.fi/

 net/bluetooth/iso.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index babba61eb335..299a9336b5e1 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -24,8 +24,14 @@ static struct bt_sock_list iso_sk_list = {
 };
 
 /* ---- ISO connections ---- */
+enum {
+	ISO_CONN_DROPPED,
+	__ISO_CONN_NUM_FLAGS
+};
+
 struct iso_conn {
 	struct hci_conn	*hcon;
+	DECLARE_BITMAP(flags, __ISO_CONN_NUM_FLAGS);
 
 	/* @lock: spinlock protecting changes to iso_conn fields */
 	spinlock_t	lock;
@@ -107,7 +113,8 @@ static void iso_conn_free(struct kref *ref)
 
 	if (conn->hcon) {
 		conn->hcon->iso_data = NULL;
-		hci_conn_drop(conn->hcon);
+		if (!test_and_set_bit(ISO_CONN_DROPPED, conn->flags))
+			hci_conn_drop(conn->hcon);
 	}
 
 	/* Ensure no more work items will run since hci_conn has been dropped */
@@ -306,6 +313,7 @@ static int __iso_chan_add(struct iso_conn *conn, struct sock *sk,
 
 	iso_pi(sk)->conn = conn;
 	conn->sk = sk;
+	clear_bit(ISO_CONN_DROPPED, conn->flags);
 
 	if (parent)
 		bt_accept_enqueue(parent, sk, true);
@@ -835,11 +843,8 @@ static void iso_sock_disconn(struct sock *sk)
 	}
 
 	sk->sk_state = BT_DISCONN;
-	iso_conn_lock(iso_pi(sk)->conn);
-	hci_conn_drop(iso_pi(sk)->conn->hcon);
-	iso_pi(sk)->conn->hcon->iso_data = NULL;
-	iso_pi(sk)->conn->hcon = NULL;
-	iso_conn_unlock(iso_pi(sk)->conn);
+	if (!test_and_set_bit(ISO_CONN_DROPPED, iso_pi(sk)->conn->flags))
+		hci_conn_drop(iso_pi(sk)->conn->hcon);
 }
 
 static void __iso_sock_close(struct sock *sk)
@@ -2042,9 +2047,18 @@ static void iso_sock_ready(struct sock *sk)
 		return;
 
 	lock_sock(sk);
+
+	switch (sk->sk_state) {
+	case BT_DISCONN:
+	case BT_CLOSED:
+		release_sock(sk);
+		return;
+	}
+
 	iso_sock_clear_timer(sk);
 	sk->sk_state = BT_CONNECTED;
 	sk->sk_state_change(sk);
+
 	release_sock(sk);
 }
 
-- 
2.55.0

[PATCH 02/11] Bluetooth: ISO: lock sk in iso_sock_getname

[Pauli Virtanen]

Accessing iso_pi(sk)->conn requires lock_sock, which is not held here.

Fix by adding the lock/release.

Fixes: 2df108c227b2 ("Bluetooth: ISO: Fix using BT_SK_PA_SYNC to detect BIS sockets")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/iso.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 299a9336b5e1..dbb8f43052f0 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1472,6 +1472,8 @@ static int iso_sock_getname(struct socket *sock, struct sockaddr *addr,
 
 	BT_DBG("sock %p, sk %p", sock, sk);
 
+	lock_sock(sk);
+
 	addr->sa_family = AF_BLUETOOTH;
 
 	if (peer) {
@@ -1493,6 +1495,8 @@ static int iso_sock_getname(struct socket *sock, struct sockaddr *addr,
 		sa->iso_bdaddr_type = iso_pi(sk)->src_type;
 	}
 
+	release_sock(sk);
+
 	return len;
 }
 
-- 
2.55.0

[PATCH 03/11] Bluetooth: ISO: lock sk in iso_connect_ind

[Pauli Virtanen]

Accessing iso_pi(sk)->conn requires lock_sock, which is not taken in the
"ev3" part of iso_connect_ind.  It may also be NULL if socket has
transitioned away from the LISTEN/CONNECT states before locking.

Fix by adding lock/release. Recheck hcon is valid after lock acquire
where needed.

Fixes: 168d9bf9c7f0 ("Bluetooth: ISO: Reassemble PA data for bcast sink")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/iso.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index dbb8f43052f0..651661833966 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -2369,7 +2369,7 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags)
 
 			lock_sock(sk);
 
-			hcon = iso_pi(sk)->conn->hcon;
+			hcon = iso_pi(sk)->conn ? iso_pi(sk)->conn->hcon : NULL;
 			iso_pi(sk)->qos.bcast.encryption = ev2->encryption;
 
 			if (ev2->num_bis < iso_pi(sk)->bc_num_bis)
@@ -2409,9 +2409,11 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags)
 		if (!sk)
 			goto done;
 
-		hcon = iso_pi(sk)->conn->hcon;
+		lock_sock(sk);
+
+		hcon = iso_pi(sk)->conn ? iso_pi(sk)->conn->hcon : NULL;
 		if (!hcon)
-			goto done;
+			goto release3;
 
 		if (ev3->data_status == LE_PA_DATA_TRUNCATED) {
 			/* The controller was unable to retrieve PA data. */
@@ -2419,12 +2421,12 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags)
 			       HCI_MAX_PER_AD_TOT_LEN);
 			hcon->le_per_adv_data_len = 0;
 			hcon->le_per_adv_data_offset = 0;
-			goto done;
+			goto release3;
 		}
 
 		if (hcon->le_per_adv_data_offset + ev3->length >
 		    HCI_MAX_PER_AD_TOT_LEN)
-			goto done;
+			goto release3;
 
 		memcpy(hcon->le_per_adv_data + hcon->le_per_adv_data_offset,
 		       ev3->data, ev3->length);
@@ -2443,18 +2445,19 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags)
 						    &base_len);
 
 			if (!base || base_len > BASE_MAX_LENGTH)
-				goto done;
+				goto release3;
 
-			lock_sock(sk);
 			memcpy(iso_pi(sk)->base, base, base_len);
 			iso_pi(sk)->base_len = base_len;
-			release_sock(sk);
 		} else {
 			/* This is a PA data fragment. Keep pa_data_len set to 0
 			 * until all data has been reassembled.
 			 */
 			hcon->le_per_adv_data_len = 0;
 		}
+
+release3:
+		release_sock(sk);
 	} else {
 		sk = iso_get_sock(hdev, &hdev->bdaddr, BDADDR_ANY,
 				  BT_LISTEN, iso_match_dst, BDADDR_ANY);
-- 
2.55.0

[PATCH 04/11] Bluetooth: ISO: fix timeout vs sync_timeout typo in check_bcast_qos

[Pauli Virtanen]

In iso.c check_bcast_qos(), missing bcast.timeout is not set to its
default value, and appears typoed as bcast.sync_timeout.

Fix the typo.

Fixes: b37cab587aa3 ("Bluetooth: ISO: Don't reject BT_ISO_QOS if parameters are unset")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/iso.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 651661833966..e51253e5c161 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1796,7 +1796,7 @@ static bool check_bcast_qos(struct bt_iso_qos *qos)
 		return false;
 
 	if (!qos->bcast.timeout)
-		qos->bcast.sync_timeout = BT_ISO_SYNC_TIMEOUT;
+		qos->bcast.timeout = BT_ISO_SYNC_TIMEOUT;
 
 	if (qos->bcast.timeout < 0x000a || qos->bcast.timeout > 0x4000)
 		return false;
-- 
2.55.0

[PATCH 05/11] Bluetooth: ISO: validate sockaddr_iso first in iso_sock_rebind_bis()

[Pauli Virtanen]

iso_sock_rebind_bis() updates socket iso_pi(sk)->bc_num_bis before
validating the BIS values, so it's possible to end up with bc_num_bis
inconsistent.

Assign to iso_pi(sk)->bc_num_bis only after validation.

Fixes: 80837140c1f2 ("Bluetooth: ISO: Allow binding a PA sync socket")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/iso.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index e51253e5c161..5de4a2f886eb 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1039,15 +1039,15 @@ static int iso_sock_rebind_bis(struct sock *sk, struct sockaddr_iso *sa,
 		goto done;
 	}
 
-	iso_pi(sk)->bc_num_bis = sa->iso_bc->bc_num_bis;
-
-	for (int i = 0; i < iso_pi(sk)->bc_num_bis; i++)
+	for (int i = 0; i < sa->iso_bc->bc_num_bis; i++)
 		if (sa->iso_bc->bc_bis[i] < 0x01 ||
 		    sa->iso_bc->bc_bis[i] > 0x1f) {
 			err = -EINVAL;
 			goto done;
 		}
 
+	iso_pi(sk)->bc_num_bis = sa->iso_bc->bc_num_bis;
+
 	memcpy(iso_pi(sk)->bc_bis, sa->iso_bc->bc_bis,
 	       iso_pi(sk)->bc_num_bis);
 
-- 
2.55.0

[PATCH 06/11] Bluetooth: ISO: hold sk properly in iso_conn_ready

[Pauli Virtanen]

sk deref in iso_conn_ready must be done either under conn->lock, or
holding a refcount, to avoid concurrent close. conn->sk is currently
accessed without either:

    [Task 1]            [Task 2]
                        iso_sock_release
    iso_conn_ready
      sk = conn->sk
                          lock_sock(sk)
                            conn->sk = NULL
      lock_sock(sk)
                          release_sock(sk)
                          iso_sock_kill(sk)
       UAF on sk deref

Fix possible UAF by holding sk refcount in iso_conn_ready().  Also
recheck after lock_sock that the socket is still valid.  Adjust locking
so conn->sk is cleared only under lock_sock.

Fixes: 27c24fda62b60 ("Bluetooth: switch to lock_sock in SCO")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/iso.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 5de4a2f886eb..80a58275891d 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -805,11 +805,13 @@ static void iso_sock_kill(struct sock *sk)
 	BT_DBG("sk %p state %d", sk, sk->sk_state);
 
 	/* Sock is dead, so set conn->sk to NULL to avoid possible UAF */
+	lock_sock(sk);
 	if (iso_pi(sk)->conn) {
 		iso_conn_lock(iso_pi(sk)->conn);
 		iso_pi(sk)->conn->sk = NULL;
 		iso_conn_unlock(iso_pi(sk)->conn);
 	}
+	release_sock(sk);
 
 	/* Kill poor orphan */
 	bt_sock_unlink(&iso_sk_list, sk);
@@ -2047,23 +2049,17 @@ static void iso_sock_ready(struct sock *sk)
 {
 	BT_DBG("sk %p", sk);
 
-	if (!sk)
-		return;
-
-	lock_sock(sk);
+	lockdep_assert(lockdep_sock_is_held(sk));
 
 	switch (sk->sk_state) {
 	case BT_DISCONN:
 	case BT_CLOSED:
-		release_sock(sk);
 		return;
 	}
 
 	iso_sock_clear_timer(sk);
 	sk->sk_state = BT_CONNECTED;
 	sk->sk_state_change(sk);
-
-	release_sock(sk);
 }
 
 static bool iso_match_big(struct sock *sk, void *data)
@@ -2093,7 +2089,7 @@ static bool iso_match_dst(struct sock *sk, void *data)
 static void iso_conn_ready(struct iso_conn *conn)
 {
 	struct sock *parent = NULL;
-	struct sock *sk = conn->sk;
+	struct sock *sk;
 	struct hci_ev_le_big_sync_established *ev = NULL;
 	struct hci_ev_le_pa_sync_established *ev2 = NULL;
 	struct hci_ev_le_per_adv_report *ev3 = NULL;
@@ -2102,7 +2098,22 @@ static void iso_conn_ready(struct iso_conn *conn)
 
 	BT_DBG("conn %p", conn);
 
+	iso_conn_lock(conn);
+	sk = iso_sock_hold(conn);
+	iso_conn_unlock(conn);
+
 	if (sk) {
+		lock_sock(sk);
+
+		/* conn->sk may have become NULL if racing with sk close, but
+		 * due to held hdev->lock, it can't become different sk.
+		 */
+		if (!conn->sk) {
+			release_sock(sk);
+			sock_put(sk);
+			return;
+		}
+
 		/* Attempt to update source address in case of BIS Sender if
 		 * the advertisement is using a random address.
 		 */
@@ -2115,14 +2126,15 @@ static void iso_conn_ready(struct iso_conn *conn)
 			adv = hci_find_adv_instance(bis->hdev,
 						    bis->iso_qos.bcast.bis);
 			if (adv && bacmp(&adv->random_addr, BDADDR_ANY)) {
-				lock_sock(sk);
 				iso_pi(sk)->src_type = BDADDR_LE_RANDOM;
 				bacpy(&iso_pi(sk)->src, &adv->random_addr);
-				release_sock(sk);
 			}
 		}
 
-		iso_sock_ready(conn->sk);
+		iso_sock_ready(sk);
+
+		release_sock(sk);
+		sock_put(sk);
 	} else {
 		hcon = conn->hcon;
 		if (!hcon)
-- 
2.55.0

[PATCH 07/11] Bluetooth: ISO: fix leaking sk after socket release

[Pauli Virtanen]

iso_sock_kill() tests !sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket ||
sock_flag(sk, SOCK_DEAD) for early return, but this is always true since
sock_orphan(sk) sets SOCK_DEAD, so the sk reference released by socket
always leaks, iso_sock_destruct is never called.

The socket reference also leaks when __iso_sock_close() does not set
SOCK_ZAPPED, since iso_conn_del() does not call iso_sock_kill() after
zapping.

Fix by replacing SOCK_DEAD by BT_SK_KILLED flag that is not used for
something else, and lock_sock to ensure iso_sock_kill() puts sk only
after socket release only once. Release and iso_conn_del may run
concurrently. Call iso_sock_kill() from iso_conn_del() to clean sk up
after zapping.

Remove call to iso_sock_kill() from iso_sock_close(), as it's generally
no-op there.

Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/iso.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 80a58275891d..5f0f45a573a7 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -62,6 +62,7 @@ static void iso_sock_kill(struct sock *sk);
 enum {
 	BT_SK_BIG_SYNC,
 	BT_SK_PA_SYNC,
+	BT_SK_KILLED,
 };
 
 struct iso_pinfo {
@@ -295,6 +296,7 @@ static void iso_conn_del(struct hci_conn *hcon, int err)
 	iso_sock_clear_timer(sk);
 	iso_chan_del(sk, err);
 	release_sock(sk);
+	iso_sock_kill(sk);
 	sock_put(sk);
 }
 
@@ -798,24 +800,29 @@ static void iso_sock_cleanup_listen(struct sock *parent)
  */
 static void iso_sock_kill(struct sock *sk)
 {
+	lock_sock(sk);
+
 	if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket ||
-	    sock_flag(sk, SOCK_DEAD))
+	    test_bit(BT_SK_KILLED, &iso_pi(sk)->flags)) {
+		release_sock(sk);
 		return;
+	}
 
 	BT_DBG("sk %p state %d", sk, sk->sk_state);
 
 	/* Sock is dead, so set conn->sk to NULL to avoid possible UAF */
-	lock_sock(sk);
 	if (iso_pi(sk)->conn) {
 		iso_conn_lock(iso_pi(sk)->conn);
 		iso_pi(sk)->conn->sk = NULL;
 		iso_conn_unlock(iso_pi(sk)->conn);
 	}
-	release_sock(sk);
 
 	/* Kill poor orphan */
 	bt_sock_unlink(&iso_sk_list, sk);
 	sock_set_flag(sk, SOCK_DEAD);
+	set_bit(BT_SK_KILLED, &iso_pi(sk)->flags);
+
+	release_sock(sk);
 	sock_put(sk);
 }
 
@@ -892,7 +899,6 @@ static void iso_sock_close(struct sock *sk)
 	iso_sock_clear_timer(sk);
 	__iso_sock_close(sk);
 	release_sock(sk);
-	iso_sock_kill(sk);
 }
 
 static void iso_sock_init(struct sock *sk, struct sock *parent)
@@ -2040,8 +2046,16 @@ static int iso_sock_release(struct socket *sock)
 		release_sock(sk);
 	}
 
+	/* Make sure sk is valid even if iso_conn_del() is concurrent */
+	sock_hold(sk);
+
+	lock_sock(sk);
 	sock_orphan(sk);
+	release_sock(sk);
+
 	iso_sock_kill(sk);
+
+	sock_put(sk);
 	return err;
 }
 
-- 
2.55.0

[PATCH 08/11] Bluetooth: ISO: avoid deadlocks in iso_sock_timeout

[Pauli Virtanen]

iso_sock_timeout() takes lock_sock, so sync disabling the timer while
holding that lock may deadlock.

iso_sock_timeout() may also run concurrently with iso_conn_del(), which
leads to UAF

        [Task 1]                      [Task hdev->workqueue]
        iso_sock_timeout              iso_conn_del
          iso_conn_hold_unless_zero     iso_chan_del
                           `------------> iso_conn_put
                                      caller frees hcon
          iso_conn_put
            iso_conn_free
              conn->hcon->iso_data = NULL; /* UAF */

Fix the deadlock by removing the disable from the lock_sock sections.
Move the timer from iso_conn to iso_pinfo to decouple it from iso_conn
which may need to be freed in lock_sock section. Convert some of the
clear_timer to disable_timer.

Fixes: dc26097bdb86 ("Bluetooth: ISO: Use kref to track lifetime of iso_conn")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/iso.c | 60 ++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 31 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 5f0f45a573a7..0cc08416fde5 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -37,8 +37,6 @@ struct iso_conn {
 	spinlock_t	lock;
 	struct sock	*sk;
 
-	struct delayed_work	timeout_work;
-
 	struct sk_buff	*rx_skb;
 	__u32		rx_len;
 	__u16		tx_sn;
@@ -81,6 +79,7 @@ struct iso_pinfo {
 	__u8			base_len;
 	__u8			base[BASE_MAX_LENGTH];
 	struct iso_conn		*conn;
+	struct delayed_work	timeout_work;
 };
 
 static struct bt_iso_qos default_qos;
@@ -118,9 +117,6 @@ static void iso_conn_free(struct kref *ref)
 			hci_conn_drop(conn->hcon);
 	}
 
-	/* Ensure no more work items will run since hci_conn has been dropped */
-	disable_delayed_work_sync(&conn->timeout_work);
-
 	kfree_skb(conn->rx_skb);
 
 	kfree(conn);
@@ -161,48 +157,45 @@ static struct sock *iso_sock_hold(struct iso_conn *conn)
 
 static void iso_sock_timeout(struct work_struct *work)
 {
-	struct iso_conn *conn = container_of(work, struct iso_conn,
-					     timeout_work.work);
-	struct sock *sk;
-
-	conn = iso_conn_hold_unless_zero(conn);
-	if (!conn)
-		return;
-
-	iso_conn_lock(conn);
-	sk = iso_sock_hold(conn);
-	iso_conn_unlock(conn);
-	iso_conn_put(conn);
-
-	if (!sk)
-		return;
+	struct iso_pinfo *pi = container_of(work, struct iso_pinfo,
+					    timeout_work.work);
+	struct sock *sk = &pi->bt.sk;
 
 	BT_DBG("sock %p state %d", sk, sk->sk_state);
 
 	lock_sock(sk);
-	sk->sk_err = ETIMEDOUT;
-	sk->sk_state_change(sk);
+	if (!sock_flag(sk, SOCK_ZAPPED)) {
+		sk->sk_err = ETIMEDOUT;
+		sk->sk_state_change(sk);
+	}
 	release_sock(sk);
-	sock_put(sk);
 }
 
 static void iso_sock_set_timer(struct sock *sk, long timeout)
 {
+	lockdep_assert(lockdep_sock_is_held(sk));
+
+	cancel_delayed_work(&iso_pi(sk)->timeout_work);
+
 	if (!iso_pi(sk)->conn)
 		return;
 
 	BT_DBG("sock %p state %d timeout %ld", sk, sk->sk_state, timeout);
-	cancel_delayed_work(&iso_pi(sk)->conn->timeout_work);
-	schedule_delayed_work(&iso_pi(sk)->conn->timeout_work, timeout);
+	schedule_delayed_work(&iso_pi(sk)->timeout_work, timeout);
 }
 
 static void iso_sock_clear_timer(struct sock *sk)
 {
-	if (!iso_pi(sk)->conn)
-		return;
+	BT_DBG("sock %p state %d", sk, sk->sk_state);
+	cancel_delayed_work(&iso_pi(sk)->timeout_work);
+}
+
+static void iso_sock_disable_timer(struct sock *sk)
+{
+	lockdep_assert(!lockdep_sock_is_held(sk));
 
 	BT_DBG("sock %p state %d", sk, sk->sk_state);
-	cancel_delayed_work(&iso_pi(sk)->conn->timeout_work);
+	disable_delayed_work_sync(&iso_pi(sk)->timeout_work);
 }
 
 /* ---- ISO connections ---- */
@@ -227,7 +220,6 @@ static struct iso_conn *iso_conn_add(struct hci_conn *hcon)
 
 	kref_init(&conn->ref);
 	spin_lock_init(&conn->lock);
-	INIT_DELAYED_WORK(&conn->timeout_work, iso_sock_timeout);
 
 	hcon->iso_data = conn;
 	conn->hcon = hcon;
@@ -292,8 +284,9 @@ static void iso_conn_del(struct hci_conn *hcon, int err)
 		return;
 	}
 
+	iso_sock_disable_timer(sk);
+
 	lock_sock(sk);
-	iso_sock_clear_timer(sk);
 	iso_chan_del(sk, err);
 	release_sock(sk);
 	iso_sock_kill(sk);
@@ -800,6 +793,8 @@ static void iso_sock_cleanup_listen(struct sock *parent)
  */
 static void iso_sock_kill(struct sock *sk)
 {
+	iso_sock_disable_timer(sk);
+
 	lock_sock(sk);
 
 	if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket ||
@@ -895,8 +890,9 @@ static void __iso_sock_close(struct sock *sk)
 /* Must be called on unlocked socket. */
 static void iso_sock_close(struct sock *sk)
 {
+	iso_sock_disable_timer(sk);
+
 	lock_sock(sk);
-	iso_sock_clear_timer(sk);
 	__iso_sock_close(sk);
 	release_sock(sk);
 }
@@ -965,6 +961,8 @@ static struct sock *iso_sock_alloc(struct net *net, struct socket *sock,
 	iso_pi(sk)->qos = default_qos;
 	iso_pi(sk)->sync_handle = -1;
 
+	INIT_DELAYED_WORK(&iso_pi(sk)->timeout_work, iso_sock_timeout);
+
 	bt_sock_link(&iso_sk_list, sk);
 	return sk;
 }
-- 
2.55.0

[PATCH 09/11] Bluetooth: ISO: ensure no dangling hcon references in iso_conn

[Pauli Virtanen]

After iso_conn_del(), ISO sockets should not dereference the hcon any
more.  Currently, clearing iso_conn::hcon relies on iso_conn_del()
releasing the last reference to the iso_conn.

Simplify this by explicitly clearing conn->hcon in iso_conn_del(), to
avoid more complex reasoning on races about who holds the last
reference.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/iso.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 0cc08416fde5..bfd39baa8503 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -263,6 +263,7 @@ static void iso_chan_del(struct sock *sk, int err)
 }
 
 static void iso_conn_del(struct hci_conn *hcon, int err)
+	__must_hold(&hcon->hdev->lock)
 {
 	struct iso_conn *conn = hcon->iso_data;
 	struct sock *sk;
@@ -277,11 +278,10 @@ static void iso_conn_del(struct hci_conn *hcon, int err)
 	iso_conn_lock(conn);
 	sk = iso_sock_hold(conn);
 	iso_conn_unlock(conn);
-	iso_conn_put(conn);
 
 	if (!sk) {
 		iso_conn_put(conn);
-		return;
+		goto done;
 	}
 
 	iso_sock_disable_timer(sk);
@@ -291,6 +291,15 @@ static void iso_conn_del(struct hci_conn *hcon, int err)
 	release_sock(sk);
 	iso_sock_kill(sk);
 	sock_put(sk);
+
+done:
+	/* No sk access to conn->hcon any more (lock_sock + hdev->lock) */
+	iso_conn_lock(conn);
+	conn->hcon = NULL;
+	hcon->iso_data = NULL;
+	iso_conn_unlock(conn);
+
+	iso_conn_put(conn);
 }
 
 static int __iso_chan_add(struct iso_conn *conn, struct sock *sk,
@@ -306,6 +315,11 @@ static int __iso_chan_add(struct iso_conn *conn, struct sock *sk,
 		return -EBUSY;
 	}
 
+	if (!conn->hcon) {
+		BT_ERR("conn->hcon missing");
+		return -EIO;
+	}
+
 	iso_pi(sk)->conn = conn;
 	conn->sk = sk;
 	clear_bit(ISO_CONN_DROPPED, conn->flags);
@@ -2500,6 +2514,7 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags)
 }
 
 static void iso_connect_cfm(struct hci_conn *hcon, __u8 status)
+	__must_hold(&hcon->hdev->lock)
 {
 	if (hcon->type != CIS_LINK && hcon->type != BIS_LINK &&
 	    hcon->type != PA_LINK) {
@@ -2511,8 +2526,10 @@ static void iso_connect_cfm(struct hci_conn *hcon, __u8 status)
 			struct hci_link *link, *t;
 
 			list_for_each_entry_safe(link, t, &hcon->link_list,
-						 list)
+						 list) {
+				lockdep_assert_held(&link->conn->hdev->lock);
 				iso_conn_del(link->conn, bt_to_errno(status));
+			}
 
 			return;
 		}
@@ -2542,6 +2559,7 @@ static void iso_connect_cfm(struct hci_conn *hcon, __u8 status)
 }
 
 static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason)
+	__must_hold(&hcon->hdev->lock)
 {
 	if (hcon->type != CIS_LINK && hcon->type !=  BIS_LINK &&
 	    hcon->type != PA_LINK)
-- 
2.55.0

[PATCH 10/11] Bluetooth: ISO: fix refcounting of iso_conn

[Pauli Virtanen]

iso_conn_del() and iso_chan_del() have a race that results to double-put
of iso_conn:

    [Task hdev->workqueue]         [Task 2]
    iso_conn_del                   iso_chan_del
      iso_conn_hold_unless_zero      iso_conn_lock
      iso_conn_lock                  conn->sk = NULL
                                     iso_conn_unlock
      sk = iso_sock_hold(conn)  <---------´
      if (!sk) iso_conn_put          iso_conn_put
      iso_conn_put /* UAF */

The extra put for !sk in iso_conn_del() is currently required since
failing iso_chan_add() may leave iso_conn not associated with any sk.

Fix by having iso_pi(sk)->conn own refcount when non-NULL, so
iso_conn_del does not need to put it.  Adjust the iso_conn_add()
refcounting so that conn is put if it does not get associated with an
sk.

Fixes: dc26097bdb86 ("Bluetooth: ISO: Use kref to track lifetime of iso_conn")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/iso.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index bfd39baa8503..30de99ba4b30 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -108,9 +108,6 @@ static void iso_conn_free(struct kref *ref)
 
 	BT_DBG("conn %p", conn);
 
-	if (conn->sk)
-		iso_pi(conn->sk)->conn = NULL;
-
 	if (conn->hcon) {
 		conn->hcon->iso_data = NULL;
 		if (!test_and_set_bit(ISO_CONN_DROPPED, conn->flags))
@@ -145,6 +142,14 @@ static struct iso_conn *iso_conn_hold_unless_zero(struct iso_conn *conn)
 	return conn;
 }
 
+static struct iso_conn *iso_conn_hold(struct iso_conn *conn)
+{
+	BT_DBG("conn %p refcnt %u", conn, kref_read(&conn->ref));
+
+	kref_get(&conn->ref);
+	return conn;
+}
+
 static struct sock *iso_sock_hold(struct iso_conn *conn)
 {
 	if (!conn || !bt_sock_linked(&iso_sk_list, conn->sk))
@@ -210,7 +215,6 @@ static struct iso_conn *iso_conn_add(struct hci_conn *hcon)
 			conn->hcon = hcon;
 			iso_conn_unlock(conn);
 		}
-		iso_conn_put(conn);
 		return conn;
 	}
 
@@ -279,10 +283,8 @@ static void iso_conn_del(struct hci_conn *hcon, int err)
 	sk = iso_sock_hold(conn);
 	iso_conn_unlock(conn);
 
-	if (!sk) {
-		iso_conn_put(conn);
+	if (!sk)
 		goto done;
-	}
 
 	iso_sock_disable_timer(sk);
 
@@ -320,7 +322,7 @@ static int __iso_chan_add(struct iso_conn *conn, struct sock *sk,
 		return -EIO;
 	}
 
-	iso_pi(sk)->conn = conn;
+	iso_pi(sk)->conn = iso_conn_hold(conn);
 	conn->sk = sk;
 	clear_bit(ISO_CONN_DROPPED, conn->flags);
 
@@ -427,6 +429,7 @@ static int iso_connect_bis(struct sock *sk)
 	}
 
 	err = iso_chan_add(conn, sk, NULL);
+	iso_conn_put(conn);
 	if (err)
 		goto unlock;
 
@@ -529,6 +532,7 @@ static int iso_connect_cis(struct sock *sk)
 	}
 
 	err = iso_chan_add(conn, sk, NULL);
+	iso_conn_put(conn);
 	if (err)
 		goto unlock;
 
@@ -1310,10 +1314,9 @@ static int iso_listen_bis(struct sock *sk)
 	}
 
 	err = iso_chan_add(conn, sk, NULL);
-	if (err) {
-		hci_conn_drop(hcon);
+	iso_conn_put(conn);
+	if (err)
 		goto unlock;
-	}
 
 unlock:
 	release_sock(sk);
@@ -2551,8 +2554,10 @@ static void iso_connect_cfm(struct hci_conn *hcon, __u8 status)
 		struct iso_conn *conn;
 
 		conn = iso_conn_add(hcon);
-		if (conn)
+		if (conn) {
 			iso_conn_ready(conn);
+			iso_conn_put(conn);
+		}
 	} else {
 		iso_conn_del(hcon, bt_to_errno(status));
 	}
-- 
2.55.0

[PATCH 11/11] Bluetooth: ISO: fix race of kfree vs kref_get_unless_zero

[Pauli Virtanen]

hci_conn::iso_data is accessed and modified without lock or RCU.
This leads to a race

    [Task hdev->workqueue]                 	[Task 2]
    iso_recv                                    iso_conn_put(conn)
      conn = LOAD hcon->iso_data                  iso_conn_free(conn)
      iso_conn_hold_unless_zero(conn)               hcon->iso_data = NULL
                                                    kfree(conn)
        kref_get_unless_zero(&conn->ref) /* UAF */

and also to races in iso_conn_add() vs. iso_conn_free().

Fix by adding spinlock hci_conn::proto_lock and using it to guard
hci_conn::iso_data.

Fixes: dc26097bdb86 ("Bluetooth: ISO: Use kref to track lifetime of iso_conn")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/hci_core.h |  4 +-
 net/bluetooth/hci_conn.c         |  2 +
 net/bluetooth/iso.c              | 64 ++++++++++++++++++++++++++------
 3 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e7133ff87fbf..3df59849dcbe 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -767,9 +767,11 @@ struct hci_conn {
 	struct dentry	*debugfs;
 
 	struct hci_dev	*hdev;
+
+	spinlock_t	proto_lock; /* lock guarding protocol data */
 	void		*l2cap_data;
 	void		*sco_data;
-	void		*iso_data;
+	void		*iso_data __guarded_by(&proto_lock);
 
 	struct list_head link_list;
 	struct hci_conn	*parent;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 1966cd153d97..ebb04badf10c 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1123,6 +1123,8 @@ static struct hci_conn *__hci_conn_add(struct hci_dev *hdev, int type,
 	INIT_DELAYED_WORK(&conn->idle_work, hci_conn_idle);
 	INIT_DELAYED_WORK(&conn->le_conn_timeout, le_conn_timeout);
 
+	spin_lock_init(&conn->proto_lock);
+
 	atomic_set(&conn->refcnt, 0);
 
 	hci_dev_hold(hdev);
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 30de99ba4b30..a461c8a4efed 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -109,9 +109,16 @@ static void iso_conn_free(struct kref *ref)
 	BT_DBG("conn %p", conn);
 
 	if (conn->hcon) {
-		conn->hcon->iso_data = NULL;
-		if (!test_and_set_bit(ISO_CONN_DROPPED, conn->flags))
-			hci_conn_drop(conn->hcon);
+		spin_lock(&conn->hcon->proto_lock);
+
+		/* Check we are not racing with iso_conn_add */
+		if (conn->hcon->iso_data == conn) {
+			conn->hcon->iso_data = NULL;
+			if (!test_and_set_bit(ISO_CONN_DROPPED, conn->flags))
+				hci_conn_drop(conn->hcon);
+		}
+
+		spin_unlock(&conn->hcon->proto_lock);
 	}
 
 	kfree_skb(conn->rx_skb);
@@ -126,7 +133,21 @@ static void iso_conn_put(struct iso_conn *conn)
 
 	BT_DBG("conn %p refcnt %d", conn, kref_read(&conn->ref));
 
+	/* The following race vs. iso_conn_del() is possible:
+	 *
+	 * 1. conn->hcon != NULL here
+	 * 2. kref_put puts the last reference
+	 * 3. concurrent iso_conn_del() gets iso_conn_hold_unless_zero() -> NULL
+	 *    and returns immediately, so conn->hcon is not cleared
+	 * 4. iso_conn_free() dereferences conn->hcon
+	 *
+	 * To avoid UAF in step 4, take RCU before decrementing the refcount.
+	 */
+	rcu_read_lock();
+
 	kref_put(&conn->ref, iso_conn_free);
+
+	rcu_read_unlock();
 }
 
 static struct iso_conn *iso_conn_hold_unless_zero(struct iso_conn *conn)
@@ -205,22 +226,28 @@ static void iso_sock_disable_timer(struct sock *sk)
 
 /* ---- ISO connections ---- */
 static struct iso_conn *iso_conn_add(struct hci_conn *hcon)
+	__must_hold(&hcon->hdev->lock)
 {
-	struct iso_conn *conn = hcon->iso_data;
+	struct iso_conn *conn;
 
-	conn = iso_conn_hold_unless_zero(conn);
+	spin_lock(&hcon->proto_lock);
+
+	conn = iso_conn_hold_unless_zero(hcon->iso_data);
 	if (conn) {
 		if (!conn->hcon) {
 			iso_conn_lock(conn);
 			conn->hcon = hcon;
 			iso_conn_unlock(conn);
 		}
+		spin_unlock(&hcon->proto_lock);
 		return conn;
 	}
 
-	conn = kzalloc_obj(*conn);
-	if (!conn)
+	conn = kzalloc_obj(*conn, GFP_ATOMIC);
+	if (!conn) {
+		spin_unlock(&hcon->proto_lock);
 		return NULL;
+	}
 
 	kref_init(&conn->ref);
 	spin_lock_init(&conn->lock);
@@ -229,6 +256,8 @@ static struct iso_conn *iso_conn_add(struct hci_conn *hcon)
 	conn->hcon = hcon;
 	conn->tx_sn = 0;
 
+	spin_unlock(&hcon->proto_lock);
+
 	BT_DBG("hcon %p conn %p", hcon, conn);
 
 	return conn;
@@ -269,10 +298,12 @@ static void iso_chan_del(struct sock *sk, int err)
 static void iso_conn_del(struct hci_conn *hcon, int err)
 	__must_hold(&hcon->hdev->lock)
 {
-	struct iso_conn *conn = hcon->iso_data;
+	struct iso_conn *conn;
 	struct sock *sk;
 
-	conn = iso_conn_hold_unless_zero(conn);
+	spin_lock(&hcon->proto_lock);
+	conn = iso_conn_hold_unless_zero(hcon->iso_data);
+	spin_unlock(&hcon->proto_lock);
 	if (!conn)
 		return;
 
@@ -296,10 +327,12 @@ static void iso_conn_del(struct hci_conn *hcon, int err)
 
 done:
 	/* No sk access to conn->hcon any more (lock_sock + hdev->lock) */
+	spin_lock(&hcon->proto_lock);
 	iso_conn_lock(conn);
 	conn->hcon = NULL;
 	hcon->iso_data = NULL;
 	iso_conn_unlock(conn);
+	spin_unlock(&hcon->proto_lock);
 
 	iso_conn_put(conn);
 }
@@ -421,6 +454,8 @@ static int iso_connect_bis(struct sock *sk)
 			iso_pi(sk)->bc_sid = hcon->sid;
 	}
 
+	lockdep_assert_held(&hcon->hdev->lock);
+
 	conn = iso_conn_add(hcon);
 	if (!conn) {
 		hci_conn_drop(hcon);
@@ -524,6 +559,8 @@ static int iso_connect_cis(struct sock *sk)
 		}
 	}
 
+	lockdep_assert_held(&hcon->hdev->lock);
+
 	conn = iso_conn_add(hcon);
 	if (!conn) {
 		hci_conn_drop(hcon);
@@ -855,8 +892,8 @@ static void iso_sock_disconn(struct sock *sk)
 		 */
 		if (bis_sk) {
 			hcon->state = BT_OPEN;
-			hcon->iso_data = NULL;
-			iso_pi(sk)->conn->hcon = NULL;
+			set_bit(ISO_CONN_DROPPED, iso_pi(sk)->conn->flags);
+
 			iso_sock_clear_timer(sk);
 			iso_chan_del(sk, bt_to_errno(hcon->abort_reason));
 			sock_put(bis_sk);
@@ -1306,6 +1343,8 @@ static int iso_listen_bis(struct sock *sk)
 		goto unlock;
 	}
 
+	lockdep_assert_held(&hcon->hdev->lock);
+
 	conn = iso_conn_add(hcon);
 	if (!conn) {
 		hci_conn_drop(hcon);
@@ -2591,7 +2630,10 @@ int iso_recv(struct hci_dev *hdev, u16 handle, struct sk_buff *skb, u16 flags)
 		return -ENOENT;
 	}
 
+	spin_lock(&hcon->proto_lock);
 	conn = iso_conn_hold_unless_zero(hcon->iso_data);
+	spin_unlock(&hcon->proto_lock);
+
 	hcon = NULL;
 
 	hci_dev_unlock(hdev);
-- 
2.55.0

Re: [PATCH 00/11] Bluetooth: ISO: fix HUP on socket release/shutdown + UAF/locking fixes

[Pauli Virtanen]

pe, 2026-07-24 kello 23:20 +0300, Pauli Virtanen kirjoitti:
> The retracted patch in commit e824c0bbe0ec9 ("Bluetooth: ISO: clear
> iso_data always when detaching conn from hcon") merged in
> bluetooth-next/master broke ISO socket transition to BT_CLOSED so they
> don't send POLLHUP properly any more, as seen in "ISO Disconnect -
> Success" test.

On Sashiko comments:
https://sashiko.dev/#/patchset/cover.1784923689.git.pav%40iki.fi

Patch 3: pre-existing issue, not UAF/locking issue so maybe should be
in separate series.

Patch 9: the comment appears to be the UAF which is fixed by the next
Patch 10 as described in its commit message. This was also pre-existing
(SCO has the same race).

Patch 10: these comments seem confused (no rcu_read_lock in
iso_conn_put in this patch, doesn't seem to notice synchronize_rcu in
hci_conn_hash_del?), but remaining UAF/locking issues should be fixed
by Patch 11.

Patch 11: the comment is about hci_conn_hold/drop refcount when
iso_conn_free and iso_conn_add happen to execute exactly at the same
time, with the same pre-existing hci_conn.

hci_connect_cis/bis don't give a new refcount when the hci_conn is pre-
existing and usable, which should mitigate this, the refcount stays at
1 and becomes owned by the new iso_conn. IIUC it should be OK as it is.


> This is rebased version of and supercedes
> https://lore.kernel.org/linux-bluetooth/fbd9dd573bb1ce5f38370128dd35447e2ddfed9c.1784625576.git.pav@iki.fi/
> 
> The first commit in this series fixes that, and subsequent mostly
> independent commits other minor issues found looking at the ISO socket
> code.
> 
> The last three fix UAF issues. The locking/lifetime of iso_conn
> complicates the code here and the last three patches can be replaced by
> somewhat simpler solution with bigger diff that removes iso_conn
> https://github.com/pv/linux/commit/e4b460b5f7aaba060b958229aaec6b2217642178
> 
> Fixed issues aside BT_CONNECT, these appear to be pre-existing in
> several previous kernel releases:
> 
> - sk is always leaked on socket release
> 
> - timeout_work may deadlock under certain conditions
> 
> - correctness of iso_conn_del() vs UAF requires somewhat too complex
>   reasoning about race conditions, and it's not quite correct
> 
>         [Task 1]                      [Task hdev->workqueue]
>         iso_sock_timeout              iso_conn_del
>           iso_conn_hold_unless_zero     iso_chan_del
>                            `------------> iso_conn_put
>                                       caller frees hcon
>           iso_conn_put
>             iso_conn_free
>               conn->hcon->iso_data = NULL; /* UAF */
> 
> - attempt to free iso_conn in iso_conn_del() races with iso_conn_del
>   with potential UAF
> 
> - iso_sock_ready() gets conn->sk without requisite locks, this should
>   be done like in sco.c
> 
> - iso_sock_getname, iso_connect_ind are missing lock_sock and NULL
>   checks
> 
> - kref_get_unless_zero(&((struct iso_conn *)hcon->iso_data)->ref)
>   requires synchronization primitive.
> 
> Tested vs iso-tester, Pipewire Qemu ucast/bcast audio tests, and real
> ucast audio streaming, which pass without KASAN/locking splats. Also
> checked iso-tester produces balanced iso_sock_init / iso_sock_destruct
> kprints.
> 
> Pauli Virtanen (11):
>   Bluetooth: ISO: fix CONNECTED -> CLOSED transition on shutdown/release
>   Bluetooth: ISO: lock sk in iso_sock_getname
>   Bluetooth: ISO: lock sk in iso_connect_ind
>   Bluetooth: ISO: fix timeout vs sync_timeout typo in check_bcast_qos
>   Bluetooth: ISO: validate sockaddr_iso first in iso_sock_rebind_bis()
>   Bluetooth: ISO: hold sk properly in iso_conn_ready
>   Bluetooth: ISO: fix leaking sk after socket release
>   Bluetooth: ISO: avoid deadlocks in iso_sock_timeout
>   Bluetooth: ISO: ensure no dangling hcon references in iso_conn
>   Bluetooth: ISO: fix refcounting of iso_conn
>   Bluetooth: ISO: fix race of kfree vs kref_get_unless_zero
> 
>  include/net/bluetooth/hci_core.h |   4 +-
>  net/bluetooth/hci_conn.c         |   2 +
>  net/bluetooth/iso.c              | 274 ++++++++++++++++++++++---------
>  3 files changed, 197 insertions(+), 83 deletions(-)

-- 
Pauli Virtanen

Re: [PATCH 00/11] Bluetooth: ISO: fix HUP on socket release/shutdown + UAF/locking fixes

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Fri, 24 Jul 2026 23:20:23 +0300 you wrote:
> The retracted patch in commit e824c0bbe0ec9 ("Bluetooth: ISO: clear
> iso_data always when detaching conn from hcon") merged in
> bluetooth-next/master broke ISO socket transition to BT_CLOSED so they
> don't send POLLHUP properly any more, as seen in "ISO Disconnect -
> Success" test.
> 
> This is rebased version of and supercedes
> https://lore.kernel.org/linux-bluetooth/fbd9dd573bb1ce5f38370128dd35447e2ddfed9c.1784625576.git.pav@iki.fi/
> 
> [...]

Here is the summary with links:
  - [01/11] Bluetooth: ISO: fix CONNECTED -> CLOSED transition on shutdown/release
    https://git.kernel.org/bluetooth/bluetooth-next/c/278aeca60782
  - [02/11] Bluetooth: ISO: lock sk in iso_sock_getname
    https://git.kernel.org/bluetooth/bluetooth-next/c/6d16b799d7f4
  - [03/11] Bluetooth: ISO: lock sk in iso_connect_ind
    https://git.kernel.org/bluetooth/bluetooth-next/c/df810355bbe7
  - [04/11] Bluetooth: ISO: fix timeout vs sync_timeout typo in check_bcast_qos
    https://git.kernel.org/bluetooth/bluetooth-next/c/c08593655456
  - [05/11] Bluetooth: ISO: validate sockaddr_iso first in iso_sock_rebind_bis()
    https://git.kernel.org/bluetooth/bluetooth-next/c/126e8f096c10
  - [06/11] Bluetooth: ISO: hold sk properly in iso_conn_ready
    https://git.kernel.org/bluetooth/bluetooth-next/c/deb1d93a07f0
  - [07/11] Bluetooth: ISO: fix leaking sk after socket release
    https://git.kernel.org/bluetooth/bluetooth-next/c/2a93966653a9
  - [08/11] Bluetooth: ISO: avoid deadlocks in iso_sock_timeout
    https://git.kernel.org/bluetooth/bluetooth-next/c/f8cb802462f5
  - [09/11] Bluetooth: ISO: ensure no dangling hcon references in iso_conn
    https://git.kernel.org/bluetooth/bluetooth-next/c/f80c3b2232a2
  - [10/11] Bluetooth: ISO: fix refcounting of iso_conn
    https://git.kernel.org/bluetooth/bluetooth-next/c/e4d507fb7f6e
  - [11/11] Bluetooth: ISO: fix race of kfree vs kref_get_unless_zero
    https://git.kernel.org/bluetooth/bluetooth-next/c/4b56754e9da3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html