Bluetooth: ISO: hold sk properly in iso_conn_ready

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

    [Task 1]            [Task 2]
                        iso_sock_release
    iso_conn_ready
      sk = conn->sk
                          lock_sock(sk)
                            conn->sk = NULL
      lock_sock(sk)
                          release_sock(sk)
                          iso_sock_kill(sk)
       UAF on sk deref

Fix possible UAF by holding sk refcount in iso_conn_ready().  Also
recheck after lock_sock that the socket is still valid.  Adjust locking
so conn->sk is cleared only under lock_sock.

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/iso.c b/net/bluetooth/iso.c
index 5de4a2f886ebc97465c91725ab4ccf46a70238cb..80a58275891d5144337e26ce3321e1aa73711d62 100644 (file)
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -805,11 +805,13 @@ static void iso_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 (iso_pi(sk)->conn) {
                iso_conn_lock(iso_pi(sk)->conn);
                iso_pi(sk)->conn->sk = NULL;
                iso_conn_unlock(iso_pi(sk)->conn);
        }
+       release_sock(sk);
 
        /* Kill poor orphan */
        bt_sock_unlink(&iso_sk_list, sk);
@@ -2047,23 +2049,17 @@ static void iso_sock_ready(struct sock *sk)
 {
        BT_DBG("sk %p", sk);
 
-       if (!sk)
-               return;
-
-       lock_sock(sk);
+       lockdep_assert(lockdep_sock_is_held(sk));
 
        switch (sk->sk_state) {
        case BT_DISCONN:
        case BT_CLOSED:
-               release_sock(sk);
                return;
        }
 
        iso_sock_clear_timer(sk);
        sk->sk_state = BT_CONNECTED;
        sk->sk_state_change(sk);
-
-       release_sock(sk);
 }
 
 static bool iso_match_big(struct sock *sk, void *data)
@@ -2093,7 +2089,7 @@ static bool iso_match_dst(struct sock *sk, void *data)
 static void iso_conn_ready(struct iso_conn *conn)
 {
        struct sock *parent = NULL;
-       struct sock *sk = conn->sk;
+       struct sock *sk;
        struct hci_ev_le_big_sync_established *ev = NULL;
        struct hci_ev_le_pa_sync_established *ev2 = NULL;
        struct hci_ev_le_per_adv_report *ev3 = NULL;
@@ -2102,7 +2098,22 @@ static void iso_conn_ready(struct iso_conn *conn)
 
        BT_DBG("conn %p", conn);
 
+       iso_conn_lock(conn);
+       sk = iso_sock_hold(conn);
+       iso_conn_unlock(conn);
+
        if (sk) {
+               lock_sock(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) {
+                       release_sock(sk);
+                       sock_put(sk);
+                       return;
+               }
+
                /* Attempt to update source address in case of BIS Sender if
                 * the advertisement is using a random address.
                 */
@@ -2115,14 +2126,15 @@ static void iso_conn_ready(struct iso_conn *conn)
                        adv = hci_find_adv_instance(bis->hdev,
                                                    bis->iso_qos.bcast.bis);
                        if (adv && bacmp(&adv->random_addr, BDADDR_ANY)) {
-                               lock_sock(sk);
                                iso_pi(sk)->src_type = BDADDR_LE_RANDOM;
                                bacpy(&iso_pi(sk)->src, &adv->random_addr);
-                               release_sock(sk);
                        }
                }
 
-               iso_sock_ready(conn->sk);
+               iso_sock_ready(sk);
+
+               release_sock(sk);
+               sock_put(sk);
        } else {
                hcon = conn->hcon;
                if (!hcon)