Re: [PATCH] Bluetooth: L2CAP: fix race l2cap_sock_cleanup_listen() vs. put_chan

[Pauli Virtanen]

ti, 2026-08-04 kello 16:16 +0800, Hillf Danton kirjoitti:
> On Tue, 04 Aug 2026 08:40:16 +0300 Pauli Virtanen wrote:
> > ti, 2026-08-04 kello 08:47 +0800, Hillf Danton kirjoitti:
> > > On Mon, 03 Aug 2026 19:53:31 +0300 Pauli Virtanen wrote:
> > > >
> > > >    [Task 1]                         [Task 2 (hdev->workqueue)]
> > > >    l2cap_sock_release(parent)       l2cap_disconn_cfm
> > > >      l2cap_sock_cleanup_listen        l2cap_conn_del
> > > >        bt_accept_dequeue                l2cap_chan_del
> > > >          lock_sock(sk)                    l2cap_sock_teardown_cb
> > > >          bt_accept_unlink
> > > >            bt_sk(sk)->parent = NULL
> > > >          release_sock(sk) ----------------> lock_sock(sk)
> > > >                                             parent = bt_sk(sk)->parent /* == NULL */
> > > >        lock_sock(sk) <--------------------- release_sock(sk)
> > > >                                             sock_set_flag(sk, SOCK_ZAPPED)
> > > >                                         l2cap_sock_close_cb
> > > >                                           l2cap_sock_kill(sk)
> > > >                                             l2cap_sock_put_chan
> > > >        chan = READ l2cap_pi(sk)->chan         l2cap_pi(sk)->chan = NULL
> > > >        l2cap_chan_hold_unless_zero            l2cap_put_chan(chan)
> > > >          kref_get_unless_zero(&chan->ref)
> > >
> > > The race window is still open after this work.
> > >
> > > 					     release_sock(sk)
> > >                                              sock_set_flag(sk, SOCK_ZAPPED)
> > >                                          l2cap_sock_close_cb
> > >                                            l2cap_sock_kill(sk)
> > >                                              l2cap_sock_put_chan
> > > 					       l2cap_pi(sk)->chan = NULL
> > > 					       l2cap_put_chan(chan)
> > > 					     sock_set_flag(sk, SOCK_DEAD);
> > > 					     sock_put(sk); // free sk
> > >         lock_sock(sk) // uaf
> > >         chan = READ l2cap_pi(sk)->chan
> > >         l2cap_chan_hold_unless_zero
> > 
> > There is no UAF there, Task 1 holds a reference on sk at this point, if
> > you look at the code sock_put() follows.
> > 
> If the subsequent put works without both race and mm leak, then it is the
> very evidence of uaf with race.

This and the "// uaf" and "// free sk" above appear to ignore what is
explained also in the comment in l2cap_sock_cleanup_listen():

	 * bt_accept_dequeue() returns sk with its temporary queue-
walk
	 * reference held, so a concurrent l2cap_conn_del()
	 * -> l2cap_sock_kill() cannot free sk under us.

bt_accept_dequeue() returns a new temporary reference to sk, which is
owned by the caller who needs to put it after use.

The "sock_put(sk); // free sk" cannot free sk, because
bt_accept_dequeue(sk) got a new reference on sk before it, so the
refcount is at least 2 here. Then "lock_sock(sk) // uaf" cannot be UAF.

If chan != NULL in Task 1, it schedules l2cap_chan teardown. That
eventually ends up in l2cap_sock_kill() to put the reference owned by
l2cap_chan & sk_socket.

If chan == NULL in Task 1, l2cap_chan cleanup is completed or
concurrent, and nothing more needs to be done.

-- 
Pauli Virtanen