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