Bluetooth: hci_core: lookup hci_conn on RX path on protocol side

The hdev lock/lookup/unlock/use pattern in the packet RX path doesn't
ensure hci_conn* is not concurrently modified/deleted. This locking
appears to be leftover from before conn_hash started using RCU
commit bf4c63252490b ("Bluetooth: convert conn hash to RCU")
and not clear if it had purpose since then.

Currently, there are code paths that delete hci_conn* from elsewhere
than the ordered hdev->workqueue where the RX work runs in. E.g.
commit 5af1f84ed13a ("Bluetooth: hci_sync: Fix UAF on hci_abort_conn_sync")
introduced some of these, and there probably were a few others before
it.  It's better to do the locking so that even if these run
concurrently no UAF is possible.

Move the lookup of hci_conn and associated socket-specific conn to
protocol recv handlers, and do them within a single critical section
to cover hci_conn* usage and lookup.

syzkaller has reported a crash that appears to be this issue:

    [Task hdev->workqueue]          [Task 2]
                                    hci_disconnect_all_sync
    l2cap_recv_acldata(hcon)
                                      hci_conn_get(hcon)
                                      hci_abort_conn_sync(hcon)
                                        hci_dev_lock
      hci_dev_lock
                                        hci_conn_del(hcon)
      v-------------------------------- hci_dev_unlock
                                      hci_conn_put(hcon)
      conn = hcon->l2cap_data (UAF)

Fixes: 5af1f84ed13a ("Bluetooth: hci_sync: Fix UAF on hci_abort_conn_sync")
Reported-by: syzbot+d32d77220b92eddd89ad@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d32d77220b92eddd89ad
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 32b1c08c8bba8c70eff05b1740dc44c8babfc5d6..0cb87687837f7efef431d7e8999974762c6cf9f1 100644 (file)
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -856,11 +856,12 @@ extern struct mutex hci_cb_list_lock;
 /* ----- HCI interface to upper protocols ----- */
 int l2cap_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr);
 int l2cap_disconn_ind(struct hci_conn *hcon);
-void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags);
+int l2cap_recv_acldata(struct hci_dev *hdev, u16 handle, struct sk_buff *skb,
+                      u16 flags);
 
 #if IS_ENABLED(CONFIG_BT_BREDR)
 int sco_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags);
-void sco_recv_scodata(struct hci_conn *hcon, struct sk_buff *skb);
+int sco_recv_scodata(struct hci_dev *hdev, u16 handle, struct sk_buff *skb);
 #else
 static inline int sco_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr,
                                  __u8 *flags)
@@ -868,23 +869,30 @@ static inline int sco_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr,
        return 0;
 }
 
-static inline void sco_recv_scodata(struct hci_conn *hcon, struct sk_buff *skb)
+static inline int sco_recv_scodata(struct hci_dev *hdev, u16 handle,
+                                  struct sk_buff *skb)
 {
+       kfree_skb(skb);
+       return -ENOENT;
 }
 #endif
 
 #if IS_ENABLED(CONFIG_BT_LE)
 int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags);
-void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags);
+int iso_recv(struct hci_dev *hdev, u16 handle, struct sk_buff *skb,
+            u16 flags);
 #else
 static inline int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr,
                                  __u8 *flags)
 {
        return 0;
 }
-static inline void iso_recv(struct hci_conn *hcon, struct sk_buff *skb,
-                           u16 flags)
+
+static inline int iso_recv(struct hci_dev *hdev, u16 handle,
+                          struct sk_buff *skb, u16 flags)
 {
+       kfree_skb(skb);
+       return -ENOENT;
 }
 #endif
 

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 1920e3d62bdaa1ea4cc68c89688bcf32bb46f3c6..8ccec73dce45c2a90214fa035f5e357ff3b7e53e 100644 (file)
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3832,13 +3832,14 @@ static void hci_tx_work(struct work_struct *work)
 static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb)
 {
        struct hci_acl_hdr *hdr;
-       struct hci_conn *conn;
        __u16 handle, flags;
+       int err;
 
        hdr = skb_pull_data(skb, sizeof(*hdr));
        if (!hdr) {
                bt_dev_err(hdev, "ACL packet too small");
-               goto drop;
+               kfree_skb(skb);
+               return;
        }
 
        handle = __le16_to_cpu(hdr->handle);
@@ -3850,36 +3851,27 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb)
 
        hdev->stat.acl_rx++;
 
