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

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

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: corrupt patch at line 75
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth