Re: [PATCH v2] Bluetooth: L2CAP: Fix use-after-free in l2cap_chan_timeout

[Hyunwoo Kim <imv4bel@gmail.com>]

On Fri, Mar 20, 2026 at 09:59:50AM -0400, Luiz Augusto von Dentz wrote:
> Hi Hyunwoo,
> 
> On Fri, Mar 20, 2026 at 7:41 AM Hyunwoo Kim <imv4bel@gmail.com> wrote:
> >
> > 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);
> 
> Hmm, so we can't use the _sync version because it could be pending on
> conn->lock while this could be called from l2cap_conn_del? I wonder if
> we should clarify the comment or if we actually need to mark the
> l2cap_chan as deleted/disconnected or something, perhaps via state,
> also we may want to return something like -EINPROGRESS if a timer
> could not be disabled which would indicate to the likes of
> l2cap_conn_del that it need a cleanup.
> 
> >         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);
> 
> 
> l2cap_clear_timer drops the reference if the timer was cancelled, so I
> think we will need to check the return of disable as well, or create a
> function like l2cap_disable_timer, that said Id return something to
> indicate that l2cap_chan_del didn't cleanup everything.
> 
> >                 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);
> 
> Let's populate the del_list only with channels that actually had a
> pending timer.
> 
> > +               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);
> > +       }
> 
> It might be better to place the disable sync-wait on
> l2cap_chan_destroy then since l2cap_chan_del is not garanteed to do
> it, which means l2cap_chan_put is not safe to be called with
> conn->lock, but I guess it was never really safe since if we destroy
> the chan while a timer is pending/running it would cause an UAF
> 
> >         l2cap_conn_put(conn);
> >  }
> >
> > --
> > 2.43.0
> >
> 
> 
> -- 
> Luiz Augusto von Dentz


This is regarding the missing Reported-by credit that was discussed 
on security@kernel.org. My credit has still not been added to the 
commit in the netdev tree:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/net/bluetooth?id=9d87cb22195b2c67405f5485d525190747ad5493

If this gets merged into mainline as-is, my credit will be 
permanently lost, correct?

It seems like the mainline pull request is not far away. I would 
appreciate a quick confirmation on this.


Best regards,
Hyunwoo Kim