Bluetooth: ISO: avoid deadlocks in iso_sock_timeout

iso_sock_timeout() takes lock_sock, so sync disabling the timer while
holding that lock may deadlock.

iso_sock_timeout() may also run concurrently with iso_conn_del(), which
leads to UAF

        [Task 1]                      [Task hdev->workqueue]
        iso_sock_timeout              iso_conn_del
          iso_conn_hold_unless_zero     iso_chan_del
                           `------------> iso_conn_put
                                      caller frees hcon
          iso_conn_put
            iso_conn_free
              conn->hcon->iso_data = NULL; /* UAF */

Fix the deadlock by removing the disable from the lock_sock sections.
Move the timer from iso_conn to iso_pinfo to decouple it from iso_conn
which may need to be freed in lock_sock section. Convert some of the
clear_timer to disable_timer.

Fixes: dc26097bdb86 ("Bluetooth: ISO: Use kref to track lifetime of iso_conn")
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 5f0f45a573a7669019e8460b3728030633cf61dd..0cc08416fde5635383c563e085abeb37ca422770 100644 (file)
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -37,8 +37,6 @@ struct iso_conn {
        spinlock_t      lock;
        struct sock     *sk;
 
-       struct delayed_work     timeout_work;
-
        struct sk_buff  *rx_skb;
        __u32           rx_len;
        __u16           tx_sn;
@@ -81,6 +79,7 @@ struct iso_pinfo {
        __u8                    base_len;
        __u8                    base[BASE_MAX_LENGTH];
        struct iso_conn         *conn;
+       struct delayed_work     timeout_work;
 };
 
 static struct bt_iso_qos default_qos;
@@ -118,9 +117,6 @@ static void iso_conn_free(struct kref *ref)
                        hci_conn_drop(conn->hcon);
        }
 
-       /* Ensure no more work items will run since hci_conn has been dropped */
-       disable_delayed_work_sync(&conn->timeout_work);
-
        kfree_skb(conn->rx_skb);
 
        kfree(conn);
@@ -161,48 +157,45 @@ static struct sock *iso_sock_hold(struct iso_conn *conn)
 
 static void iso_sock_timeout(struct work_struct *work)
 {
-       struct iso_conn *conn = container_of(work, struct iso_conn,
-                                            timeout_work.work);
-       struct sock *sk;
-
-       conn = iso_conn_hold_unless_zero(conn);
-       if (!conn)
-               return;
-
-       iso_conn_lock(conn);
-       sk = iso_sock_hold(conn);
-       iso_conn_unlock(conn);
-       iso_conn_put(conn);
-
-       if (!sk)
-               return;
+       struct iso_pinfo *pi = container_of(work, struct iso_pinfo,
+                                           timeout_work.work);
+       struct sock *sk = &pi->bt.sk;
 
        BT_DBG("sock %p state %d", sk, sk->sk_state);
 
        lock_sock(sk);
-       sk->sk_err = ETIMEDOUT;
-       sk->sk_state_change(sk);
+       if (!sock_flag(sk, SOCK_ZAPPED)) {
+               sk->sk_err = ETIMEDOUT;
+               sk->sk_state_change(sk);
+       }
        release_sock(sk);
-       sock_put(sk);
 }
 
 static void iso_sock_set_timer(struct sock *sk, long timeout)
 {
+       lockdep_assert(lockdep_sock_is_held(sk));
+
+       cancel_delayed_work(&iso_pi(sk)->timeout_work);
+
        if (!iso_pi(sk)->conn)
                return;
 
        BT_DBG("sock %p state %d timeout %ld", sk, sk->sk_state, timeout);
-       cancel_delayed_work(&iso_pi(sk)->conn->timeout_work);
-       schedule_delayed_work(&iso_pi(sk)->conn->timeout_work, timeout);
+       schedule_delayed_work(&iso_pi(sk)->timeout_work, timeout);
 }
 
 static void iso_sock_clear_timer(struct sock *sk)
 {
-       if (!iso_pi(sk)->conn)
-               return;
+       BT_DBG("sock %p state %d", sk, sk->sk_state);
+       cancel_delayed_work(&iso_pi(sk)->timeout_work);
+}
+
+static void iso_sock_disable_timer(struct sock *sk)
+{
+       lockdep_assert(!lockdep_sock_is_held(sk));
 
        BT_DBG("sock %p state %d", sk, sk->sk_state);
-       cancel_delayed_work(&iso_pi(sk)->conn->timeout_work);
+       disable_delayed_work_sync(&iso_pi(sk)->timeout_work);
 }
 
 /* ---- ISO connections ---- */
@@ -227,7 +220,6 @@ static struct iso_conn *iso_conn_add(struct hci_conn *hcon)
 
        kref_init(&conn->ref);
        spin_lock_init(&conn->lock);
-       INIT_DELAYED_WORK(&conn->timeout_work, iso_sock_timeout);
 
        hcon->iso_data = conn;
        conn->hcon = hcon;
@@ -292,8 +284,9 @@ static void iso_conn_del(struct hci_conn *hcon, int err)
                return;
        }
 
+       iso_sock_disable_timer(sk);
+
        lock_sock(sk);
-       iso_sock_clear_timer(sk);
        iso_chan_del(sk, err);
        release_sock(sk);
        iso_sock_kill(sk);
@@ -800,6 +793,8 @@ static void iso_sock_cleanup_listen(struct sock *parent)
  */
 static void iso_sock_kill(struct sock *sk)
 {
+       iso_sock_disable_timer(sk);
+
        lock_sock(sk);
 
        if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket ||
@@ -895,8 +890,9 @@ static void __iso_sock_close(struct sock *sk)
 /* Must be called on unlocked socket. */
 static void iso_sock_close(struct sock *sk)
 {
+       iso_sock_disable_timer(sk);
+
        lock_sock(sk);
-       iso_sock_clear_timer(sk);
        __iso_sock_close(sk);
        release_sock(sk);
 }
@@ -965,6 +961,8 @@ static struct sock *iso_sock_alloc(struct net *net, struct socket *sock,
        iso_pi(sk)->qos = default_qos;
        iso_pi(sk)->sync_handle = -1;
 
+       INIT_DELAYED_WORK(&iso_pi(sk)->timeout_work, iso_sock_timeout);
+
        bt_sock_link(&iso_sk_list, sk);
        return sk;
 }