Bluetooth: SCO: hold sk properly in sco_conn_ready

sk deref in sco_conn_ready must be done either under conn->lock, or
holding a refcount, to avoid concurrent close. conn->sk and parent sk is
currently accessed without either, and without checking parent->sk_state:

    [Task 1]            [Task 2]
                        sco_sock_release
    sco_conn_ready
      sk = conn->sk
                          lock_sock(sk)
                            conn->sk = NULL
      lock_sock(sk)
                          release_sock(sk)
                          sco_sock_kill(sk)
       UAF on sk deref

and similarly for access to sco_get_sock_listen() return value.

Fix possible UAF by holding sk refcount in sco_conn_ready() and making
sco_get_sock_listen() increase refcount. Also recheck after lock_sock
that the socket is still valid.  Adjust conn->sk locking so it's
protected also by lock_sock() of the associated socket if any.

Fixes: 27c24fda62b60 ("Bluetooth: switch to lock_sock in SCO")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
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 3a5479538e85878c8a157e59c690715676bf7dc7..eba44525d41d9880fb76773540dd1a990f3f167f 100644 (file)
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -472,9 +472,13 @@ static struct sock *sco_get_sock_listen(bdaddr_t *src)
                        sk1 = sk;
        }
 
+       sk = sk ? sk : sk1;
+       if (sk)
+               sock_hold(sk);
+
        read_unlock(&sco_sk_list.lock);
 
-       return sk ? sk : sk1;
+       return sk;
 }
 
 static void sco_sock_destruct(struct sock *sk)
@@ -515,11 +519,13 @@ static void sco_sock_kill(struct sock *sk)
        BT_DBG("sk %p state %d", sk, sk->sk_state);
 
        /* Sock is dead, so set conn->sk to NULL to avoid possible UAF */
+       lock_sock(sk);
        if (sco_pi(sk)->conn) {
                sco_conn_lock(sco_pi(sk)->conn);
                sco_pi(sk)->conn->sk = NULL;
                sco_conn_unlock(sco_pi(sk)->conn);
        }
+       release_sock(sk);
 
        /* Kill poor orphan */
        bt_sock_unlink(&sco_sk_list, sk);
@@ -1365,17 +1371,28 @@ static int sco_sock_release(struct socket *sock)
 
 static void sco_conn_ready(struct sco_conn *conn)
 {
-       struct sock *parent;
-       struct sock *sk = conn->sk;
+       struct sock *parent, *sk;
+
+       sco_conn_lock(conn);
+       sk = sco_sock_hold(conn);
+       sco_conn_unlock(conn);
 
        BT_DBG("conn %p", conn);
 
        if (sk) {
                lock_sock(sk);
-               sco_sock_clear_timer(sk);
-               sk->sk_state = BT_CONNECTED;
-               sk->sk_state_change(sk);
+
+               /* conn->sk may have become NULL if racing with sk close, but
+                * due to held hdev->lock, it can't become different sk.
+                */
+               if (conn->sk) {
+                       sco_sock_clear_timer(sk);
+                       sk->sk_state = BT_CONNECTED;
+                       sk->sk_state_change(sk);
+               }
+
                release_sock(sk);
+               sock_put(sk);
        } else {
                if (!conn->hcon)
                        return;
@@ -1390,13 +1407,15 @@ static void sco_conn_ready(struct sco_conn *conn)
 
                sco_conn_lock(conn);
 
+               /* hdev->lock guarantees conn->sk == NULL still here */
+
+               if (parent->sk_state != BT_LISTEN)
+                       goto release;
+
                sk = sco_sock_alloc(sock_net(parent), NULL,
                                    BTPROTO_SCO, GFP_ATOMIC, 0);
-               if (!sk) {
-                       sco_conn_unlock(conn);
-                       release_sock(parent);
-                       return;
-               }
+               if (!sk)
+                       goto release;
 
                sco_sock_init(sk, parent);
 
@@ -1415,9 +1434,10 @@ static void sco_conn_ready(struct sco_conn *conn)
                /* Wake up parent */
                parent->sk_data_ready(parent);
 
+release:
                sco_conn_unlock(conn);
-
                release_sock(parent);
+               sock_put(parent);
        }
 }