Bluetooth: ISO: fix race of kfree vs kref_get_unless_zero

hci_conn::iso_data is accessed and modified without lock or RCU.
This leads to a race

    [Task hdev->workqueue]                 	[Task 2]
    iso_recv                                    iso_conn_put(conn)
      conn = LOAD hcon->iso_data                  iso_conn_free(conn)
      iso_conn_hold_unless_zero(conn)               hcon->iso_data = NULL
                                                    kfree(conn)
        kref_get_unless_zero(&conn->ref) /* UAF */

and also to races in iso_conn_add() vs. iso_conn_free().

Fix by adding spinlock hci_conn::proto_lock and using it to guard
hci_conn::iso_data.

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/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e7133ff87fbfa81b836fcff87cb64fae601fe0da..3df59849dcbeaa277fe3e6bdd15dd072c78e10f2 100644 (file)
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -767,9 +767,11 @@ struct hci_conn {
        struct dentry   *debugfs;
 
        struct hci_dev  *hdev;
+
+       spinlock_t      proto_lock; /* lock guarding protocol data */
        void            *l2cap_data;
        void            *sco_data;
-       void            *iso_data;
+       void            *iso_data __guarded_by(&proto_lock);
 
        struct list_head link_list;
        struct hci_conn *parent;

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 1966cd153d9730dda12c1ce64cdd930bb50dcee0..ebb04badf10c0ca40f1669cc2d5dd45d75fb2c7d 100644 (file)
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1123,6 +1123,8 @@ static struct hci_conn *__hci_conn_add(struct hci_dev *hdev, int type,
        INIT_DELAYED_WORK(&conn->idle_work, hci_conn_idle);
        INIT_DELAYED_WORK(&conn->le_conn_timeout, le_conn_timeout);
 
+       spin_lock_init(&conn->proto_lock);
+
        atomic_set(&conn->refcnt, 0);
 
        hci_dev_hold(hdev);

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 30de99ba4b30ef20cc6869bb50239b5b5b02e98a..a461c8a4efed400e787ce0f1bf2037a073b70f2b 100644 (file)
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -109,9 +109,16 @@ static void iso_conn_free(struct kref *ref)
        BT_DBG("conn %p", conn);
 
        if (conn->hcon) {
-               conn->hcon->iso_data = NULL;
-               if (!test_and_set_bit(ISO_CONN_DROPPED, conn->flags))
-                       hci_conn_drop(conn->hcon);
+               spin_lock(&conn->hcon->proto_lock);
+
+               /* Check we are not racing with iso_conn_add */
+               if (conn->hcon->iso_data == conn) {
+                       conn->hcon->iso_data = NULL;
+                       if (!test_and_set_bit(ISO_CONN_DROPPED, conn->flags))
+                               hci_conn_drop(conn->hcon);
+               }
+
+               spin_unlock(&conn->hcon->proto_lock);
        }
 
        kfree_skb(conn->rx_skb);
@@ -126,7 +133,21 @@ static void iso_conn_put(struct iso_conn *conn)
 
        BT_DBG("conn %p refcnt %d", conn, kref_read(&conn->ref));
 
+       /* The following race vs. iso_conn_del() is possible:
+        *
+        * 1. conn->hcon != NULL here
+        * 2. kref_put puts the last reference
+        * 3. concurrent iso_conn_del() gets iso_conn_hold_unless_zero() -> NULL
+        *    and returns immediately, so conn->hcon is not cleared
+        * 4. iso_conn_free() dereferences conn->hcon
+        *
+        * To avoid UAF in step 4, take RCU before decrementing the refcount.
+        */
+       rcu_read_lock();
+
        kref_put(&conn->ref, iso_conn_free);
+
+       rcu_read_unlock();
 }
 
 static struct iso_conn *iso_conn_hold_unless_zero(struct iso_conn *conn)
@@ -205,22 +226,28 @@ static void iso_sock_disable_timer(struct sock *sk)
 
 /* ---- ISO connections ---- */
 static struct iso_conn *iso_conn_add(struct hci_conn *hcon)
+       __must_hold(&hcon->hdev->lock)
 {
-       struct iso_conn *conn = hcon->iso_data;
+       struct iso_conn *conn;
+
+       spin_lock(&hcon->proto_lock);
 
-       conn = iso_conn_hold_unless_zero(conn);
+       conn = iso_conn_hold_unless_zero(hcon->iso_data);
        if (conn) {
                if (!conn->hcon) {
                        iso_conn_lock(conn);
                        conn->hcon = hcon;
                        iso_conn_unlock(conn);
                }
+               spin_unlock(&hcon->proto_lock);
                return conn;
        }
 
-       conn = kzalloc_obj(*conn);
-       if (!conn)
+       conn = kzalloc_obj(*conn, GFP_ATOMIC);
+       if (!conn) {
+               spin_unlock(&hcon->proto_lock);
                return NULL;
+       }
 
        kref_init(&conn->ref);
        spin_lock_init(&conn->lock);
@@ -229,6 +256,8 @@ static struct iso_conn *iso_conn_add(struct hci_conn *hcon)
        conn->hcon = hcon;
        conn->tx_sn = 0;
 
+       spin_unlock(&hcon->proto_lock);
+
        BT_DBG("hcon %p conn %p", hcon, conn);
 
        return conn;
@@ -269,10 +298,12 @@ static void iso_chan_del(struct sock *sk, int err)
 static void iso_conn_del(struct hci_conn *hcon, int err)
        __must_hold(&hcon->hdev->lock)
 {
-       struct iso_conn *conn = hcon->iso_data;
+       struct iso_conn *conn;
        struct sock *sk;
 
-       conn = iso_conn_hold_unless_zero(conn);
+       spin_lock(&hcon->proto_lock);
+       conn = iso_conn_hold_unless_zero(hcon->iso_data);
+       spin_unlock(&hcon->proto_lock);
        if (!conn)
                return;
 
@@ -296,10 +327,12 @@ static void iso_conn_del(struct hci_conn *hcon, int err)
 
 done:
        /* No sk access to conn->hcon any more (lock_sock + hdev->lock) */
+       spin_lock(&hcon->proto_lock);
        iso_conn_lock(conn);
        conn->hcon = NULL;
        hcon->iso_data = NULL;
        iso_conn_unlock(conn);
+       spin_unlock(&hcon->proto_lock);
 
        iso_conn_put(conn);
 }
@@ -421,6 +454,8 @@ static int iso_connect_bis(struct sock *sk)
                        iso_pi(sk)->bc_sid = hcon->sid;
        }
 
+       lockdep_assert_held(&hcon->hdev->lock);
+
        conn = iso_conn_add(hcon);
        if (!conn) {
                hci_conn_drop(hcon);
@@ -524,6 +559,8 @@ static int iso_connect_cis(struct sock *sk)
                }
        }
 
