Re: [PATCH] Bluetooth: L2CAP: fix race l2cap_sock_cleanup_listen() vs. put_chan
From: Pauli Virtanen
Date: Tue Aug 04 2026 - 01:46:10 EST
Hi,
ti, 2026-08-04 kello 08:47 +0800, Hillf Danton kirjoitti:
> On Mon, 03 Aug 2026 19:53:31 +0300 Pauli Virtanen wrote:
> > ma, 2026-08-03 kello 14:13 +0800, Hillf Danton kirjoitti:
> > > On Sun, 2 Aug 2026 15:12:28 +0300 Pauli Virtanen wrote:
> > > > For L2CAP sockets without owning sk->sk_socket, reading
> > > > l2cap_pi(sk)->chan may race against concurrent
> > > > l2cap_sock_kill() ->
> > > > l2cap_sock_put_chan(). This excludes simultaneous proto_ops
> > > > callbacks,
> > > > but access in l2cap_sock_cleanup_listen() has unsafe lockless
> > > > read.
> > > >
> > > > Fix the race by taking lock_sock() in l2cap_sock_kill() to
> > > > synchronize with l2cap_sock_cleanup_listen().
> > > > hold_unless_zero() is not
> > > > needed here, l2cap_pi(sk)->chan owns reference if it is non-
> > > > NULL.
> > > >
> > > > Fixes: 0e2c0392b9dc ("Bluetooth: L2CAP: Fix use-after-free in
> > > > l2cap_sock_new_connection_cb()")
> > > > Reported-by:
> > > > syzbot+e6382a2f53f5fc7453ac@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > Closes:
> > > > https://syzkaller.appspot.com/bug?extid=e6382a2f53f5fc7453ac
> > > > Signed-off-by: Pauli Virtanen <pav@xxxxxx>
> > > > ---
> > > > include/net/bluetooth/l2cap.h | 5 +++++
> > > > net/bluetooth/l2cap_sock.c | 23 +++++++++++++----------
> > > > 2 files changed, 18 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/include/net/bluetooth/l2cap.h
> > > > b/include/net/bluetooth/l2cap.h
> > > > index ef6ce1c20a4f..3d9a32094347 100644
> > > > --- a/include/net/bluetooth/l2cap.h
> > > > +++ b/include/net/bluetooth/l2cap.h
> > > > @@ -699,7 +699,12 @@ struct l2cap_rx_busy {
> > > >
> > > > struct l2cap_pinfo {
> > > > struct bt_sock bt;
> > > > +
> > > > + /* With owning sk_socket chan may be read without
> > > > lock, other access
> > > > + * should hold lock_sock.
> > > > + */
> > > > struct l2cap_chan *chan;
> > > > +
> > > > struct list_head rx_busy;
> > > > };
> > > >
> > > > diff --git a/net/bluetooth/l2cap_sock.c
> > > > b/net/bluetooth/l2cap_sock.c
> > > > index 735167f73f31..9540617a0e6c 100644
> > > > --- a/net/bluetooth/l2cap_sock.c
> > > > +++ b/net/bluetooth/l2cap_sock.c
> > > > @@ -1312,7 +1312,12 @@ static void l2cap_sock_kill(struct sock
> > > > *sk)
> > > >
> > > > BT_DBG("sk %p state %s", sk, state_to_string(sk-
> > > > >sk_state));
> > > >
> > > > + /* Take lock to synchronize against access without
> > > > owning sk->sk_socket,
> > > > + * eg. in l2cap_sock_cleanup_listen(). proto_ops etc.
> > > > don't need lock.
> > > > + */
> > > > + lock_sock(sk);
> > > > l2cap_sock_put_chan(sk);
> > > > + release_sock(sk);
> > > >
> > > > /* Kill poor orphan */
> > > > sock_set_flag(sk, SOCK_DEAD);
> > >
> > > In l2cap_sock_teardown_cb(), sock is only zapped after cleanup
> > > including unlink,
> > > so why do you see a linked and zapped sock in
> > > l2cap_sock_cleanup_listen()?
> >
> > l2cap_sock_cleanup_listen() is not a single critical section.
> >
> > There is the following race:
> >
> > [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.
I don't think there is a remaining problem.
--
Pauli Virtanen