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

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-Pl5Pbv+GP7P466ipTTIvnc23WoclnBCfAL8bYrjMMd8@public.gmane.org
> > > Closes: https://syzkaller.appspot.com/bug?extid=e6382a2f53f5fc7453ac
> > > Signed-off-by: Pauli Virtanen <[email protected]>
> > > ---
> > >  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