[PATCH] Bluetooth: SCO: fix sco_conn double free on outgoing connect

[PATCH] Bluetooth: SCO: fix sco_conn double free on outgoing connect

[Baul Lee]

sco_conn is refcounted with a kref that sco_conn_add() initialises to 1.
That single reference is the connection's association reference, owned
by the hcon and released when the link goes down.

The incoming attach path takes an extra reference for the socket before
__sco_chan_add(), but the outgoing path, sco_connect() -> sco_chan_add(),
does not.  An outgoing SCO socket is therefore attached while conn->ref
is still 1, and two unrelated teardown paths both release that one
reference: sco_chan_del(), reached from close(), and the !sk branch of
sco_conn_del(), reached from the sco_connect_cfm() / sco_disconn_cfm()
callbacks.

When an outgoing SCO setup fails while the socket is being closed, the
two race, starting from conn->ref == 1:

  1. sco_chan_del() clears conn->sk.
  2. sco_conn_del() takes a temporary reference (ref 2) and, seeing
     conn->sk already cleared, gets a NULL sk.
  3. sco_chan_del() puts the reference it believes it owns (ref 1).
  4. sco_conn_del() drops its temporary reference (ref 0) and the
     sco_conn is freed.
  5. sco_conn_del() takes the !sk branch and puts the freed object.

KASAN reports a slab-use-after-free of the kmalloc-128 sco_conn in
sco_chan_del(), followed by a refcount_t underflow.  Both operations are
reachable from an unprivileged AF_BLUETOOTH / BTPROTO_SCO socket doing
connect() and close().

Give the socket its own reference on the outgoing attach so that the two
teardown owners no longer contend for a single reference, and drop the
association reference in sco_conn_del()'s socket-kill path so that it is
released exactly once there as well, symmetrically with the existing !sk
branch.  sco_conn_ready() consequently has to take both references
itself, because sco_connect_cfm() puts the one from sco_conn_add() as
soon as sco_conn_ready() returns.

Discovered by XBOW, triaged by Baul Lee <baul.lee@xbow.com>
Reported privately to the maintainers on 2026-07-10 with root-cause
analysis, a PoC, a KASAN log and this fix; posting to the list was
requested as the follow-up.

Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn")
Reported-by: Federico Kirschbaum <federico.kirschbaum@xbow.com>
Reported-by: Baul Lee <baul.lee@xbow.com>
Cc: stable@vger.kernel.org
Signed-off-by: Baul Lee <baul.lee@xbow.com>
---
 net/bluetooth/sco.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index c05f79b7aa31..3db6552de06c 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -276,6 +276,9 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
 	sco_chan_del(sk, err);
 	release_sock(sk);
 	sock_put(sk);
+
+	/* Drop the association reference, as the !sk branch above does */
+	sco_conn_put(conn);
 }
 
 static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
@@ -296,10 +299,16 @@ static int sco_chan_add(struct sco_conn *conn, struct sock *sk,
 	int err = 0;
 
 	sco_conn_lock(conn);
-	if (conn->sk || sco_pi(sk)->conn)
+	if (conn->sk || sco_pi(sk)->conn) {
 		err = -EBUSY;
-	else
+	} else {
+		/* Take the socket reference, which sco_chan_del() drops when
+		 * the socket detaches.  Without it the socket and the hcon
+		 * would share the single reference from sco_conn_add().
+		 */
+		sco_conn_hold(conn);
 		__sco_chan_add(conn, sk, parent);
+	}
 
 	sco_conn_unlock(conn);
 	return err;
@@ -1452,6 +1461,13 @@ 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);
 
+		/* Two references are needed here: the socket one, dropped by
+		 * sco_chan_del(), and the association one, dropped by
+		 * sco_conn_del().  Unlike the outgoing path, the reference
+		 * from sco_conn_add() cannot serve as the latter, because
+		 * sco_connect_cfm() puts it as soon as this function returns.
+		 */
+		sco_conn_hold(conn);
 		sco_conn_hold(conn);
 		hci_conn_hold(conn->hcon);
 		__sco_chan_add(conn, sk, parent);
-- 
2.50.1 (Apple Git-155)

