public inbox for linux-bluetooth@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] Bluetooth: L2CAP: Fix use-after-free in l2cap_chan_timeout
@ 2026-03-20 11:41 Hyunwoo Kim
  2026-03-20 12:13 ` [v2] " bluez.test.bot
  2026-03-20 13:59 ` [PATCH v2] " Luiz Augusto von Dentz
  0 siblings, 2 replies; 6+ messages in thread
From: Hyunwoo Kim @ 2026-03-20 11:41 UTC (permalink / raw)
  To: marcel, johan.hedberg, luiz.dentz; +Cc: linux-bluetooth, imv4bel

l2cap_chan_timeout() reads chan->conn without holding any lock and
without taking a reference on the connection. If l2cap_conn_del()
frees the connection concurrently, mutex_lock(&conn->lock) operates
on freed memory. Additionally, the existing NULL check early return
leaks the channel reference held by the timer.

Fix the root cause by disabling channel timers when the channel is
being destroyed. Use disable_delayed_work() in l2cap_chan_del() to
prevent timers from being rearmed, and disable_delayed_work_sync()
in l2cap_conn_del() to ensure running timer work has completed
before the connection is freed.

The sync cannot be done directly inside l2cap_chan_del() because it
would deadlock in three independent ways: (1) l2cap_chan_timeout()
can call l2cap_chan_close() -> l2cap_chan_del(), so syncing chan_timer
would wait for itself; (2) callers hold conn->lock while
l2cap_chan_timeout() acquires it; (3) callers hold chan->lock while
the ERTM timer functions acquire it. Therefore, the sync is
performed in l2cap_conn_del() after releasing conn->lock.

Also fix the reference leak on the !conn early return path in
l2cap_chan_timeout().

Fixes: 3df91ea20e74 ("Bluetooth: Revert to mutexes from RCU list")
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
Changes in v2:
- Moved the fix from l2cap_chan_timeout() to l2cap_chan_del()/l2cap_conn_del() to address the root cause instead of adding locking in the callback
- Used disable_delayed_work()/disable_delayed_work_sync() to prevent timer rearming and ensure completion, as suggested by Luiz
- Applied the fix to all four channel timers (chan_timer, retrans_timer, monitor_timer, ack_timer)
- Removed l2cap_conn_get()/l2cap_conn_put() and the lock reordering that caused a circular locking dependency in v1
- v1: https://lore.kernel.org/all/abwSxg_C6n8Avid3@v4bel/
---
 net/bluetooth/l2cap_core.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 5deb6c4f1e41..c08478e4512b 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -411,8 +411,10 @@ static void l2cap_chan_timeout(struct work_struct *work)
 
 	BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
 
-	if (!conn)
+	if (!conn) {
+		l2cap_chan_put(chan);
 		return;
+	}
 
 	mutex_lock(&conn->lock);
 	/* __set_chan_timer() calls l2cap_chan_hold(chan) while scheduling
@@ -649,6 +651,11 @@ void l2cap_chan_del(struct l2cap_chan *chan, int err)
 	struct l2cap_conn *conn = chan->conn;
 
 	__clear_chan_timer(chan);
+	/* Prevent the timer from being rearmed since the channel is about
+	 * to be destroyed. Running timer work will be synced by
+	 * l2cap_conn_del() after releasing conn->lock.
+	 */
+	disable_delayed_work(&chan->chan_timer);
 
 	BT_DBG("chan %p, conn %p, err %d, state %s", chan, conn, err,
 	       state_to_string(chan->state));
@@ -688,6 +695,10 @@ void l2cap_chan_del(struct l2cap_chan *chan, int err)
 		__clear_retrans_timer(chan);
 		__clear_monitor_timer(chan);
 		__clear_ack_timer(chan);
+		/* Prevent the ERTM timers from being rearmed. */
+		disable_delayed_work(&chan->retrans_timer);
+		disable_delayed_work(&chan->monitor_timer);
+		disable_delayed_work(&chan->ack_timer);
 
 		skb_queue_purge(&chan->srej_q);
 
@@ -1742,6 +1753,7 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 {
 	struct l2cap_conn *conn = hcon->l2cap_data;
 	struct l2cap_chan *chan, *l;
+	LIST_HEAD(del_list);
 
 	if (!conn)
 		return;
@@ -1780,7 +1792,7 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 		chan->ops->close(chan);
 
 		l2cap_chan_unlock(chan);
-		l2cap_chan_put(chan);
+		list_add_tail(&chan->list, &del_list);
 	}
 
 	if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
@@ -1791,6 +1803,20 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 
 	hcon->l2cap_data = NULL;
 	mutex_unlock(&conn->lock);
+
+	/* Disable and sync-wait for any running channel timer work without
+	 * holding conn->lock to avoid AB-BA deadlock with
+	 * l2cap_chan_timeout() which acquires conn->lock.
+	 */
+	list_for_each_entry_safe(chan, l, &del_list, list) {
+		disable_delayed_work_sync(&chan->chan_timer);
+		disable_delayed_work_sync(&chan->retrans_timer);
+		disable_delayed_work_sync(&chan->monitor_timer);
+		disable_delayed_work_sync(&chan->ack_timer);
+		list_del(&chan->list);
+		l2cap_chan_put(chan);
+	}
+
 	l2cap_conn_put(conn);
 }
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-03-23 19:44 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-20 11:41 [PATCH v2] Bluetooth: L2CAP: Fix use-after-free in l2cap_chan_timeout Hyunwoo Kim
2026-03-20 12:13 ` [v2] " bluez.test.bot
2026-03-20 13:59 ` [PATCH v2] " Luiz Augusto von Dentz
2026-03-21 16:29   ` Hyunwoo Kim
2026-03-23 19:16     ` Luiz Augusto von Dentz
2026-03-23 19:44       ` Hyunwoo Kim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox