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

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:
> > > > [email protected]
> > > > 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

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