[PATCH v2] Bluetooth: SCO: give the socket its own sco_conn reference

[PATCH v2] Bluetooth: SCO: give the socket its own sco_conn reference

[Aldo Ariel Panzardo]

sco_conn_del() drops a reference it does not own. It takes one transient
reference via sco_conn_hold_unless_zero() and releases it with the
sco_conn_put() that follows sco_sock_hold(); the additional put in the
!sk branch releases a second one:

    conn = sco_conn_hold_unless_zero(conn);
    ...
    sk = sco_sock_hold(conn);
    sco_conn_unlock(conn);
    sco_conn_put(conn);

    if (!sk) {
            sco_conn_put(conn);
            return;
    }

When close() races the controller's Disconnection Complete, sco_chan_del()
clears conn->sk and drops the socket's reference while sco_conn_del() is
running. sco_conn_del() then sees sk == NULL, its own put drops the count
to zero and frees the conn, and the second put writes to the freed kref:

    BUG: KASAN: slab-use-after-free in sco_conn_put.part.0+0x1a/0x190
    Write of size 4 at addr ffff8881099dec74 by task kworker/u17:3/413
    Workqueue: hci1 hci_rx_work
    Call Trace:
     sco_conn_put.part.0+0x1a/0x190
     hci_disconn_complete_evt+0x1ee/0x3e0
     hci_event_packet+0x54a/0x650
     hci_rx_work+0x321/0x3d0
    Allocated by task 413:
     sco_conn_add+0x72/0x1a0
     sco_connect_cfm+0x88/0x670
    Freed by task 413:
     sco_conn_del.isra.0+0x3f/0xf0
     hci_disconn_complete_evt+0x1ee/0x3e0
    refcount_t: underflow; use-after-free.

Simply deleting the extra put is not enough, because the reference it
releases is not always accounted for elsewhere. __sco_chan_add() stores
the connection in the socket without taking a reference:

    sco_pi(sk)->conn = conn;

so the socket inherits whatever reference its caller happened to hold.
That works out for sco_conn_ready(), which takes an explicit
sco_conn_hold() beforehand and whose caller puts its own reference, and
for the success path of sco_connect(), where the reference returned by
sco_conn_add() is silently handed over and later released by
sco_sock_destruct(). It does not work out for the two error paths of
sco_connect(): if the socket state changed while the lock was dropped, or
if sco_chan_add() returns -EBUSY, the reference from sco_conn_add() is
never released and the connection is leaked. The extra put in
sco_conn_del() is what eventually reclaims those orphans, which is why
removing it in isolation trades a use-after-free for a leak.

Make the ownership explicit instead. __sco_chan_add() now takes the
socket's reference itself, sco_connect() releases the one it got from
sco_conn_add() on every path, and the now redundant hold in
sco_conn_ready() is dropped. With the socket holding a counted reference,
a connection can no longer reach zero while conn->sk is set, so
sco_conn_free() no longer has to clear sco_pi(conn->sk)->conn. Every
reference then has exactly one owner: the one sco_conn_add() returns
belongs to its caller, the socket's is taken and released with the
channel, and sco_conn_del() and sco_sock_timeout() only ever hold
transient ones.

Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
v2:
 - Do not just delete the extra put: make the socket own its reference,
   balance sco_connect()'s error paths and drop the redundant hold in
   sco_conn_ready(), per Pauli Virtanen's review.
 - Drop the now unreachable sco_pi(conn->sk)->conn clearing in
   sco_conn_free().
 - Indent the quoted code with spaces so gitlint stops complaining.

On hci_conn_drop() vs hci_connect_sco(), which was also asked about: the
reference hci_connect_sco() returns is released by hci_conn_drop() on
each error path of sco_connect(), and on the success path it is handed to
the connection and released by sco_conn_free(). That side looks balanced.
There is a separate asymmetry that this patch does not touch: when
sco_conn_add() returns a connection that already existed for the hcon,
hci_connect_sco() has taken a fresh hci_conn reference but sco_conn_free()
only ever issues one hci_conn_drop(). That looks like a pre-existing
hci_conn leak rather than an sco_conn one; I did not want to fold it into
this fix.