RE: Bluetooth: SCO: fix sco_conn double free on outgoing connect

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    FAIL      0.76 seconds
VerifyFixes                   PASS      0.13 seconds
VerifySignedoff               PASS      0.14 seconds
GitLint                       PASS      0.36 seconds
SubjectPrefix                 PASS      0.13 seconds
BuildKernel                   PASS      28.19 seconds
CheckAllWarning               PASS      30.75 seconds
CheckSparse                   PASS      30.22 seconds
BuildKernel32                 PASS      27.15 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      533.50 seconds
TestRunner_sco-tester         FAIL      35.18 seconds
IncrementalBuild              PASS      27.47 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
Bluetooth: SCO: fix sco_conn double free on outgoing connect
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#142: 
Reported-by: Federico Kirschbaum <federico.kirschbaum@xbow.com>
Reported-by: Baul Lee <baul.lee@xbow.com>

WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#143: 
Reported-by: Baul Lee <baul.lee@xbow.com>
Cc: stable@vger.kernel.org

total: 0 errors, 2 warnings, 0 checks, 40 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14710765.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found
##############################
Test: TestRunner_sco-tester - FAIL
Desc: Run sco-tester with test-runner
Output:
Total: 30, Passed: 29 (96.7%), Failed: 1, Not Run: 0

Failed Test Cases
SCO Disconnect - Success                             Timed out    2.151 seconds


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

---
Regards,
Linux Bluetooth

Re: [PATCH] Bluetooth: SCO: fix sco_conn double free on outgoing connect

[Pauli Virtanen]

Hi,

su, 2026-07-26 kello 14:54 +0900, Baul Lee kirjoitti:
> sco_conn is refcounted with a kref that sco_conn_add() initialises to 1.
> That single reference is the connection's association reference, owned
> by the hcon and released when the link goes down.
> 
> The incoming attach path takes an extra reference for the socket before
> __sco_chan_add(), but the outgoing path, sco_connect() -> sco_chan_add(),
> does not.  An outgoing SCO socket is therefore attached while conn->ref
> is still 1, and two unrelated teardown paths both release that one
> reference: sco_chan_del(), reached from close(), and the !sk branch of
> sco_conn_del(), reached from the sco_connect_cfm() / sco_disconn_cfm()
> callbacks.

This appears to break closing SCO connections, indeed "SCO Disconnect -
Success" fails now, due to hci_conn_drop() not called on socket close
since one sco_conn refcount is now owned by hci_conn::sco_data.

The socket should own a hci_conn_hold/drop refcount that it drops when
closing.

Another option is to have only socket own sco_conn, like here
https://lore.kernel.org/linux-bluetooth/20260725195230.967546-1-qwe.aldo@gmail.com/

However, in that solution one would still need additional
synchronization to deal with the race between sco_conn_del(),
sco_conn_free(), sco_conn_add() vs. hci_conn::sco_data access
https://lore.kernel.org/linux-bluetooth/b84fd01c20f0d2975c7817da345a6937d67cf314.1784923689.git.pav@iki.fi/


In the patch here there seems to be some remaining UAF, could make
sense to try to address them while at it

sco_chan_del()				sco_conn_del()
  sco_conn_lock
  conn->sk = NULL
  sco_conn_unlock
					sk = ... /* = NULL */
  					if (!sk)
					  { sco_conn_put; return; }
					/* free hci_conn */
  sco_conn_put
    conn->hcon->sco_data = NULL /* UAF */

and

sco_sock_timeout()			sco_conn_del()
  conn = ...				conn = ...
  ...	  				...
					sco_conn_put()
					free hci_conn
  sco_conn_put()
    conn->hcon->sco_data = NULL /* UAF */

since access to sco_data is not serialized by any lock. Probably it can
be cleared in sco_conn_del() so it can get
__guarded_by(&hdev->lock) context analysis annotation
https://docs.kernel.org/dev-tools/context-analysis.html

