[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: [v2] Bluetooth: SCO: give the socket its own sco_conn reference

[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=1133573

---Test result---

Test Summary:
CheckPatch                    PASS      0.75 seconds
VerifyFixes                   PASS      0.13 seconds
VerifySignedoff               PASS      0.14 seconds
GitLint                       PASS      0.34 seconds
SubjectPrefix                 PASS      0.15 seconds
BuildKernel                   PASS      25.23 seconds
CheckAllWarning               PASS      28.01 seconds
CheckSparse                   PASS      26.67 seconds
BuildKernel32                 PASS      24.81 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      461.05 seconds
TestRunner_sco-tester         PASS      31.22 seconds
IncrementalBuild              PASS      24.43 seconds

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


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

---
Regards,
Linux Bluetooth

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: [v3] Bluetooth: SCO: give the socket its own sco_conn reference

[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=1134465

---Test result---

Test Summary:
CheckPatch                    PASS      0.73 seconds
VerifyFixes                   PASS      0.13 seconds
VerifySignedoff               PASS      0.12 seconds
GitLint                       PASS      0.32 seconds
SubjectPrefix                 PASS      0.12 seconds
BuildKernel                   PASS      27.14 seconds
CheckAllWarning               PASS      29.74 seconds
CheckSparse                   PASS      28.33 seconds
BuildKernel32                 PASS      26.03 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      496.84 seconds
TestRunner_sco-tester         PASS      31.82 seconds
IncrementalBuild              PASS      26.14 seconds

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


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

---
Regards,
Linux Bluetooth