Bluetooth: KASAN: slab-use-after-free in hci_chan_sent (chan/conn used after rcu_read_unlock in TX scheduler)

Bluetooth: KASAN: slab-use-after-free in hci_chan_sent (chan/conn used after rcu_read_unlock in TX scheduler)

[luckdog]

Dear Bluetooth maintainers,

I am reporting a slab use-after-free (UAF) in the HCI TX scheduler. I force-verified
the core race under KASAN. I have not produced a natural misbehaving-
controller reproducer; the KASAN evidence below was obtained with a small
verification harness that force-triggers the race, because the natural window
is narrow. Observed on Linux v7.3-rc1 (commit: cee9395acd8043be0644b25c34bfa86623f2b935).

Call Trace & Context
==================================================================
BUG: KASAN: slab-use-after-free in hci_chan_sent+0x838/0x840
Read of size 8 at addr ffff888010ac6b98 by task kworker/u4:3/45

CPU: 0 UID: 0 PID: 45 Comm: kworker/u4:3 Not tainted 7.3.0-rc1 #4 PREEMPT(lazy)
Workqueue: repro hci_tx_work
Call Trace:
 <TASK>
 dump_stack_lvl+0xab/0xe0
 print_report+0xcb/0x5e0
 kasan_report+0xb8/0xf0
 hci_chan_sent+0x838/0x840          (hci_quote_sent(chan->conn))
 hci_tx_work+0x6b3/0xc60             (skb_peek(&chan->data_q))
 ...
 </TASK>

Allocated by task 1:
 hci_chan_create+0x9f/0x350
 (verification harness)

Freed by task 1:
 kfree+0x162/0x450
 hci_chan_del+0x.../0x...           fs/bluetooth/hci_conn.c:2916   (called by the harness)
 ...

BUG: KASAN: slab-use-after-free in hci_tx_work+0xbdb/0xc60
Read of size 8 at addr ffff888010ac6ba0 by task kworker/u4:3/45
Workqueue: repro hci_tx_work
 ... hci_tx_work+0xbdb/0xc60   -> hci_sched_acl_pkt at fs/bluetooth/hci_core.c:3792
 Allocated by task 1:  hci_chan_create+0x9f/0x350
 Freed by task 1:      kfree+0x162/0x450  (via hci_chan_del)
 freed 64-byte region [ffff888010ac6b80, ffff888010ac6bc0)   /* kmalloc-64 == struct hci_chan */
==================================================================

The two reads land 24 bytes and 32 bytes inside the freed 64-byte object,
which matches the layout of `struct hci_chan`:

    struct hci_chan {           /* include/net/bluetooth/hci_core.h:802 */
        struct list_head    list;     /* off 0..15  */
        __u16               handle;   /* off 16     */
        /* pad */                      18..23
        struct hci_conn   *conn;     /* off 24  <-- KASAN hit #1 (hci_chan_sent:3507) */
        struct sk_buff_head data_q;  /* off 32  <-- KASAN hit #2 (hci_sched_acl_pkt:3792) */
        unsigned int        sent;
        __u8                state;
    };

Execution Flow & Code Context
`hci_chan_sent()` walks the connection/chan lists under rcu_read_lock(),
picks a `struct hci_chan`, then rcu_read_unlock(), and continues to
dereference the just-obtained pointer afterwards:

```c
// net/bluetooth/hci_core.c
static struct hci_chan *hci_chan_sent(struct hci_dev *hdev, __u8 type,
                                      int *quote)
{
    ...
    rcu_read_lock();                                   /* 3461 */
    list_for_each_entry_rcu(conn, &h->list, list) {    /* 3474 */
        if (conn->state != BT_CONNECTED && conn->state != BT_CONFIG)
            continue;
        list_for_each_entry_rcu(tmp, &conn->chan_list, list) { /* 3484 */
            ...
            if (conn->sent < min) { min = conn->sent; chan = tmp; }
        }
    }
    rcu_read_unlock();                                 /* 3502  <-- RCU section ends */
    if (!chan)
        return NULL;
    hci_quote_sent(chan->conn, num, quote);            /* 3507  <-- deref chan->conn AFTER unlock */
    ...
    return chan;                                       /* 3510  <-- bare chan handed to caller */
}
```