> When an outgoing SCO setup fails while the socket is being closed, the
> two race, starting from conn->ref == 1:
> 
>   1. sco_chan_del() clears conn->sk.
>   2. sco_conn_del() takes a temporary reference (ref 2) and, seeing
>      conn->sk already cleared, gets a NULL sk.
>   3. sco_chan_del() puts the reference it believes it owns (ref 1).
>   4. sco_conn_del() drops its temporary reference (ref 0) and the
>      sco_conn is freed.
>   5. sco_conn_del() takes the !sk branch and puts the freed object.
> 
> KASAN reports a slab-use-after-free of the kmalloc-128 sco_conn in
> sco_chan_del(), followed by a refcount_t underflow.  Both operations are
> reachable from an unprivileged AF_BLUETOOTH / BTPROTO_SCO socket doing
> connect() and close().
> 
> Give the socket its own reference on the outgoing attach so that the two
> teardown owners no longer contend for a single reference, and drop the
> association reference in sco_conn_del()'s socket-kill path so that it is
> released exactly once there as well, symmetrically with the existing !sk
> branch.  sco_conn_ready() consequently has to take both references
> itself, because sco_connect_cfm() puts the one from sco_conn_add() as
> soon as sco_conn_ready() returns.
> 
> Discovered by XBOW, triaged by Baul Lee <baul.lee@xbow.com>
> Reported privately to the maintainers on 2026-07-10 with root-cause
> analysis, a PoC, a KASAN log and this fix; posting to the list was
> requested as the follow-up.
> 
> Fixes: e6720779ae61 ("Bluetooth: SCO: Use kref to track lifetime of sco_conn")
> Reported-by: Federico Kirschbaum <federico.kirschbaum@xbow.com>
> Reported-by: Baul Lee <baul.lee@xbow.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Baul Lee <baul.lee@xbow.com>
> ---
>  net/bluetooth/sco.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index c05f79b7aa31..3db6552de06c 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -276,6 +276,9 @@ static void sco_conn_del(struct hci_conn *hcon, int err)
>  	sco_chan_del(sk, err);
>  	release_sock(sk);
>  	sock_put(sk);
> +
> +	/* Drop the association reference, as the !sk branch above does */
> +	sco_conn_put(conn);
>  }
>  
>  static void __sco_chan_add(struct sco_conn *conn, struct sock *sk,
> @@ -296,10 +299,16 @@ static int sco_chan_add(struct sco_conn *conn, struct sock *sk,
>  	int err = 0;
>  
>  	sco_conn_lock(conn);
> -	if (conn->sk || sco_pi(sk)->conn)
> +	if (conn->sk || sco_pi(sk)->conn) {
>  		err = -EBUSY;
> -	else
> +	} else {
> +		/* Take the socket reference, which sco_chan_del() drops when
> +		 * the socket detaches.  Without it the socket and the hcon
> +		 * would share the single reference from sco_conn_add().
> +		 */
> +		sco_conn_hold(conn);
>  		__sco_chan_add(conn, sk, parent);
> +	}
>  
>  	sco_conn_unlock(conn);
>  	return err;
> @@ -1452,6 +1461,13 @@ 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);
>  
> +		/* Two references are needed here: the socket one, dropped by
> +		 * sco_chan_del(), and the association one, dropped by
> +		 * sco_conn_del().  Unlike the outgoing path, the reference
> +		 * from sco_conn_add() cannot serve as the latter, because
> +		 * sco_connect_cfm() puts it as soon as this function returns.
> +		 */
> +		sco_conn_hold(conn);
>  		sco_conn_hold(conn);

I think it would be stylistically better to have the holds where they
are assigned to the fields that own the references, and the puts where
the variable is cleared.

>  		hci_conn_hold(conn->hcon);
>  		__sco_chan_add(conn, sk, parent);

-- 
Pauli Virtanen

Re: [PATCH] Bluetooth: SCO: fix sco_conn double free on outgoing connect

[Aldo Ariel Panzardo]

Hi Pauli, Baul, Federico,

Thanks for looking at both patches side by side.

To connect the threads: the "only socket owns sco_conn" approach you
pointed to is my v3

  https://lore.kernel.org/linux-bluetooth/20260725195230.967546-1-qwe.aldo@gmail.com/

which is on the list and passed sco-tester in CI. I found and
root-caused this independently and posted the first public fix on
2026-07-23 (v1), with a /dev/vhci KASAN reproducer that races close() of
an SCO socket against an injected Disconnection Complete. I see from
Baul's patch that XBOW reported the same issue privately on 2026-07-10 --
I'm happy for that earlier report to be credited (Reported-by, or however
you and they prefer); I don't want to step on it.

On the substance I agree the sco_data access needs serializing, and this
is the residual UAF I already flagged when I sent v3: with the over-put
fixed, sco_recv_scodata() still reads hcon->sco_data under hci_dev_lock
and sco_conn_hold_unless_zero()s it, but the clear in sco_conn_free() is
not under that lock, so the read can land on an already-freed sco_conn:

    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

I'll send the serialization as a follow-up on top of v3, along the lines
you sketched: clear hcon->sco_data in sco_conn_del() under hdev->lock so
the field can carry a __guarded_by(&hdev->lock) annotation, and -- to
avoid the "SCO Disconnect - Success" regression you spotted in the other
patch -- give the socket its own hci_conn reference that it drops on
close(), so closing still tears the link down while the sco_conn keeps a
single association reference cleared under the lock in sco_conn_del().
I'll only post it once the reproducer is clean under KASAN and sco-tester
passes.

Thanks,
Aldo