[PATCH v2] Bluetooth: SCO: serialise sco_conn lifetime against sco_recv_scodata()

[PATCH v2] Bluetooth: SCO: serialise sco_conn lifetime against sco_recv_scodata()

[Aldo Ariel Panzardo]

sco_recv_scodata() upgrades the weak hcon->sco_data back-pointer to a
strong reference under hdev->lock:

    hci_dev_lock(hdev);
    hcon = hci_conn_hash_lookup_handle(hdev, handle);
    ...
    conn = sco_conn_hold_unless_zero(hcon->sco_data);
    hci_dev_unlock(hdev);

but the pointer is cleared from the other side without that lock. When
the last sco_conn reference is dropped, sco_conn_free() ran

    conn->hcon->sco_data = NULL;

with no hdev->lock held, so the RX path could read hcon->sco_data and
call kref_get_unless_zero() on an sco_conn that was concurrently freed:

    BUG: KASAN: slab-use-after-free in sco_conn_hold_unless_zero+0xbe/0x160
    Write of size 4 by task kworker/u17:0
    Workqueue: hci0 hci_rx_work
    Call Trace:
     sco_conn_hold_unless_zero+0xbe/0x160
     sco_recv_scodata+0x13f/0x490
     hci_rx_work+0x3af/0x730

kref_get_unless_zero() only guards against a zero refcount, not against
the backing memory already being freed.

Give hcon->sco_data an actual reference on the sco_conn it points to, so
the object cannot be freed while the pointer is still installed, and only
clear and drop it from sco_conn_del(), which runs under hdev->lock (its
callers, sco_connect_cfm() and sco_disconn_cfm(), hold it). The reader in
sco_recv_scodata() also takes hdev->lock, so the store and the read are
now serialised: the RX path either observes NULL or a reference that is
guaranteed to stay valid until it drops its own.

sco_conn_add() no longer hands its kref_init() reference to the caller as
the connection's only reference; that initial reference is the one owned
by hcon->sco_data, and callers get their own via sco_conn_hold().

Because the sco_conn can now outlive the socket (the association keeps it
alive until the link goes down), the hci_conn can no longer be dropped
from sco_conn_free() without regressing socket close: closing a connected
SCO socket must still tear the link down. Move hci_conn ownership to the
socket instead: __sco_chan_add() takes an hci_conn reference and it is
released from sco_chan_del() and sco_sock_destruct(), exactly once. The
sco_conn no longer owns an hci_conn reference, so sco_conn_add() stops
consuming one and sco_connect() drops the hci_connect_sco() reference on
every path; sco_connect_cfm() no longer needs its hci_conn_hold() dance.

Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn")
Cc: stable@vger.kernel.org
Suggested-by: Pauli Virtanen <pav@iki.fi>
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
v2: quote the inlined code and KASAN splat in the commit message with
    spaces instead of hard tabs, to satisfy the bluez CI gitlint check;
    no functional change (CI sco-tester and sparse pass).

 net/bluetooth/sco.c | 84 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 69 insertions(+), 15 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 3d4362a09df4..8df0fa48fc60 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -84,12 +84,14 @@ static void sco_conn_free(struct kref *ref)
 	if (conn->sk)
 		sco_pi(conn->sk)->conn = NULL;
 
-	if (conn->hcon) {
-		conn->hcon->sco_data = NULL;
-		hci_conn_drop(conn->hcon);
-	}
+	/* hcon->sco_data is cleared and the association's reference on the
+	 * sco_conn is dropped in sco_conn_del() under hdev->lock, and the
+	 * hci_conn is now owned by the socket (held in __sco_chan_add() and
+	 * dropped in sco_chan_del()/sco_sock_destruct()), so there is nothing
+	 * left to release towards hcon here.
+	 */
 
-	/* Ensure no more work items will run since hci_conn has been dropped */
+	/* Ensure no more work items will run before the connection is freed */
 	disable_delayed_work_sync(&conn->timeout_work);
 
 	kfree(conn);
@@ -188,8 +190,11 @@ static void sco_sock_clear_timer(struct sock *sk)
 }
 
 /* ---- SCO connections ---- */
-/* Consumes a reference on @hcon, which the returned sco_conn owns until it is
- * freed. On failure (NULL return) the reference is left for the caller to drop.
+/* Returns a new reference the caller must drop with sco_conn_put(). The
+ * hcon->sco_data association holds its own reference on the sco_conn for the
+ * connection's lifetime; it is dropped in sco_conn_del() under hdev->lock.
+ * @hcon is not consumed: the hci_conn reference is taken and owned by the
+ * socket in __sco_chan_add().
  */
 static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
 {
@@ -201,9 +206,6 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
 			sco_conn_lock(conn);
 			conn->hcon = hcon;
 			sco_conn_unlock(conn);
-		} else {
-			/* conn already owns a reference on hcon */
-			hci_conn_drop(hcon);
 		}
 		return conn;
 	}