-       hci_dev_lock(hdev);
-       conn = hci_conn_hash_lookup_handle(hdev, handle);
-       hci_dev_unlock(hdev);
-
-       if (conn) {
-               hci_conn_enter_active_mode(conn, BT_POWER_FORCE_ACTIVE_OFF);
-
-               /* Send to upper protocol */
-               l2cap_recv_acldata(conn, skb, flags);
-               return;
-       } else {
+       err = l2cap_recv_acldata(hdev, handle, skb, flags);
+       if (err == -ENOENT)
                bt_dev_err(hdev, "ACL packet for unknown connection handle %d",
                           handle);
-       }
-
-drop:
-       kfree_skb(skb);
+       else if (err)
+               bt_dev_dbg(hdev, "ACL packet recv for handle %d failed: %d",
+                          handle, err);
 }
 
 /* SCO data packet */
 static void hci_scodata_packet(struct hci_dev *hdev, struct sk_buff *skb)
 {
        struct hci_sco_hdr *hdr;
-       struct hci_conn *conn;
        __u16 handle, flags;
+       int err;
 
        hdr = skb_pull_data(skb, sizeof(*hdr));
        if (!hdr) {
                bt_dev_err(hdev, "SCO packet too small");
-               goto drop;
+               kfree_skb(skb);
+               return;
        }
 
        handle = __le16_to_cpu(hdr->handle);
@@ -3891,34 +3883,28 @@ static void hci_scodata_packet(struct hci_dev *hdev, struct sk_buff *skb)
 
        hdev->stat.sco_rx++;
 
-       hci_dev_lock(hdev);
-       conn = hci_conn_hash_lookup_handle(hdev, handle);
-       hci_dev_unlock(hdev);
+       hci_skb_pkt_status(skb) = flags & 0x03;
 
-       if (conn) {
-               /* Send to upper protocol */
-               hci_skb_pkt_status(skb) = flags & 0x03;
-               sco_recv_scodata(conn, skb);
-               return;
-       } else {
+       err = sco_recv_scodata(hdev, handle, skb);
+       if (err == -ENOENT)
                bt_dev_err_ratelimited(hdev, "SCO packet for unknown connection handle %d",
                                       handle);
-       }
-
-drop:
-       kfree_skb(skb);
+       else if (err)
+               bt_dev_dbg(hdev, "SCO packet recv for handle %d failed: %d",
+                          handle, err);
 }
 
 static void hci_isodata_packet(struct hci_dev *hdev, struct sk_buff *skb)
 {
        struct hci_iso_hdr *hdr;
-       struct hci_conn *conn;
        __u16 handle, flags;
+       int err;
 
        hdr = skb_pull_data(skb, sizeof(*hdr));
        if (!hdr) {
                bt_dev_err(hdev, "ISO packet too small");
-               goto drop;
+               kfree_skb(skb);
+               return;
        }
 
        handle = __le16_to_cpu(hdr->handle);
@@ -3928,22 +3914,13 @@ static void hci_isodata_packet(struct hci_dev *hdev, struct sk_buff *skb)
        bt_dev_dbg(hdev, "len %d handle 0x%4.4x flags 0x%4.4x", skb->len,
                   handle, flags);
 
-       hci_dev_lock(hdev);
-       conn = hci_conn_hash_lookup_handle(hdev, handle);
-       hci_dev_unlock(hdev);
-
-       if (!conn) {
+       err = iso_recv(hdev, handle, skb, flags);
+       if (err == -ENOENT)
                bt_dev_err(hdev, "ISO packet for unknown connection handle %d",
                           handle);
-               goto drop;
-       }
-
-       /* Send to upper protocol */
-       iso_recv(conn, skb, flags);
-       return;
-
-drop:
-       kfree_skb(skb);
+       else if (err)
+               bt_dev_dbg(hdev, "ISO packet recv for handle %d failed: %d",
+                          handle, err);
 }
 
 static bool hci_req_is_complete(struct hci_dev *hdev)

diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 3d98cb6291da67a9e234fbc705aff457bc9bf424..616c2fef91d24d53056d0131c4dbec6e6fa65829 100644 (file)
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -2314,14 +2314,31 @@ static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason)
        iso_conn_del(hcon, bt_to_errno(reason));
 }
 