+       lockdep_assert_held(&hcon->hdev->lock);
+
        conn = iso_conn_add(hcon);
        if (!conn) {
                hci_conn_drop(hcon);
@@ -855,8 +892,8 @@ static void iso_sock_disconn(struct sock *sk)
                 */
                if (bis_sk) {
                        hcon->state = BT_OPEN;
-                       hcon->iso_data = NULL;
-                       iso_pi(sk)->conn->hcon = NULL;
+                       set_bit(ISO_CONN_DROPPED, iso_pi(sk)->conn->flags);
+
                        iso_sock_clear_timer(sk);
                        iso_chan_del(sk, bt_to_errno(hcon->abort_reason));
                        sock_put(bis_sk);
@@ -1306,6 +1343,8 @@ static int iso_listen_bis(struct sock *sk)
                goto unlock;
        }
 
+       lockdep_assert_held(&hcon->hdev->lock);
+
        conn = iso_conn_add(hcon);
        if (!conn) {
                hci_conn_drop(hcon);
@@ -2591,7 +2630,10 @@ int iso_recv(struct hci_dev *hdev, u16 handle, struct sk_buff *skb, u16 flags)
                return -ENOENT;
        }
 
+       spin_lock(&hcon->proto_lock);
        conn = iso_conn_hold_unless_zero(hcon->iso_data);
+       spin_unlock(&hcon->proto_lock);
+
        hcon = NULL;
 
        hci_dev_unlock(hdev);