The caller then dereferences the returned chan locklessly:

```c
// net/bluetooth/hci_core.c
static void hci_sched_acl_pkt(struct hci_dev *hdev)
{
    ...
    while (hdev->acl_cnt && (chan = hci_chan_sent(hdev, ACL_LINK, &quote))) {
        u32 priority = (skb_peek(&chan->data_q))->priority;          /* 3792: deref chan->data_q */
        while (quote-- && (skb = skb_peek(&chan->data_q))) {
            ...
            hci_conn_enter_active_mode(chan->conn, ...);             /* 3663 */
            hci_send_conn_frame(hdev, chan->conn, skb);              /* 3666 */
            chan->sent++;                                           /* 3670 */
            chan->conn->sent++;                                     /* 3671 */
        }
    }
}
```

`hci_low_sent()` has the symmetric shape (post-unlock deref of `conn`).

The freer is `hci_chan_del()`:

```c
// net/bluetooth/hci_conn.c
void hci_chan_del(struct hci_chan *chan)
{
    ...
    list_del_rcu(&chan->list);     /* 2906 */
    synchronize_rcu();             /* 2908: only waits for in-section readers */
    set_bit(HCI_CONN_DROP, &conn->flags);
    hci_conn_put(conn);            /* 2913 */
    skb_queue_purge(&chan->data_q);/* 2915 */
    kfree(chan);                   /* 2916: unconditional, no kfree_rcu/call_rcu */
}
```

`struct hci_chan` has no refcount and no rcu_head; the slab is plain
kmalloc (not SLAB_TYPESAFE_BY_RCU).

Root Cause Analysis
This is the textbook "using an RCU-protected pointer after rcu_read_unlock()"
antipattern. `synchronize_rcu()` in `hci_chan_del()` only guarantees that
readers *inside* their rcu_read_lock()...rcu_read_unlock() critical section
are done; it does not cover the reader's post-unlock use of `chan`. Because
`hci_chan` carries no refcount, once the reader exits rcu_read_unlock(),
`hci_chan_del()` can kfree() the chan while the reader still touches
chan->conn / chan->data_q.

It is important to be precise about when reader and freer can actually race:

1) The *common* disconnect path is SAFE. A normal disconnect (supervision
   timeout, mgmt DISCONNECT, protocol event) produces a disconnect-complete
   event processed by hci_rx_work() on *hdev->workqueue*:

       hci_disconn_complete_evt() -> hci_conn_del() -> hci_chan_list_flush
         -> hci_chan_del() -> kfree(chan)

   hci_rx_work and hci_tx_work are both on hdev->workqueue, which is an
   alloc_ordered_workqueue() (single-concurrency) [hci_core.c:2568], so they
   are mutually exclusive. The common freer never races tx_work.

2) The only freer path that can race tx_work runs on a separate ordered
   workqueue (hdev->req_workqueue):

       hci_abort_conn_sync()  (net/bluetooth/hci_sync.c:5957)
         case BT_CONNECTED: hci_disconnect_sync()
           -> __hci_cmd_sync_status_sk(HCI_OP_DISCONNECT,
                 HCI_EV_DISCONN_COMPLETE, HCI_CMD_TIMEOUT)   /* 2 seconds */
         -> hci_conn_failed(conn)            (hci_conn.c:1399)
              conn->state = BT_CLOSED
              hci_conn_del(conn)             (1415)
                -> hci_chan_list_flush -> hci_chan_del -> kfree(chan)

   This path only reaches hci_chan_del() when the controller does NOT emit
   HCI_EV_DISCONN_COMPLETE, i.e. hci_disconnect_sync() times out
   (HCI_CMD_TIMEOUT = 2s, include/net/bluetooth/hci.h:483). That requires
   an unresponsive/misbehaving controller (e.g. an attacker-controlled
   virtual HCI via /dev/vhci, or a buggy real controller).

3) Even on the concurrent path, the reader's post-unlock burst is fast
   (hci_quote_sent arithmetic + skb_peek + hdev->send, which for vhci and
   common USB/UART drivers is non-blocking), so it usually finishes before
   the freer's kfree (which is delayed by the 2s timeout plus two
   synchronize_rcu() grace periods). Overlap needs the reader preempted in
   post-unlock, or a real controller whose hdev->send blocks under flow
   control. The window is therefore narrow.