@@ -227,7 +229,10 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
 
 	BT_DBG("hcon %p conn %p", hcon, conn);
 
-	return conn;
+	/* kref_init() above set the association reference owned by
+	 * hcon->sco_data; hand the caller its own reference.
+	 */
+	return sco_conn_hold(conn);
 }
 
 /* Delete channel.
@@ -242,9 +247,20 @@ static void sco_chan_del(struct sock *sk, int err)
 	BT_DBG("sk %p, conn %p, err %d", sk, conn, err);
 
 	if (conn) {
+		struct hci_conn *hcon;
+
 		sco_conn_lock(conn);
 		conn->sk = NULL;
+		hcon = conn->hcon;
 		sco_conn_unlock(conn);
+
+		/* Release the socket's own reference on the hci_conn taken in
+		 * __sco_chan_add() so that closing the socket still tears the
+		 * link down even while the sco_conn is kept alive by the
+		 * hcon->sco_data association reference.
+		 */
+		if (hcon)
+			hci_conn_drop(hcon);
 		sco_conn_put(conn);
 	}
 
@@ -266,6 +282,15 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
 
 	BT_DBG("hcon %p conn %p, err %d", hcon, conn, err);
 
+	/* Detach the connection from the hci_conn and drop the reference held
+	 * by the hcon->sco_data association.  The caller holds hdev->lock,
+	 * which serialises this NULL store and put against the read of
+	 * hcon->sco_data in sco_recv_scodata(), closing the use-after-free
+	 * where the RX path could upgrade an already-freed sco_conn.
+	 */
+	hcon->sco_data = NULL;
+	sco_conn_put(conn);
+
 	sco_conn_lock(conn);
 	sk = sco_sock_hold(conn);
 	sco_conn_unlock(conn);
@@ -290,6 +315,13 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
 	sco_pi(sk)->conn = sco_conn_hold(conn);
 	conn->sk = sk;
 
+	/* The socket owns a reference on the hci_conn for as long as it stays
+	 * attached; it is dropped in sco_chan_del()/sco_sock_destruct().  This
+	 * keeps close() tearing the link down now that the sco_conn can outlive
+	 * the socket through the hcon->sco_data association reference.
+	 */
+	hci_conn_hold(conn->hcon);
+
 	if (parent)
 		bt_accept_enqueue(parent, sk, true);
 }
@@ -371,6 +403,7 @@ static int sco_connect(struct sock *sk)
 	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
 		release_sock(sk);
 		sco_conn_put(conn);
+		hci_conn_drop(hcon);
 		err = -EBADFD;
 		goto unlock;
 	}
@@ -379,6 +412,7 @@ static int sco_connect(struct sock *sk)
 	sco_conn_put(conn);
 	if (err) {
 		release_sock(sk);
+		hci_conn_drop(hcon);
 		goto unlock;
 	}
 
@@ -395,6 +429,11 @@ static int sco_connect(struct sock *sk)
 
 	release_sock(sk);
 
+	/* The socket took its own hci_conn reference in __sco_chan_add(); drop
+	 * the one returned by hci_connect_sco().
+	 */
+	hci_conn_drop(hcon);
+
 unlock:
 	hci_dev_unlock(hdev);
 	hci_dev_put(hdev);
