Bluetooth: SCO: fix race conditions in sco_sock_connect()

sco_sock_connect() checks sk_state and sk_type without holding
the socket lock. Two concurrent connect() syscalls on the same
socket can both pass the check and enter sco_connect(), leading
to use-after-free.

The buggy scenario involves three participants and was confirmed
with additional logging instrumentation:

  Thread A (connect):    HCI disconnect:      Thread B (connect):

  sco_sock_connect(sk)                        sco_sock_connect(sk)
  sk_state==BT_OPEN                           sk_state==BT_OPEN
  (pass, no lock)                             (pass, no lock)
  sco_connect(sk):                            sco_connect(sk):
    hci_dev_lock                                hci_dev_lock
    hci_connect_sco                               <- blocked
      -> hcon1
    sco_conn_add->conn1
    lock_sock(sk)
    sco_chan_add:
      conn1->sk = sk
      sk->conn = conn1
    sk_state=BT_CONNECT
    release_sock
    hci_dev_unlock
                           hci_dev_lock
                           sco_conn_del:
                             lock_sock(sk)
                             sco_chan_del:
                               sk->conn=NULL
                               conn1->sk=NULL
                               sk_state=
                                 BT_CLOSED
                               SOCK_ZAPPED
                             release_sock
                           hci_dev_unlock
                                                  (unblocked)
                                                  hci_connect_sco
                                                    -> hcon2
                                                  sco_conn_add
                                                    -> conn2
                                                  lock_sock(sk)
                                                  sco_chan_add:
                                                    sk->conn=conn2
                                                  sk_state=
                                                    BT_CONNECT
                                                  // zombie sk!
                                                  release_sock
                                                  hci_dev_unlock

Thread B revives a BT_CLOSED + SOCK_ZAPPED socket back to
BT_CONNECT. Subsequent cleanup triggers double sock_put() and
use-after-free. Meanwhile conn1 is leaked as it was orphaned
when sco_conn_del() cleared the association.

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 catch state changes during the window between the locks
- Adding sco_pi(sk)->conn check in sco_chan_add() to prevent
  double-attach of a 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>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 584e059de20a660e6ea5704466bf8df3dfdc6cec..b84587811ef4f83accf2c21e9e275e57543cbf05 100644 (file)
--- 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;
        }
 
@@ -656,13 +667,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);