Re: [PATCH v3] Bluetooth: af_bluetooth: Fix double list_del and UAF in accept_q

[Pauli Virtanen]

Hi,

pe, 2026-09-18 kello 17:47 +0700, Nguyen Ngoc Thang kirjoitti:
> bt_sk(sk)->parent is a raw pointer with no refcount backing it, so a
> child sitting in the listening socket's accept_q can outlive it. When
> the child's channel is torn down independently (e.g. hci_error_reset()
> -> l2cap_conn_del() -> l2cap_sock_teardown_cb()), bt_accept_unlink()
> dereferences the freed parent, corrupting its accept_q_lock and list.

This was resent without replying to previous comments:

https://lore.kernel.org/linux-bluetooth/1baff112d1ebd255575c7e52936b13db10f218b7.camel@xxxxxx/

AFAIK the issue is already fixed upstream in a different way.

> Additionally, if multiple threads concurrently tear down the child
> socket, bt_accept_unlink() can be called multiple times. Without
> checking if the socket is still in the list, the second thread will
> perform a double list_del_init(), causing list_del corruption (kernel BUG).
> 
> Fix this by:
> 1. Taking a reference on parent in bt_accept_enqueue() and dropping it
>    only when actually unlinked in bt_accept_unlink().
> 2. Checking !list_empty(&bt_sk(sk)->accept_q) inside the accept_q_lock
>    to prevent double unlinking.
> 3. Moving sock_put() outside of the spinlock to avoid sleeping in an
>    atomic context.

These don't look right: eg. sock_put() was not inside spinlock even
before.

> Reported-by: syzbot+534002670dd34a114fdc@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=534002670dd34a114fdc
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> 
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>
> ---
>  net/bluetooth/af_bluetooth.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 411d66f24393..7797b1052808 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -224,6 +224,10 @@ void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
>  	else
>  		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>  
> +	/* Hold a reference on parent so it cannot be freed while sk keeps
> +	 * a raw pointer to it via bt_sk(sk)->parent.
> +	 */
> +	sock_hold(parent);
>  	bt_sk(sk)->parent = parent;
>  
>  	spin_lock_bh(&par->accept_q_lock);
> @@ -257,15 +261,24 @@ EXPORT_SYMBOL(bt_accept_enqueue);
>  void bt_accept_unlink(struct sock *sk)
>  {
>  	struct sock *parent = bt_sk(sk)->parent;
> +	bool unlinked = false;
>  
> -	BT_DBG("sk %p state %d", sk, sk->sk_state);
> +	if (!parent)
> +		return;
>  
>  	spin_lock_bh(&bt_sk(parent)->accept_q_lock);
> -	list_del_init(&bt_sk(sk)->accept_q);
> -	sk_acceptq_removed(parent);
> +	if (!list_empty(&bt_sk(sk)->accept_q)) {
> +		list_del_init(&bt_sk(sk)->accept_q);
> +		sk_acceptq_removed(parent);
> +		unlinked = true;
> +	}
>  	spin_unlock_bh(&bt_sk(parent)->accept_q_lock);
> -	bt_sk(sk)->parent = NULL;
> -	sock_put(sk);
> +
> +	if (unlinked) {
> +		bt_sk(sk)->parent = NULL;
> +		sock_put(parent);
> +		sock_put(sk);
> +	}
>  }
>  EXPORT_SYMBOL(bt_accept_unlink);
>  

-- 
Pauli Virtanen