Re: [PATCH v2 1/1] Bluetooth: Fix parent socket UAF in accept queues

[Pauli Virtanen]

Hi,

to, 2026-09-03 kello 11:11 +0000, Zihan Xi kirjoitti:
> Bluetooth children queued on a listening socket store the listener in
> bt_sk(sk)->parent, but the accept queue did not hold a reference on
> that parent socket.  The child side can later fetch that pointer and
> unlink itself from the accept queue while still needing to notify the
> listener, for example from L2CAP, ISO or RFCOMM teardown/state-change
> callbacks.

Please revalidate the KASAN crash on current bluetooth-next/master,
there have been related fixes since v1 of the patch and the v7.2-rc6
shown in the KASAN crash in the cover letter.

With commit d4bfa78fd679 ("Bluetooth: L2CAP: reject accept queue add
unless BT_LISTEN") in v7.3-rc1 cherry-picked on v7.2-rc6 the POC no
longer reproduces for me.

***

The design intent AFAICS is that the accept queue of the parent socket
shall be empty when the parent socket is freed.

Otherwise, the child sockets in accept queue would leak.

There must then be parent->sk_state == BT_LISTEN check before
bt_accept_enqueue() and some were missing in v7.2-rc6.

bt_sk(sk)->parent read/write is guarded by lock_sock(sk), and it is set
to NULL when removed from accept queue.

Dangling bt_sk(sk)->parent should then not occur.

If bt_sk(sk)->parent != NULL is observed under lock_sock(sk), the
parent socket is valid during that critical section.

The sock_hold/put(parent) in this patch are in lock_sock(sk) critical
sections, so should be no-ops.

The accept queue items owning reference to parent also should be no-
ops.

> If the listener is closed concurrently, removing the child from the
> accept queue can drop the last listener reference before those
> callbacks call parent->sk_data_ready(parent), leaving a stale parent
> pointer and a use-after-free.
> 
> Take a reference on the parent when a child is queued and drop it when
> the child is unlinked.  Since unlinking now drops the accept-queue
> parent reference, take a temporary parent reference in the callbacks
> that continue to notify the parent after bt_accept_unlink().
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Vega <vega@xxxxxxxxxx>
> Assisted-by: Codex:gpt-5.4
> Signed-off-by: Zihan Xi <zihanx@xxxxxxxxxx>
> ---
> changes in v2:
>   - rebase onto current bluetooth-next
>   - refresh trailers to the current submission template
>   - retarget author identity to Zihan Xi <zihanx@xxxxxxxxxx>
>   - v1 Link: https://lore.kernel.org/all/65767989c644f8adf52f35334f4034c66f47881f.1784383243.git.xizh2024@xxxxxxxxxx/
> 
>  net/bluetooth/af_bluetooth.c | 2 ++
>  net/bluetooth/iso.c          | 2 ++
>  net/bluetooth/l2cap_sock.c   | 2 ++
>  net/bluetooth/rfcomm/sock.c  | 2 ++
>  4 files changed, 8 insertions(+)
> 
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 411d66f24393..61a232378e6c 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -218,6 +218,7 @@ void bt_accept_enqueue(struct sock *parent, struct sock *sk, bool bh)
>  	BT_DBG("parent %p, sk %p", parent, sk);
>  
>  	sock_hold(sk);
> +	sock_hold(parent);
>  
>  	if (bh)
>  		bh_lock_sock_nested(sk);
> @@ -266,6 +267,7 @@ void bt_accept_unlink(struct sock *sk)
>  	spin_unlock_bh(&bt_sk(parent)->accept_q_lock);
>  	bt_sk(sk)->parent = NULL;
>  	sock_put(sk);
> +	sock_put(parent);
>  }
>  EXPORT_SYMBOL(bt_accept_unlink);
>  
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 75bfd5938b2e..e709292aa112 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -286,8 +286,10 @@ static void iso_chan_del(struct sock *sk, int err)
>  
>  	parent = bt_sk(sk)->parent;
>  	if (parent) {
> +		sock_hold(parent);
>  		bt_accept_unlink(sk);
>  		parent->sk_data_ready(parent);
> +		sock_put(parent);
>  	} else {
>  		sk->sk_state_change(sk);
>  	}
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index b553b6356af8..3720ab2e39ed 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -1748,8 +1748,10 @@ static void l2cap_sock_teardown_cb(struct l2cap_chan *chan, int err)
>  		sk->sk_err = err;
>  
>  		if (parent) {
> +			sock_hold(parent);
>  			bt_accept_unlink(sk);
>  			parent->sk_data_ready(parent);
> +			sock_put(parent);
>  		} else {
>  			sk->sk_state_change(sk);
>  		}
> diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> index 958081adb9b5..a16daa68ecb8 100644
> --- a/net/bluetooth/rfcomm/sock.c
> +++ b/net/bluetooth/rfcomm/sock.c
> @@ -78,11 +78,13 @@ static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)
>  
>  	parent = bt_sk(sk)->parent;
>  	if (parent) {
> +		sock_hold(parent);
>  		if (d->state == BT_CLOSED) {
>  			sock_set_flag(sk, SOCK_ZAPPED);
>  			bt_accept_unlink(sk);
>  		}
>  		parent->sk_data_ready(parent);
> +		sock_put(parent);
>  	} else {
>  		if (d->state == BT_CONNECTED)
>  			rfcomm_session_getaddr(d->session,

-- 
Pauli Virtanen