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

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

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