So the defect is a real RCU rule violation; the natural trigger is narrow
(needs a misbehaving controller + unlucky preemption/flow-control timing),
not "every disconnect".

Potential Impact
Local, requires CAP_NET_ADMIN (creating/bringing up a Bluetooth adapter,
driving a virtual HCI) or a buggy/misbehaving controller. Best characterized
as a local DoS (kernel panic/Oops under KASAN; potential memory corruption
otherwise) rather than a reliable privilege escalation. The common disconnect
path does not trigger it; only the 2s-timeout (misbehaving controller) path
races the reader.

Verification (force-triggered, under KASAN)
Because the natural window is narrow, I verified the core race with a small
gated harness added to net/bluetooth/hci_core.c. It (a) inserts a gated
msleep() right after rcu_read_unlock() in hci_chan_sent() to widen the
post-unlock window, and (b) a debugfs trigger that builds a throwaway
hci_dev+conn+chan, queues hci_tx_work (so hci_chan_sent picks the chan and
stalls in post-unlock holding chan), then calls the REAL hci_chan_del(chan)
to free it during the stall. The harness calls only real BT functions
(hci_chan_create / hci_chan_sent / hci_tx_work / hci_chan_del / kfree); it
replaces only the upstream freer reachability (a debugfs write instead of the
natural abort_conn_sync + 2s timeout) and widens the reader window (msleep).
Default stall=0 -> no-op, kernel behaves exactly as upstream.

```diff
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -26,6 +26,7 @@
 #include <linux/rfkill.h>
 #include <linux/debugfs.h>
+#include <linux/delay.h>
 #include <linux/crypto.h>
@@
+/* REPRO-ONLY: verification harness (not a fix). Default 0 = no-op. */
+unsigned int hci_chan_repro_stall_ms;
+EXPORT_SYMBOL_GPL(hci_chan_repro_stall_ms);
+static struct hci_chan *repro_chan;
+
+static int repro_hdev_open(struct hci_dev *hdev) { return 0; }
+static int repro_hdev_close(struct hci_dev *hdev) { return 0; }
+static int repro_hdev_send(struct hci_dev *hdev, struct sk_buff *skb)
+{ kfree_skb(skb); return 0; }
+
+static int hci_chan_repro_force(void)
+{
+    struct hci_dev *hdev; struct hci_conn *conn; struct hci_chan *chan;
+    struct sk_buff *skb; bdaddr_t dst = {{0,}};
+
+    hdev = hci_alloc_dev(); if (!hdev) return -ENOMEM;
+    hdev->open = repro_hdev_open; hdev->close = repro_hdev_close;
+    hdev->send = repro_hdev_send; hdev->bus = HCI_VIRTUAL;
+    hdev->acl_mtu = 1024; hdev->acl_pkts = 100; hdev->acl_cnt = 100;
+    hdev->workqueue = alloc_ordered_workqueue("repro", 0);
+    hdev->req_workqueue = alloc_ordered_workqueue("reprorq", 0);
+
+    hci_dev_lock(hdev);
+    conn = hci_conn_add(hdev, ACL_LINK, &dst, 0, HCI_ROLE_MASTER, 1);
+    if (IS_ERR(conn)) { hci_dev_unlock(hdev); goto out; }
+    conn->state = BT_CONNECTED;
+    chan = hci_chan_create(conn);
+    if (!chan) { hci_dev_unlock(hdev); goto out; }
+    skb = alloc_skb(64, GFP_KERNEL); if (skb) skb_queue_tail(&chan->data_q, skb);
+    hci_dev_unlock(hdev);
+
+    WRITE_ONCE(repro_chan, NULL);
+    WRITE_ONCE(hci_chan_repro_stall_ms, 50);
+    queue_work(hdev->workqueue, &hdev->tx_work);
+    while (!READ_ONCE(repro_chan))
+        schedule_timeout_uninterruptible(msecs_to_jiffies(1));
+    hci_dev_lock(hdev);
+    hci_chan_del(chan);   /* list_del_rcu + synchronize_rcu + kfree */
+    hci_dev_unlock(hdev);
+    flush_work(&hdev->tx_work);   /* reader resumes, derefs freed chan -> KASAN */
+    WRITE_ONCE(hci_chan_repro_stall_ms, 0);
+out:
+    destroy_workqueue(hdev->req_workqueue);
+    destroy_workqueue(hdev->workqueue);
+    hci_free_dev(hdev);
+    return 0;
+}
+
+static ssize_t hci_chan_repro_force_write(struct file *f, const char __user *b,
+                                          size_t n, loff_t *o)
+{ hci_chan_repro_force(); return n; }
+static const struct file_operations hci_chan_repro_fops = {
+    .write = hci_chan_repro_force_write, .open = simple_open, .llseek = noop_llseek,
+};
+late_initcall(... /* debugfs_create_file("hci_chan_repro_force", 0220, bt_debugfs, ...) */);
@@ static struct hci_chan *hci_chan_sent(...)
     rcu_read_unlock();
     if (!chan)
         return NULL;
+    if (hci_chan_repro_stall_ms) {
+        WRITE_ONCE(repro_chan, chan);
+        msleep(hci_chan_repro_stall_ms);
+    }
     hci_quote_sent(chan->conn, num, quote);
```