@@ -495,9 +534,26 @@ static struct sock *sco_get_sock_listen(bdaddr_t *src)
 
 static void sco_sock_destruct(struct sock *sk)
 {
+	struct sco_conn *conn = sco_pi(sk)->conn;
+
 	BT_DBG("sk %p", sk);
 
-	sco_conn_put(sco_pi(sk)->conn);
+	/* If the channel was not already torn down via sco_chan_del(), drop the
+	 * socket's own references here.  sco_chan_del() clears sco_pi(sk)->conn,
+	 * so the hci_conn and sco_conn references are released exactly once.
+	 */
+	if (conn) {
+		struct hci_conn *hcon;
+
+		sco_conn_lock(conn);
+		hcon = conn->hcon;
+		sco_conn_unlock(conn);
+
+		if (hcon)
+			hci_conn_drop(hcon);
+		sco_pi(sk)->conn = NULL;
+		sco_conn_put(conn);
+	}
 
 	skb_queue_purge(&sk->sk_receive_queue);
 	skb_queue_purge(&sk->sk_write_queue);
@@ -1511,12 +1567,10 @@ static void sco_connect_cfm(struct hci_conn *hcon, __u8 status)
 	if (!status) {
 		struct sco_conn *conn;
 
-		conn = sco_conn_add(hci_conn_hold(hcon));
+		conn = sco_conn_add(hcon);
 		if (conn) {
 			sco_conn_ready(conn);
 			sco_conn_put(conn);
-		} else {
-			hci_conn_drop(hcon);
 		}
 	} else
 		sco_conn_del(hcon, bt_to_errno(status));
-- 
2.43.0

RE: [v2] Bluetooth: SCO: serialise sco_conn lifetime against sco_recv_scodata()

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1135608

---Test result---

Test Summary:
CheckPatch                    PASS      0.81 seconds
VerifyFixes                   PASS      0.14 seconds
VerifySignedoff               PASS      0.13 seconds
GitLint                       PASS      0.34 seconds
SubjectPrefix                 PASS      0.13 seconds
BuildKernel                   PASS      28.35 seconds
CheckAllWarning               PASS      31.52 seconds
CheckSparse                   PASS      30.02 seconds
BuildKernel32                 PASS      27.63 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      516.14 seconds
TestRunner_sco-tester         PASS      33.81 seconds
IncrementalBuild              PASS      26.03 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found


https://github.com/bluez/bluetooth-next/pull/503

---
Regards,
Linux Bluetooth

Re: [PATCH v2] Bluetooth: SCO: serialise sco_conn lifetime against sco_recv_scodata()

[Pauli Virtanen]

Hi,

ti, 2026-07-28 kello 01:26 -0300, Aldo Ariel Panzardo kirjoitti:
> sco_recv_scodata() upgrades the weak hcon->sco_data back-pointer to a
> strong reference under hdev->lock:

iso.c in current bluetooth-next/master kept the weak-reference iso_data
and addressed races in a different way (I had alternative unsent patch
also for the strong ref approach).

Either one should work, strong ref may be a bit simpler.

>     hci_dev_lock(hdev);
>     hcon = hci_conn_hash_lookup_handle(hdev, handle);
>     ...
>     conn = sco_conn_hold_unless_zero(hcon->sco_data);
>     hci_dev_unlock(hdev);
> 
> but the pointer is cleared from the other side without that lock. When
> the last sco_conn reference is dropped, sco_conn_free() ran
> 
>     conn->hcon->sco_data = NULL;
> 
> with no hdev->lock held, so the RX path could read hcon->sco_data and
> call kref_get_unless_zero() on an sco_conn that was concurrently freed:
> 
>     BUG: KASAN: slab-use-after-free in sco_conn_hold_unless_zero+0xbe/0x160
>     Write of size 4 by task kworker/u17:0
>     Workqueue: hci0 hci_rx_work
>     Call Trace:
>      sco_conn_hold_unless_zero+0xbe/0x160
>      sco_recv_scodata+0x13f/0x490
>      hci_rx_work+0x3af/0x730
> 
> kref_get_unless_zero() only guards against a zero refcount, not against
> the backing memory already being freed.
> 
> Give hcon->sco_data an actual reference on the sco_conn it points to, so
> the object cannot be freed while the pointer is still installed, and only
> clear and drop it from sco_conn_del(), which runs under hdev->lock (its
> callers, sco_connect_cfm() and sco_disconn_cfm(), hold it). The reader in
> sco_recv_scodata() also takes hdev->lock, so the store and the read are
> now serialised: the RX path either observes NULL or a reference that is
> guaranteed to stay valid until it drops its own.
> 
> sco_conn_add() no longer hands its kref_init() reference to the caller as
> the connection's only reference; that initial reference is the one owned
> by hcon->sco_data, and callers get their own via sco_conn_hold().
> 
> Because the sco_conn can now outlive the socket (the association keeps it
> alive until the link goes down), the hci_conn can no longer be dropped
> from sco_conn_free() without regressing socket close: closing a connected
> SCO socket must still tear the link down. Move hci_conn ownership to the
> socket instead: __sco_chan_add() takes an hci_conn reference and it is
> released from sco_chan_del() and sco_sock_destruct(), exactly once. The
> sco_conn no longer owns an hci_conn reference, so sco_conn_add() stops
> consuming one and sco_connect() drops the hci_connect_sco() reference on
> every path; sco_connect_cfm() no longer needs its hci_conn_hold() dance.
> 
> Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn")
> Cc: stable@vger.kernel.org
> Suggested-by: Pauli Virtanen <pav@iki.fi>
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
> v2: quote the inlined code and KASAN splat in the commit message with
>     spaces instead of hard tabs, to satisfy the bluez CI gitlint check;
>     no functional change (CI sco-tester and sparse pass).
> 
>  net/bluetooth/sco.c | 84 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 69 insertions(+), 15 deletions(-)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 3d4362a09df4..8df0fa48fc60 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -84,12 +84,14 @@ static void sco_conn_free(struct kref *ref)
>  	if (conn->sk)
>  		sco_pi(conn->sk)->conn = NULL;
>  
> -	if (conn->hcon) {
> -		conn->hcon->sco_data = NULL;
> -		hci_conn_drop(conn->hcon);
> -	}
> +	/* hcon->sco_data is cleared and the association's reference on the
> +	 * sco_conn is dropped in sco_conn_del() under hdev->lock, and the
> +	 * hci_conn is now owned by the socket (held in __sco_chan_add() and
> +	 * dropped in sco_chan_del()/sco_sock_destruct()), so there is nothing
> +	 * left to release towards hcon here.
> +	 */
>  
> -	/* Ensure no more work items will run since hci_conn has been dropped */
> +	/* Ensure no more work items will run before the connection is freed */
>  	disable_delayed_work_sync(&conn->timeout_work);
>  
>  	kfree(conn);
> @@ -188,8 +190,11 @@ static void sco_sock_clear_timer(struct sock *sk)
>  }
>  
>  /* ---- SCO connections ---- */
> -/* Consumes a reference on @hcon, which the returned sco_conn owns until it is
> - * freed. On failure (NULL return) the reference is left for the caller to drop.
> +/* Returns a new reference the caller must drop with sco_conn_put(). The
> + * hcon->sco_data association holds its own reference on the sco_conn for the
> + * connection's lifetime; it is dropped in sco_conn_del() under hdev->lock.
> + * @hcon is not consumed: the hci_conn reference is taken and owned by the
> + * socket in __sco_chan_add().
>   */
>  static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
>  {
> @@ -201,9 +206,6 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
>  			sco_conn_lock(conn);
>  			conn->hcon = hcon;
>  			sco_conn_unlock(conn);

The above is probably dead code now, is conn->hcon == NULL && hcon-
>sco_data != NULL possible?

> -		} else {
> -			/* conn already owns a reference on hcon */
> -			hci_conn_drop(hcon);
>  		}
>  		return conn;
>  	}
> @@ -227,7 +229,10 @@ static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
>  
>  	BT_DBG("hcon %p conn %p", hcon, conn);
>  
> -	return conn;
> +	/* kref_init() above set the association reference owned by
> +	 * hcon->sco_data; hand the caller its own reference.
> +	 */
> +	return sco_conn_hold(conn);
>  }
>  
>  /* Delete channel.
> @@ -242,9 +247,20 @@ static void sco_chan_del(struct sock *sk, int err)
>  	BT_DBG("sk %p, conn %p, err %d", sk, conn, err);
>  
>  	if (conn) {
> +		struct hci_conn *hcon;
> +
>  		sco_conn_lock(conn);
>  		conn->sk = NULL;
> +		hcon = conn->hcon;
>  		sco_conn_unlock(conn);
> +
> +		/* Release the socket's own reference on the hci_conn taken in
> +		 * __sco_chan_add() so that closing the socket still tears the
> +		 * link down even while the sco_conn is kept alive by the
> +		 * hcon->sco_data association reference.
> +		 */
> +		if (hcon)
> +			hci_conn_drop(hcon);

