[PATCH] Bluetooth: SCO: fix refcount over-put in sco_conn_del()

[PATCH] Bluetooth: SCO: fix refcount over-put in sco_conn_del()

[Your Name]

From: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

sco_conn_del() takes exactly one transient reference to the sco_conn via
sco_conn_hold_unless_zero() and drops it with the sco_conn_put() that
follows sco_sock_hold().  The additional sco_conn_put() in the !sk branch
drops a reference the function never acquired:

	conn = sco_conn_hold_unless_zero(conn);
	if (!conn)
		return;
	...
	sco_conn_lock(conn);
	sk = sco_sock_hold(conn);
	sco_conn_unlock(conn);
	sco_conn_put(conn);

	if (!sk) {
		sco_conn_put(conn);	/* drops a reference we do not own */
		return;
	}

sco_sock_timeout() has the same entry semantics -- one
sco_conn_hold_unless_zero(), no incoming reference owned -- and simply
returns from its !sk branch with no second put.

In steady state the only counted reference to a struct sco_conn is the
one held by the socket.  When close() races the controller's Disconnection
Complete, sco_chan_del() clears conn->sk and drops the socket reference
while sco_conn_del() is running.  sco_conn_del() then observes 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
   hci_conn_complete_evt+0x4c5/0x890
  Freed by task 413:
   sco_conn_del.isra.0+0x3f/0xf0
   hci_disconn_complete_evt+0x1ee/0x3e0
  refcount_t: underflow; use-after-free.

The freed object is a kmalloc-128 allocation, so this is an out-of-bounds
write into a reclaimed slab object rather than a plain crash.  Opening an
SCO socket requires no capability, and the race is reached whenever the
controller delivers a Disconnection Complete concurrently with the socket
being closed.

A related refcount imbalance introduced by the same commit was already
fixed in commit ed9588554943 ("Bluetooth: SCO: remove the redundant
sco_conn_put").

Drop the extra put so the !sk branch simply returns.

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>
---
 net/bluetooth/sco.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index fcc597be5bbd..6502fcdfc0f0 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -265,10 +265,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);
-- 
2.43.0

Re: [PATCH] Bluetooth: SCO: fix refcount over-put in sco_conn_del()

[Pauli Virtanen]

Hi,

to, 2026-07-23 kello 19:10 -0300, Your Name kirjoitti:
> From: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> 
> sco_conn_del() takes exactly one transient reference to the sco_conn via
> sco_conn_hold_unless_zero() and drops it with the sco_conn_put() that
> follows sco_sock_hold().  The additional sco_conn_put() in the !sk branch
> drops a reference the function never acquired:
> 
> 	conn = sco_conn_hold_unless_zero(conn);
> 	if (!conn)
> 		return;
> 	...
> 	sco_conn_lock(conn);
> 	sk = sco_sock_hold(conn);
> 	sco_conn_unlock(conn);
> 	sco_conn_put(conn);
> 
> 	if (!sk) {
> 		sco_conn_put(conn);	/* drops a reference we do not own */
> 		return;
> 	}
> 
> sco_sock_timeout() has the same entry semantics -- one
> sco_conn_hold_unless_zero(), no incoming reference owned -- and simply
> returns from its !sk branch with no second put.
> 
> In steady state the only counted reference to a struct sco_conn is the
> one held by the socket.  When close() races the controller's Disconnection
> Complete, sco_chan_del() clears conn->sk and drops the socket reference
> while sco_conn_del() is running.  sco_conn_del() then observes 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
>    hci_conn_complete_evt+0x4c5/0x890
>   Freed by task 413:
>    sco_conn_del.isra.0+0x3f/0xf0
>    hci_disconn_complete_evt+0x1ee/0x3e0
>   refcount_t: underflow; use-after-free.
> 
> The freed object is a kmalloc-128 allocation, so this is an out-of-bounds
> write into a reclaimed slab object rather than a plain crash.  Opening an
> SCO socket requires no capability, and the race is reached whenever the
> controller delivers a Disconnection Complete concurrently with the socket
> being closed.
> 
> A related refcount imbalance introduced by the same commit was already
> fixed in commit ed9588554943 ("Bluetooth: SCO: remove the redundant
> sco_conn_put").
> 
> Drop the extra put so the !sk branch simply returns.
> 
> 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>
> ---
>  net/bluetooth/sco.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index fcc597be5bbd..6502fcdfc0f0 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -265,10 +265,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;

This will leak the sco_conn under some conditions, you probably need
something like the below and check all code paths touching the sco_conn
are consistent who owns the reference. 

Previously the idea appears to have been it is sometimes owned by
hci_conn::sco_data but it's probably simpler vs race conditions to have
it owned only by sco_pi(sk)->conn.

With this change, sco_conn_free() probably should not touch
sco_pi(conn->sk)->conn

Please also check the hci_conn_drop() refcounting vs. hci_connect_sco()
is OK.

Please also describe to what extent this was tested.


diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index c05f79b7aa31..3000b723f081 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -283,7 +283,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,12 +366,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);
@@ -1452,7 +1454,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);

> -	}
>  
>  	/* Kill socket */
>  	lock_sock(sk);

-- 
Pauli Virtanen

RE: Bluetooth: SCO: fix refcount over-put in sco_conn_del()

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2205 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=1133551

---Test result---

Test Summary:
CheckPatch                    PASS      0.57 seconds
VerifyFixes                   PASS      0.08 seconds
VerifySignedoff               PASS      0.50 seconds
GitLint                       FAIL      0.22 seconds
SubjectPrefix                 PASS      0.07 seconds
BuildKernel                   PASS      27.98 seconds
CheckAllWarning               PASS      30.76 seconds
CheckSparse                   PASS      29.85 seconds
BuildKernel32                 PASS      27.22 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      517.17 seconds
TestRunner_sco-tester         PASS      33.44 seconds
IncrementalBuild              PASS      26.28 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
Bluetooth: SCO: fix refcount over-put in sco_conn_del()

10: B3 Line contains hard tab characters (\t): "	conn = sco_conn_hold_unless_zero(conn);"
11: B3 Line contains hard tab characters (\t): "	if (!conn)"
12: B3 Line contains hard tab characters (\t): "		return;"
13: B3 Line contains hard tab characters (\t): "	..."
14: B3 Line contains hard tab characters (\t): "	sco_conn_lock(conn);"
15: B3 Line contains hard tab characters (\t): "	sk = sco_sock_hold(conn);"
16: B3 Line contains hard tab characters (\t): "	sco_conn_unlock(conn);"
17: B3 Line contains hard tab characters (\t): "	sco_conn_put(conn);"
19: B3 Line contains hard tab characters (\t): "	if (!sk) {"
20: B3 Line contains hard tab characters (\t): "		sco_conn_put(conn);	/* drops a reference we do not own */"
21: B3 Line contains hard tab characters (\t): "		return;"
22: B3 Line contains hard tab characters (\t): "	}"
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found


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

---
Regards,
Linux Bluetooth