[PATCH] Bluetooth: SCO: fix race conditions in sco_sock_connect()

[PATCH] Bluetooth: SCO: fix race conditions in sco_sock_connect()

[Cen Zhang]

sco_sock_connect() checks sk_state and sk_type without holding the
socket lock, creating a TOCTOU race where two threads can both pass the
state check and proceed to sco_connect() concurrently.

This causes multiple issues:
1. Both threads create separate HCI and SCO connections
2. The second thread's sco_chan_add() overwrites sco_pi(sk)->conn,
   orphaning the first connection (memory/resource leak)
3. If the socket is closed between the state check and sco_connect()'s
   inner lock_sock(), the connect proceeds on a dead socket (UAF)

Fix this by:
- Moving lock_sock() before the sk_state/sk_type checks in
  sco_sock_connect() to serialize concurrent connect attempts
- Fixing the sk_type != SOCK_SEQPACKET check to actually return the
  error instead of just assigning it
- Adding a state re-check in sco_connect() after lock_sock() to detect
  state changes during the lock gap
- Adding sco_pi(sk)->conn check in sco_chan_add() to prevent
  double-attach of the socket to multiple connections
- Adding hci_conn_drop() on sco_chan_add failure to prevent HCI
  connection leaks

Fixes: 9a8ec9e8ebb5 ("Bluetooth: SCO: Fix possible circular locking dependency on sco_connect_cfm")
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
 net/bluetooth/sco.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index e7db50165879..689788ad26a4 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -298,7 +298,7 @@ static int sco_chan_add(struct sco_conn *conn, struct sock *sk,
 	int err = 0;
 
 	sco_conn_lock(conn);
-	if (conn->sk)
+	if (conn->sk || sco_pi(sk)->conn)
 		err = -EBUSY;
 	else
 		__sco_chan_add(conn, sk, parent);
@@ -353,9 +353,20 @@ static int sco_connect(struct sock *sk)
 
 	lock_sock(sk);
 
+	/* Recheck state after reacquiring the socket lock, as another
+	 * thread may have changed it (e.g., closed the socket).
+	 */
+	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
+		release_sock(sk);
+		hci_conn_drop(hcon);
+		err = -EBADFD;
+		goto unlock;
+	}
+
 	err = sco_chan_add(conn, sk, NULL);
 	if (err) {
 		release_sock(sk);
+		hci_conn_drop(hcon);
 		goto unlock;
 	}
 
@@ -652,13 +663,18 @@ static int sco_sock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 	    addr->sa_family != AF_BLUETOOTH)
 		return -EINVAL;
 
-	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND)
+	lock_sock(sk);
+
+	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
+		release_sock(sk);
 		return -EBADFD;
+	}
 
-	if (sk->sk_type != SOCK_SEQPACKET)
-		err = -EINVAL;
+	if (sk->sk_type != SOCK_SEQPACKET) {
+		release_sock(sk);
+		return -EINVAL;
+	}
 
-	lock_sock(sk);
 	/* Set destination address and psm */
 	bacpy(&sco_pi(sk)->dst, &sa->sco_bdaddr);
 	release_sock(sk);
-- 
2.34.1

RE: Bluetooth: SCO: fix race conditions in sco_sock_connect()

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PENDING   0.33 seconds
GitLint                       PENDING   0.27 seconds
SubjectPrefix                 PASS      0.12 seconds
BuildKernel                   PASS      26.65 seconds
CheckAllWarning               PASS      28.94 seconds
CheckSparse                   PASS      27.81 seconds
BuildKernel32                 PASS      25.66 seconds
TestRunnerSetup               PASS      571.81 seconds
TestRunner_l2cap-tester       PASS      28.22 seconds
TestRunner_iso-tester         FAIL      34.43 seconds
TestRunner_bnep-tester        PASS      8.59 seconds
TestRunner_mgmt-tester        FAIL      114.33 seconds
TestRunner_rfcomm-tester      PASS      9.52 seconds
TestRunner_sco-tester         FAIL      14.46 seconds
TestRunner_ioctl-tester       PASS      10.28 seconds
TestRunner_mesh-tester        FAIL      12.49 seconds
TestRunner_smp-tester         PASS      8.67 seconds
TestRunner_userchan-tester    PASS      6.81 seconds
IncrementalBuild              PENDING   1.07 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
BUG: KASAN: slab-use-after-free in le_read_features_complete+0x7e/0x2b0
Total: 141, Passed: 141 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 494, Passed: 489 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.105 seconds
##############################
Test: TestRunner_sco-tester - FAIL
Desc: Run sco-tester with test-runner
Output:
WARNING: possible circular locking dependency detected
BUG: sleeping function called from invalid context at net/core/sock.c:3782
Total: 30, Passed: 30 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.733 seconds
Mesh - Send cancel - 2                               Timed out    1.995 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH] Bluetooth: SCO: fix race conditions in sco_sock_connect()