UAF race here, similarly in sco_sock_destruct()

    [Task 1]                          [Task hdev->workqueue]
    sco_chan_del()                    sco_conn_del()
       sco_conn_lock                    conn = sco_conn_hold_unless_zero(conn);
       conn->sk = NULL                  hcon->sco_data = NULL;
       sco_conn_unlock -------------->  sco_conn_lock
                                        sk = sco_sock_hold(conn); /* -> NULL */
                                        sco_conn_unlock
					if (!sk) return;
                                      /* caller frees hci_conn */
       hci_conn_drop(hcon) /* UAF */

Moving the drop before conn->sk = NULL probably fixes this.

Probably a good idea to have GPT or other recent AI model review your
patch (give sco.c and the patch as context) with prompt that explains
the hci_conn lifetime (iso_conn_del called before free, hci_conn_hold
does not prevent free).

Theres AI review at sashiko.dev which often useful, but it has trouble
understanding hci_conn lifetime, and it didn't get this one.

>  		sco_conn_put(conn);
>  	}
>  
> @@ -266,6 +282,15 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
>  
>  	BT_DBG("hcon %p conn %p, err %d", hcon, conn, err);
>  
> +	/* Detach the connection from the hci_conn and drop the reference held
> +	 * by the hcon->sco_data association.  The caller holds hdev->lock,
> +	 * which serialises this NULL store and put against the read of
> +	 * hcon->sco_data in sco_recv_scodata(), closing the use-after-free
> +	 * where the RX path could upgrade an already-freed sco_conn.
> +	 */
> +	hcon->sco_data = NULL;
> +	sco_conn_put(conn);