Testing: the original defect reproduced 45 times across 2 independent
runs on unmodified v7.2-rc1-240-g71dfdfb0209b with KASAN, driven through
/dev/vhci by racing close() of an SCO socket against an injected
Disconnection Complete; both KASAN and the refcount_t underflow fired
every time. The BlueZ CI ran sco-tester against v1 with no regression.

 net/bluetooth/sco.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index fcc597be5bbd..21f829575803 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -81,9 +81,6 @@ static void sco_conn_free(struct kref *r
 
 	BT_DBG("conn %p", conn);
 
-	if (conn->sk)
-		sco_pi(conn->sk)->conn = NULL;
-
 	if (conn->hcon) {
 		conn->hcon->sco_data = NULL;
 		hci_conn_drop(conn->hcon);
@@ -265,10 +262,8 @@ static void sco_conn_del(struct hci_conn
 	sco_conn_unlock(conn);
 	sco_conn_put(conn);
 
-	if (!sk) {
-		sco_conn_put(conn);
+	if (!sk)
 		return;
-	}
 
 	/* Kill socket */
 	lock_sock(sk);
@@ -283,7 +278,7 @@ static void __sco_chan_add(struct sco_co
 {
 	BT_DBG("conn %p", conn);
 
-	sco_pi(sk)->conn = conn;
+	sco_pi(sk)->conn = sco_conn_hold(conn);
 	conn->sk = sk;
 
 	if (parent)
@@ -366,12 +361,14 @@ 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;
 	}
 
 	err = sco_chan_add(conn, sk, NULL);
+	sco_conn_put(conn);
 	if (err) {
 		release_sock(sk);
 		hci_conn_drop(hcon);
@@ -1439,7 +1436,6 @@ static void sco_conn_ready(struct sco_co
 		bacpy(&sco_pi(sk)->src, &conn->hcon->src);
 		bacpy(&sco_pi(sk)->dst, &conn->hcon->dst);
 
-		sco_conn_hold(conn);
 		hci_conn_hold(conn->hcon);
 		__sco_chan_add(conn, sk, parent);
 
-- 
2.43.0
 
 

Re: [PATCH v2] Bluetooth: SCO: give the socket its own sco_conn reference

[Pauli Virtanen]

Hi,

to, 2026-07-23 kello 20:29 -0300, Aldo Ariel Panzardo kirjoitti:
> sco_conn_del() drops a reference it does not own. It takes one transient
> reference via sco_conn_hold_unless_zero() and releases it with the
> sco_conn_put() that follows sco_sock_hold(); the additional put in the
> !sk branch releases a second one:
> 
>     conn = sco_conn_hold_unless_zero(conn);
>     ...
>     sk = sco_sock_hold(conn);
>     sco_conn_unlock(conn);
>     sco_conn_put(conn);
> 
>     if (!sk) {
>             sco_conn_put(conn);
>             return;
>     }
> 
> When close() races the controller's Disconnection Complete, sco_chan_del()
> clears conn->sk and drops the socket's reference while sco_conn_del() is
> running. sco_conn_del() then sees sk == NULL, its own put drops the count
> to zero and frees the conn, and the second put writes to the freed kref:
> 
>     BUG: KASAN: slab-use-after-free in sco_conn_put.part.0+0x1a/0x190
>     Write of size 4 at addr ffff8881099dec74 by task kworker/u17:3/413
>     Workqueue: hci1 hci_rx_work
>     Call Trace:
>      sco_conn_put.part.0+0x1a/0x190
>      hci_disconn_complete_evt+0x1ee/0x3e0
>      hci_event_packet+0x54a/0x650
>      hci_rx_work+0x321/0x3d0
>     Allocated by task 413:
>      sco_conn_add+0x72/0x1a0
>      sco_connect_cfm+0x88/0x670
>     Freed by task 413:
>      sco_conn_del.isra.0+0x3f/0xf0
>      hci_disconn_complete_evt+0x1ee/0x3e0
>     refcount_t: underflow; use-after-free.
> 
> Simply deleting the extra put is not enough, because the reference it
> releases is not always accounted for elsewhere. __sco_chan_add() stores
> the connection in the socket without taking a reference:
> 
>     sco_pi(sk)->conn = conn;
> 
> so the socket inherits whatever reference its caller happened to hold.
> That works out for sco_conn_ready(), which takes an explicit
> sco_conn_hold() beforehand and whose caller puts its own reference, and
> for the success path of sco_connect(), where the reference returned by
> sco_conn_add() is silently handed over and later released by
> sco_sock_destruct(). It does not work out for the two error paths of
> sco_connect(): if the socket state changed while the lock was dropped, or
> if sco_chan_add() returns -EBUSY, the reference from sco_conn_add() is
> never released and the connection is leaked. The extra put in
> sco_conn_del() is what eventually reclaims those orphans, which is why
> removing it in isolation trades a use-after-free for a leak.
> 
> Make the ownership explicit instead. __sco_chan_add() now takes the
> socket's reference itself, sco_connect() releases the one it got from
> sco_conn_add() on every path, and the now redundant hold in
> sco_conn_ready() is dropped. With the socket holding a counted reference,
> a connection can no longer reach zero while conn->sk is set, so
> sco_conn_free() no longer has to clear sco_pi(conn->sk)->conn. Every
> reference then has exactly one owner: the one sco_conn_add() returns
> belongs to its caller, the socket's is taken and released with the
> channel, and sco_conn_del() and sco_sock_timeout() only ever hold
> transient ones.
> 
> Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn")
> Cc: stable@vger.kernel.org
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
> v2:
>  - Do not just delete the extra put: make the socket own its reference,
>    balance sco_connect()'s error paths and drop the redundant hold in
>    sco_conn_ready(), per Pauli Virtanen's review.
>  - Drop the now unreachable sco_pi(conn->sk)->conn clearing in
>    sco_conn_free().
>  - Indent the quoted code with spaces so gitlint stops complaining.
> 
> On hci_conn_drop() vs hci_connect_sco(), which was also asked about: the
> reference hci_connect_sco() returns is released by hci_conn_drop() on
> each error path of sco_connect(), and on the success path it is handed to
> the connection and released by sco_conn_free(). That side looks balanced.
> There is a separate asymmetry that this patch does not touch: when
> sco_conn_add() returns a connection that already existed for the hcon,
> hci_connect_sco() has taken a fresh hci_conn reference but sco_conn_free()
> only ever issues one hci_conn_drop(). That looks like a pre-existing
> hci_conn leak rather than an sco_conn one; I did not want to fold it into
> this fix.

There's a double drop on the error paths now, so it'll probably hit
WARN_ON() if these are reached, but it's not fatal.

The SCO refcounting rule probably should be that sco_conn owns one
hci_conn_hold reference all of its lifetime.

hci_connect_sco() returns a hci_conn with a new hci_conn_hold reference
given to the caller.

sco_connect_cfm() never gives callee a hci_conn_hold refcount.

So probably (this will have to be thought out better and maybe separate
patch):

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 5cab7e2fb898..0e1dd6a8e6d7 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -185,6 +185,8 @@ static void sco_sock_clear_timer(struct sock *sk)
 }
 
 /* ---- SCO connections ---- */
+
+/* Consumes hci_conn_hold refcount */
 static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
 {
 	struct sco_conn *conn = hcon->sco_data;
@@ -195,6 +197,9 @@ static struct sco_conn *sco_conn_add(struct
hci_conn *hcon)
 			sco_conn_lock(conn);
 			conn->hcon = hcon;
 			sco_conn_unlock(conn);
+		} else {
+			/* We already own the refcount */
+			hci_conn_drop(hcon);
 		}
 		return conn;
 	}
@@ -362,7 +367,6 @@ 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;
 	}
@@ -371,7 +375,6 @@ static int sco_connect(struct sock *sk)
 	sco_conn_put(conn);
 	if (err) {
 		release_sock(sk);
-		hci_conn_drop(hcon);
 		goto unlock;
 	}
 
@@ -1449,7 +1452,6 @@ static void sco_conn_ready(struct sco_conn *conn)
 		bacpy(&sco_pi(sk)->src, &conn->hcon->src);
 		bacpy(&sco_pi(sk)->dst, &conn->hcon->dst);
 
-		hci_conn_hold(conn->hcon);
 		__sco_chan_add(conn, sk, parent);
 
 		if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)-
>flags))
@@ -1505,7 +1507,7 @@ static void sco_connect_cfm(struct hci_conn
*hcon, __u8 status)
 	if (!status) {
 		struct sco_conn *conn;
 
-		conn = sco_conn_add(hcon);
+		conn = sco_conn_add(hci_conn_hold(hcon));
 		if (conn) {
 			sco_conn_ready(conn);
 			sco_conn_put(conn);

> 
> Testing: the original defect reproduced 45 times across 2 independent
> runs on unmodified v7.2-rc1-240-g71dfdfb0209b with KASAN, driven through
> /dev/vhci by racing close() of an SCO socket against an injected
> Disconnection Complete; both KASAN and the refcount_t underflow fired
> every time. The BlueZ CI ran sco-tester against v1 with no regression.
> 
>  net/bluetooth/sco.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index fcc597be5bbd..21f829575803 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -81,9 +81,6 @@ static void sco_conn_free(struct kref *r
>  
>  	BT_DBG("conn %p", conn);
>  
> -	if (conn->sk)
> -		sco_pi(conn->sk)->conn = NULL;
> -
>  	if (conn->hcon) {
>  		conn->hcon->sco_data = NULL;
>  		hci_conn_drop(conn->hcon);
> @@ -265,10 +262,8 @@ static void sco_conn_del(struct hci_conn
>  	sco_conn_unlock(conn);
>  	sco_conn_put(conn);
>  
> -	if (!sk) {
> -		sco_conn_put(conn);
> +	if (!sk)
>  		return;
> -	}
>  
>  	/* Kill socket */
>  	lock_sock(sk);
> @@ -283,7 +278,7 @@ static void __sco_chan_add(struct sco_co
>  {
>  	BT_DBG("conn %p", conn);
>  
> -	sco_pi(sk)->conn = conn;
> +	sco_pi(sk)->conn = sco_conn_hold(conn);
>  	conn->sk = sk;
>  
>  	if (parent)
> @@ -366,12 +361,14 @@ 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;
>  	}
>  
>  	err = sco_chan_add(conn, sk, NULL);
> +	sco_conn_put(conn);
>  	if (err) {
>  		release_sock(sk);
>  		hci_conn_drop(hcon);
> @@ -1439,7 +1436,6 @@ static void sco_conn_ready(struct sco_co
>  		bacpy(&sco_pi(sk)->src, &conn->hcon->src);
>  		bacpy(&sco_pi(sk)->dst, &conn->hcon->dst);
>  
> -		sco_conn_hold(conn);
>  		hci_conn_hold(conn->hcon);
>  		__sco_chan_add(conn, sk, parent);
>  

-- 
Pauli Virtanen

[PATCH v3] Bluetooth: SCO: give the socket its own sco_conn reference

[Aldo Ariel Panzardo]

sco_conn_del() drops a reference it does not own. It takes one transient
reference via sco_conn_hold_unless_zero() and releases it with the
sco_conn_put() that follows sco_sock_hold(); the additional put in the
!sk branch releases a second one:

    conn = sco_conn_hold_unless_zero(conn);
    ...
    sk = sco_sock_hold(conn);
    sco_conn_unlock(conn);
    sco_conn_put(conn);

    if (!sk) {
            sco_conn_put(conn);
            return;
    }

When close() races the controller's Disconnection Complete, sco_chan_del()
clears conn->sk and drops the socket's reference while sco_conn_del() is
running. sco_conn_del() then sees sk == NULL, its own put drops the count
to zero and frees the conn, and the second put writes to the freed kref:

    BUG: KASAN: slab-use-after-free in sco_conn_put.part.0+0x1a/0x190
    Write of size 4 at addr ffff8881099dec74 by task kworker/u17:3/413
    Workqueue: hci1 hci_rx_work
    Call Trace:
     sco_conn_put.part.0+0x1a/0x190
     hci_disconn_complete_evt+0x1ee/0x3e0
     hci_event_packet+0x54a/0x650
     hci_rx_work+0x321/0x3d0
    Allocated by task 413:
     sco_conn_add+0x72/0x1a0
     sco_connect_cfm+0x88/0x670
    Freed by task 413:
     sco_conn_del.isra.0+0x3f/0xf0
     hci_disconn_complete_evt+0x1ee/0x3e0
    refcount_t: underflow; use-after-free.

The root cause is that the socket stores the connection without holding a
reference of its own. __sco_chan_add() does:

    sco_pi(sk)->conn = conn;

so the socket borrows whatever reference its caller happened to hold, and
the callers paper over that with ad-hoc holds and puts. Give the socket a
counted reference instead: __sco_chan_add() takes one and it is released
together with the channel (sco_chan_del()) and in sco_sock_destruct().
With the socket holding its own reference, sco_conn_del() no longer needs
the extra put and the redundant hold in sco_conn_ready() goes away.

Making the socket own its reference means the connection is now actually
freed on the error paths of sco_connect() where it used to leak, which in
turn runs sco_conn_free() and its hci_conn_drop(conn->hcon). To keep the
hci_conn accounting balanced, make that ownership explicit as well:
sco_conn_add() consumes one hci_conn reference and the sco_conn owns it for
its lifetime. sco_connect() hands over the reference returned by
hci_connect_sco() and no longer drops it on the error paths;
sco_connect_cfm(), which is not given a reference, takes one with
hci_conn_hold() before handing it to sco_conn_add() (and drops it again if
the allocation fails); and the explicit hci_conn_hold() in sco_conn_ready()
is removed. Every reference then has a single, clear owner.

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>
---
v3:
 - Incorporate Pauli Virtanen's review: make sco_conn own one hci_conn
   reference for its whole lifetime -- sco_conn_add() consumes an hci_conn
   reference, sco_connect() no longer drops hcon on its error paths,
   sco_connect_cfm() holds one before sco_conn_add(), and the hci_conn_hold()
   in sco_conn_ready() is removed. This avoids the double hci_conn_drop() on
   sco_connect()'s error paths that v2 would otherwise introduce.
 - Keep the sco_pi(conn->sk)->conn = NULL clearing in sco_conn_free(). v2
   removed it as unreachable, but KASAN testing of close() racing the
   Disconnection Complete showed that dropping it reintroduces a
   use-after-free on the sco_sock_release() path, so it is retained.
 - Drop the hcon reference in sco_connect_cfm() when sco_conn_add() fails,
   so the allocation-failure path does not leak it.
v2:
 - Make the socket own its sco_conn reference rather than only deleting the
   extra put, per Pauli Virtanen's review.

Testing: on v7.2-rc4 with KASAN and a /dev/vhci reproducer that races
close() of an SCO socket against an injected Disconnection Complete, the
unpatched kernel hits the refcount_t underflow / use-after-free above
within a few thousand iterations; with this patch the sco_conn_del()
over-put on the Disconnection Complete path no longer reproduces.

 net/bluetooth/sco.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index fcc597be5bbd..aa9f61a748ab 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -188,6 +188,9 @@ 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.
+ */
 static struct sco_conn *sco_conn_add(struct hci_conn *hcon)
 {
 	struct sco_conn *conn = hcon->sco_data;
@@ -198,6 +201,9 @@ 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;
 	}
@@ -265,10 +271,8 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
 	sco_conn_unlock(conn);
 	sco_conn_put(conn);
 
-	if (!sk) {
-		sco_conn_put(conn);
+	if (!sk)
 		return;
-	}
 
 	/* Kill socket */
 	lock_sock(sk);
@@ -283,7 +287,7 @@ static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
 {
 	BT_DBG("conn %p", conn);
 
-	sco_pi(sk)->conn = conn;
+	sco_pi(sk)->conn = sco_conn_hold(conn);
 	conn->sk = sk;
 
 	if (parent)
@@ -366,15 +370,15 @@ static int sco_connect(struct sock *sk)
 	 */
 	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
 		release_sock(sk);
-		hci_conn_drop(hcon);
+		sco_conn_put(conn);
 		err = -EBADFD;
 		goto unlock;
 	}
 
 	err = sco_chan_add(conn, sk, NULL);
+	sco_conn_put(conn);
 	if (err) {
 		release_sock(sk);
-		hci_conn_drop(hcon);
 		goto unlock;
 	}
 
@@ -1439,8 +1443,6 @@ static void sco_conn_ready(struct sco_conn *conn)
 		bacpy(&sco_pi(sk)->src, &conn->hcon->src);
 		bacpy(&sco_pi(sk)->dst, &conn->hcon->dst);
 
-		sco_conn_hold(conn);
-		hci_conn_hold(conn->hcon);
 		__sco_chan_add(conn, sk, parent);
 
 		if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags))
@@ -1496,10 +1498,12 @@ static void sco_connect_cfm(struct hci_conn *hcon, __u8 status)
 	if (!status) {
 		struct sco_conn *conn;
 
-		conn = sco_conn_add(hcon);
+		conn = sco_conn_add(hci_conn_hold(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: [PATCH v2] Bluetooth: SCO: give the socket its own sco_conn reference

[Aldo Ariel Panzardo]

Hi Pauli,

Thanks a lot for the detailed review and for sketching the hci_conn
ownership rule -- v3 incorporates it (you're on the patch as Suggested-by):
sco_conn_add() now consumes one hci_conn reference and the sco_conn owns it
for its whole lifetime, sco_connect() no longer drops hcon on its error
paths, sco_connect_cfm() takes a reference before sco_conn_add() (and drops
it again if the allocation fails), and the hci_conn_hold() in
sco_conn_ready() is gone. That removes the double hci_conn_drop() you
spotted on the error paths.

One correction to v2: I had also dropped the

	if (conn->sk)
		sco_pi(conn->sk)->conn = NULL;

clearing in sco_conn_free() as "unreachable". v3 keeps it. Testing the
close()-vs-Disconnection-Complete race under KASAN showed that removing it
reintroduces a use-after-free on the sco_sock_release() path, so it stays.

While validating this I built a /dev/vhci reproducer that races close() of
an SCO socket against an injected Disconnection Complete over many
iterations, on v7.2-rc4 with KASAN. It reproduces the sco_conn_del()
over-put reliably on an unmodified tree (a few thousand iterations), and
with this patch that over-put no longer fires.

I want to be upfront that it does not leave the reproducer fully clean,
though. With the over-put fixed, the same race still occasionally trips a
separate use-after-free in the SCODATA receive path, which this patch does
not touch:

    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

sco_recv_scodata() takes conn from hcon->sco_data and calls
sco_conn_hold_unless_zero() on it, but kref_get_unless_zero() only guards
against a zero refcount, not against the sco_conn already having been
freed and its memory reclaimed -- the read of the refcount itself is the
UAF. It looks like a pre-existing race around the hcon->sco_data weak
pointer rather than something introduced here, but I haven't yet isolated
it on an otherwise-unmodified tree, so I didn't want to fold a fix into
this patch blind.

Happy to dig into that one next (it feels like the sco_conn lifetime wants
a firmer owner for hcon->sco_data), either as a follow-up or however you'd
prefer to see it handled. Let me know if you'd rather I respin this
together with that.

Thanks,
Aldo

Re: [PATCH v3] Bluetooth: SCO: give the socket its own sco_conn reference

[patchwork-bot+bluetooth]

Hello:

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

On Sat, 25 Jul 2026 16:52:30 -0300 you wrote:
> sco_conn_del() drops a reference it does not own. It takes one transient
> reference via sco_conn_hold_unless_zero() and releases it with the
> sco_conn_put() that follows sco_sock_hold(); the additional put in the
> !sk branch releases a second one:
> 
>     conn = sco_conn_hold_unless_zero(conn);
>     ...
>     sk = sco_sock_hold(conn);
>     sco_conn_unlock(conn);
>     sco_conn_put(conn);
> 
> [...]

Here is the summary with links:
  - [v3] Bluetooth: SCO: give the socket its own sco_conn reference
    https://git.kernel.org/bluetooth/bluetooth-next/c/018d1e023ea5

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