[Luiz Augusto von Dentz]

Hi Cen,

On Thu, Mar 26, 2026 at 8:49 AM Cen Zhang <zzzccc427@gmail.com> wrote:
>
> sco_sock_connect() checks sk_state and sk_type without holding the
> socket lock, creating a TOCTOU race where two threads can both pass the
> state check and proceed to sco_connect() concurrently.
>
> This causes multiple issues:
> 1. Both threads create separate HCI and SCO connections

Ok, the first and the second points don't seem to agree. If each
thread creates its own socket then the sk wouldn't be the same would
it? Or is the first point about two concurrent connect syscalls to the
same fd? In which case that should have been made clearer.

> 2. The second thread's sco_chan_add() overwrites sco_pi(sk)->conn,
>    orphaning the first connection (memory/resource leak)
> 3. If the socket is closed between the state check and sco_connect()'s
>    inner lock_sock(), the connect proceeds on a dead socket (UAF)
>
> Fix this by:
> - Moving lock_sock() before the sk_state/sk_type checks in
>   sco_sock_connect() to serialize concurrent connect attempts
> - Fixing the sk_type != SOCK_SEQPACKET check to actually return the
>   error instead of just assigning it
> - Adding a state re-check in sco_connect() after lock_sock() to detect
>   state changes during the lock gap
> - Adding sco_pi(sk)->conn check in sco_chan_add() to prevent
>   double-attach of the socket to multiple connections
> - Adding hci_conn_drop() on sco_chan_add failure to prevent HCI
>   connection leaks
>
> Fixes: 9a8ec9e8ebb5 ("Bluetooth: SCO: Fix possible circular locking dependency on sco_connect_cfm")
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
>  net/bluetooth/sco.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index e7db50165879..689788ad26a4 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -298,7 +298,7 @@ static int sco_chan_add(struct sco_conn *conn, struct sock *sk,
>         int err = 0;
>
>         sco_conn_lock(conn);
> -       if (conn->sk)
> +       if (conn->sk || sco_pi(sk)->conn)
>                 err = -EBUSY;
>         else
>                 __sco_chan_add(conn, sk, parent);
> @@ -353,9 +353,20 @@ static int sco_connect(struct sock *sk)
>
>         lock_sock(sk);
>
> +       /* Recheck state after reacquiring the socket lock, as another
> +        * thread may have changed it (e.g., closed the socket).
> +        */
> +       if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
> +               release_sock(sk);
> +               hci_conn_drop(hcon);
> +               err = -EBADFD;
> +               goto unlock;
> +       }
> +
>         err = sco_chan_add(conn, sk, NULL);
>         if (err) {
>                 release_sock(sk);
> +               hci_conn_drop(hcon);
>                 goto unlock;
>         }
>
> @@ -652,13 +663,18 @@ static int sco_sock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>             addr->sa_family != AF_BLUETOOTH)
>                 return -EINVAL;
>
> -       if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND)
> +       lock_sock(sk);
> +
> +       if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
> +               release_sock(sk);
>                 return -EBADFD;
> +       }
>
> -       if (sk->sk_type != SOCK_SEQPACKET)
> -               err = -EINVAL;
> +       if (sk->sk_type != SOCK_SEQPACKET) {
> +               release_sock(sk);
> +               return -EINVAL;
> +       }
>
> -       lock_sock(sk);
>         /* Set destination address and psm */
>         bacpy(&sco_pi(sk)->dst, &sa->sco_bdaddr);
>         release_sock(sk);
> --
> 2.34.1
>


-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: SCO: fix race conditions in sco_sock_connect()

[Cen Zhang]

Hi Luiz,

> Ok, the first and the second points don't seem to agree. If each
> thread creates its own socket then the sk wouldn't be the same would
> it? Or is the first point about two concurrent connect syscalls to the
> same fd? In which case that should have been made clearer.
>

Thank you for pointing this out. You are right -- the original
description was not clear enough. This is indeed about two concurrent
connect() syscalls on the same socket fd.

I have rewritten the commit message with a detailed timing diagram
that reflects the actual scenario I confirmed with logging
instrumentation. The race requires three participants:

1. Thread A calls connect() and completes sco_connect() under
   hci_dev_lock, setting sk_state = BT_CONNECT.

2. Thread B also called connect() on the same fd and passed the
   sk_state check (without lock_sock), but is blocked on
   hci_dev_lock inside sco_connect().

3. While Thread B is blocked, Thread A's HCI connection
   disconnects. sco_conn_del() runs under hci_dev_lock,
   clears the socket (sk_state = BT_CLOSED, SOCK_ZAPPED),
   and releases hci_dev_lock.

4. Thread B unblocks, creates a new HCI connection and
   sco_conn, and successfully attaches it to the already-closed
   socket -- reviving a zombie socket from BT_CLOSED back to
   BT_CONNECT, leading to use-after-free on subsequent cleanup.

Best regards,
Cen