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

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()?

> @@ -1516,14 +1521,10 @@ static void l2cap_sock_cleanup_listen(struct sock *parent)
>  	 * establish sk_lock -> conn->lock and invert the established
>  	 * conn->lock -> chan->lock -> sk_lock order (lockdep deadlock).
>  	 *
> -	 * Instead, briefly take the child sk lock to fetch and pin its chan.
> -	 * l2cap_conn_del() reaches the chan free only via
> -	 * l2cap_chan_del() -> l2cap_sock_teardown_cb(), which itself takes
> -	 * the child sk lock; holding it across l2cap_chan_hold_unless_zero()
> -	 * therefore guarantees the chan cannot be freed while we read and
> -	 * pin it (hold_unless_zero() additionally skips a chan already past
> -	 * its last reference).  We then drop the sk lock before taking
> -	 * chan->lock, so sk and chan locks are never held together.
> +	 * Instead, briefly take the child sk lock to synchronize vs.
> +	 * l2cap_sock_kill that puts l2cap_pi(sk)->chan. We then drop the sk
> +	 * lock before taking chan->lock, so sk and chan locks are never held
> +	 * together.
>  	 *
>  	 * Since we cannot call l2cap_chan_close() without conn->lock,
>  	 * schedule l2cap_chan_timeout to close the channel; it already
> @@ -1533,10 +1534,12 @@ static void l2cap_sock_cleanup_listen(struct sock *parent)
>  		struct l2cap_chan *chan;
>  
>  		lock_sock_nested(sk, L2CAP_NESTING_NORMAL);
> -		chan = l2cap_chan_hold_unless_zero(l2cap_pi(sk)->chan);
> +		chan = l2cap_pi(sk)->chan;
> +		if (chan)
> +			l2cap_chan_hold(chan);
>  		release_sock(sk);
>  		if (!chan) {
> -			/* l2cap_conn_del() already tearing this child down */
> +			/* Already torn down */
>  			sock_put(sk);
>  			continue;
>  		}
> -- 
> 2.55.0