In this design you can add `void *sco_data __guarded_by(&hdev->lock)`
context analysis annotation, so there's no need for the temporary hold
of sco_data in sco_conn_del() as access is serialized by hdev lock.

> +
>  	sco_conn_lock(conn);
>  	sk = sco_sock_hold(conn);
>  	sco_conn_unlock(conn);
> @@ -290,6 +315,13 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
>  	sco_pi(sk)->conn = sco_conn_hold(conn);
>  	conn->sk = sk;
>  
> +	/* The socket owns a reference on the hci_conn for as long as it stays
> +	 * attached; it is dropped in sco_chan_del()/sco_sock_destruct().  This
> +	 * keeps close() tearing the link down now that the sco_conn can outlive
> +	 * the socket through the hcon->sco_data association reference.
> +	 */
> +	hci_conn_hold(conn->hcon);
> +
>  	if (parent)
>  		bt_accept_enqueue(parent, sk, true);
>  }
> @@ -371,6 +403,7 @@ static int sco_connect(struct sock *sk)
>  	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
>  		release_sock(sk);
>  		sco_conn_put(conn);
> +		hci_conn_drop(hcon);
>  		err = -EBADFD;
>  		goto unlock;
>  	}
> @@ -379,6 +412,7 @@ static int sco_connect(struct sock *sk)
>  	sco_conn_put(conn);
>  	if (err) {
>  		release_sock(sk);
> +		hci_conn_drop(hcon);
>  		goto unlock;
>  	}
>  
> @@ -395,6 +429,11 @@ static int sco_connect(struct sock *sk)
>  
>  	release_sock(sk);
>  
> +	/* The socket took its own hci_conn reference in __sco_chan_add(); drop
> +	 * the one returned by hci_connect_sco().
> +	 */
> +	hci_conn_drop(hcon);

You can move this after __sco_chan_add, probably doesn't need comment.

> +
>  unlock:
>  	hci_dev_unlock(hdev);
>  	hci_dev_put(hdev);
> @@ -495,9 +534,26 @@ static struct sock *sco_get_sock_listen(bdaddr_t *src)
>  
>  static void sco_sock_destruct(struct sock *sk)
>  {
> +	struct sco_conn *conn = sco_pi(sk)->conn;
> +
>  	BT_DBG("sk %p", sk);
>  
> -	sco_conn_put(sco_pi(sk)->conn);
> +	/* If the channel was not already torn down via sco_chan_del(), drop the
> +	 * socket's own references here.  sco_chan_del() clears sco_pi(sk)->conn,
> +	 * so the hci_conn and sco_conn references are released exactly once.
> +	 */
> +	if (conn) {
> +		struct hci_conn *hcon;
> +
> +		sco_conn_lock(conn);
> +		hcon = conn->hcon;
> +		sco_conn_unlock(conn);
> +
> +		if (hcon)
> +			hci_conn_drop(hcon);

UAF since conn->hcon is not valid after conn->sk = NULL in
sco_sock_kill.

> +		sco_pi(sk)->conn = NULL;
> +		sco_conn_put(conn);
> +	}
>  
>  	skb_queue_purge(&sk->sk_receive_queue);
>  	skb_queue_purge(&sk->sk_write_queue);
> @@ -1511,12 +1567,10 @@ static void sco_connect_cfm(struct hci_conn *hcon, __u8 status)
>  	if (!status) {
>  		struct sco_conn *conn;
>  
> -		conn = sco_conn_add(hci_conn_hold(hcon));
> +		conn = sco_conn_add(hcon);
>  		if (conn) {
>  			sco_conn_ready(conn);
>  			sco_conn_put(conn);
> -		} else {
> -			hci_conn_drop(hcon);
>  		}
>  	} else
>  		sco_conn_del(hcon, bt_to_errno(status));

-- 
Pauli Virtanen