-void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
+int iso_recv(struct hci_dev *hdev, u16 handle, struct sk_buff *skb, u16 flags)
 {
-       struct iso_conn *conn = hcon->iso_data;
+       struct hci_conn *hcon;
+       struct iso_conn *conn;
        struct skb_shared_hwtstamps *hwts;
        __u16 pb, ts, len, sn;
 
-       if (!conn)
-               goto drop;
+       hci_dev_lock(hdev);
+
+       hcon = hci_conn_hash_lookup_handle(hdev, handle);
+       if (!hcon) {
+               hci_dev_unlock(hdev);
+               kfree_skb(skb);
+               return -ENOENT;
+       }
+
+       conn = iso_conn_hold_unless_zero(hcon->iso_data);
+       hcon = NULL;
+
+       hci_dev_unlock(hdev);
+
+       if (!conn) {
+               kfree_skb(skb);
+               return -EINVAL;
+       }
 
        pb     = hci_iso_flags_pb(flags);
        ts     = hci_iso_flags_ts(flags);
@@ -2377,7 +2394,7 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
                        hci_skb_pkt_status(skb) = flags & 0x03;
                        hci_skb_pkt_seqnum(skb) = sn;
                        iso_recv_frame(conn, skb);
-                       return;
+                       goto done;
                }
 
                if (pb == ISO_SINGLE) {
@@ -2455,6 +2472,9 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 
 drop:
        kfree_skb(skb);
+done:
+       iso_conn_put(conn);
+       return 0;
 }
 
 static struct hci_cb iso_cb = {

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 35c57657bcf4e8fa40d8f6eb5671cf2d7292e8d9..07b493331fd781c0d16a0e6ce43b6eedc1359463 100644 (file)
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -7510,13 +7510,24 @@ struct l2cap_conn *l2cap_conn_hold_unless_zero(struct l2cap_conn *c)
        return c;
 }
 
-void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
+int l2cap_recv_acldata(struct hci_dev *hdev, u16 handle,
+                      struct sk_buff *skb, u16 flags)
 {
+       struct hci_conn *hcon;
        struct l2cap_conn *conn;
        int len;
 
-       /* Lock hdev to access l2cap_data to avoid race with l2cap_conn_del */
-       hci_dev_lock(hcon->hdev);
+       /* Lock hdev for hci_conn, and race on l2cap_data vs. l2cap_conn_del */
+       hci_dev_lock(hdev);
+
+       hcon = hci_conn_hash_lookup_handle(hdev, handle);
+       if (!hcon) {
+               hci_dev_unlock(hdev);
+               kfree_skb(skb);
+               return -ENOENT;
+       }
+
+       hci_conn_enter_active_mode(hcon, BT_POWER_FORCE_ACTIVE_OFF);
 
        conn = hcon->l2cap_data;
 
@@ -7524,12 +7535,13 @@ void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
                conn = l2cap_conn_add(hcon);
 
        conn = l2cap_conn_hold_unless_zero(conn);
+       hcon = NULL;
 
-       hci_dev_unlock(hcon->hdev);
+       hci_dev_unlock(hdev);
 
        if (!conn) {
                kfree_skb(skb);
-               return;
+               return -EINVAL;
        }
 
        BT_DBG("conn %p len %u flags 0x%x", conn, skb->len, flags);
@@ -7643,6 +7655,7 @@ drop:
 unlock:
        mutex_unlock(&conn->lock);
        l2cap_conn_put(conn);
+       return 0;
 }
 
 static struct hci_cb l2cap_cb = {

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index ab0cf442d57b95fc1368a17f26e2344e673b7364..298c2a9ab4df8f7826800bf4066f23727c825e9e 100644 (file)
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -1458,22 +1458,39 @@ static void sco_disconn_cfm(struct hci_conn *hcon, __u8 reason)
        sco_conn_del(hcon, bt_to_errno(reason));
 }
 
-void sco_recv_scodata(struct hci_conn *hcon, struct sk_buff *skb)
+int sco_recv_scodata(struct hci_dev *hdev, u16 handle, struct sk_buff *skb)
 {
-       struct sco_conn *conn = hcon->sco_data;
+       struct hci_conn *hcon;
+       struct sco_conn *conn;
 
-       if (!conn)
-               goto drop;
+       hci_dev_lock(hdev);
+
+       hcon = hci_conn_hash_lookup_handle(hdev, handle);
+       if (!hcon) {
+               hci_dev_unlock(hdev);
+               kfree_skb(skb);
+               return -ENOENT;
+       }
+
+       conn = sco_conn_hold_unless_zero(hcon->sco_data);
+       hcon = NULL;
+
+       hci_dev_unlock(hdev);
+
+       if (!conn) {
+               kfree_skb(skb);
+               return -EINVAL;
+       }
 
        BT_DBG("conn %p len %u", conn, skb->len);
 
-       if (skb->len) {
+       if (skb->len)
                sco_recv_frame(conn, skb);
-               return;
-       }
+       else
+               kfree_skb(skb);
 
-drop:
-       kfree_skb(skb);
+       sco_conn_put(conn);
+       return 0;
 }
 
 static struct hci_cb sco_cb = {