Proposed Fix
The fundamental issue is that chan/conn obtained under rcu_read_lock() are
used after rcu_read_unlock(). `synchronize_rcu()` in `hci_chan_del()` cannot
cover that. The cleanest fix is to give `struct hci_chan` a refcount and use
the self-held-ref pattern (mirroring hci_conn), so the reader holds a ref
across its post-unlock use:

```diff
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ struct hci_chan {
 	struct sk_buff_head data_q;
 	unsigned int	sent;
 	__u8		state;
+	refcount_t	refcnt;
 };

+static inline void hci_chan_hold(struct hci_chan *c)   { refcount_inc(&c->refcnt); }
+static inline bool hci_chan_get(struct hci_chan *c)    { return refcount_inc_not_zero(&c->refcnt); }
+static inline void hci_chan_put(struct hci_chan *c)    { if (refcount_dec_and_test(&c->refcnt)) kfree(c); }
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ struct hci_chan *hci_chan_create(struct hci_conn *conn)
 	chan->conn = hci_conn_get(conn);
 	skb_queue_head_init(&chan->data_q);
 	chan->state = BT_CONNECTED;
+	refcount_set(&chan->refcnt, 1);   /* initial ref for the list */
 	list_add_rcu(&chan->list, &conn->chan_list);
 	return chan;
@@ void hci_chan_del(struct hci_chan *chan)
 	hci_conn_put(conn);
 	skb_queue_purge(&chan->data_q);
-	kfree(chan);
+	hci_chan_put(chan);
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ static struct hci_chan *hci_chan_sent(...)
 	}
 	rcu_read_unlock();
 	if (!chan)
 		return NULL;
+	if (!hci_chan_get(chan))   /* take a ref before post-unlock use */
+		return NULL;
 	hci_quote_sent(chan->conn, num, quote);
 	...
 	return chan;
@@ static void hci_sched_acl_pkt(struct hci_dev *hdev)
 	while (hdev->acl_cnt && (chan = hci_chan_sent(hdev, ACL_LINK, &quote))) {
 		...
+		hci_chan_put(chan);   /* drop the ref taken in hci_chan_sent */
 	}
```

An alternative is to hold rcu_read_lock() across the whole chan use in
hci_sched_acl_pkt() (and have hci_chan_sent() not drop it), but that requires
auditing every hdev->send() implementation for sleeping, since an RCU
read-section must not sleep; the refcount approach is safer.

Unfortunately, I was unable to produce a natural (misbehaving-controller)
reproducer: the common disconnect path is safe (ordered workqueue), and the
concurrent path needs a controller that does not emit
HCI_EV_DISCONN_COMPLETE (so that hci_disconnect_sync() times out after 2s)
plus unlucky preemption/flow-control timing for the reader's microsecond
post-unlock window to overlap the freer's delayed kfree. The KASAN evidence
above was therefore obtained with the force-trigger harness shown in the
Verification section, which proves the core race is a real UAF (not a false
positive) but does not establish field exploitability. I would be grateful if
the maintainers could assess practical severity and pick a fix.If you have fixed this bug, 
please add "reported by: Jianzhou Zhao".


Best regards,
Jianzhou Zhao
luckd0g@163.com