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

PATCH In review Your Name 3 participants 3 messages Jul 23

Subsystems (3)

Stable branch Bluetooth drivers Bluetooth

lore

Share & tools

lore

Your Name

author

2026-07-23 22:10 UTC #0 lore raw

PATCH 1/1 Bluetooth: SCO: fix refcount over-put in sco_conn_del()

From: Aldo Ariel Panzardo <[email protected]>

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 [email protected]

Signed-off-by Aldo Ariel Panzardo <[email protected]>

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

net/bluetooth/sco.c +1 -3

index fcc597be5bbd..6502fcdfc0f0 100644
@@ -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);

Signature · 1 line

2.43.0

Pauli Virtanen

2026-07-23 22:51 UTC #1 lore raw

Hi,

to, 2026-07-23 kello 19:10 -0300, Your Name kirjoitti:

Quoted 73 lines

From: Aldo Ariel Panzardo <[email protected]>
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 [email protected]
Signed-off-by Aldo Ariel Panzardo <[email protected]>
---
 net/bluetooth/sco.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
net/bluetooth/sco.c +0 -0
index fcc597be5bbd..6502fcdfc0f0 100644

net/bluetooth/sco.c static void sco_conn_del(struct hci_conn *hcon, int err) · line 269

Quoted 7 lines

net/bluetooth/sco.c +1 -2

@@ -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.

net/bluetooth/sco.c +0 -0

index c05f79b7aa31..3000b723f081 100644
@@ -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);

Quoted 4 lines

net/bluetooth/sco.c +0 -1

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

Signature · 1 line

Pauli Virtanen

[email protected]

2026-07-23 23:07 UTC #2 lore raw

In reply to Your Name

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