Re: [PATCH] Bluetooth: ISO: Serialize concurrent connect calls

[Pauli Virtanen <pav@iki.fi>]

Hi,

ma, 2026-10-05 kello 00:24 +0800, Chengfeng Ye kirjoitti:
> iso_sock_connect() checks the socket state before taking the socket lock
> and drops the lock again before setting up a connection. Two callers can
> both pass the admission check while the socket is open or bound.
> 
> Caller A can copy its destination for route selection, then caller B can
> replace the socket destination before A binds or connects the CIS. A
> then uses B's destination with the route selected for its own request.
> B can also proceed after A attaches a connection and sets BT_CONNECT.
> For deferred BIS setup, B can bind a second BIS and overwrite
> iso_pi(sk)->conn, leaving the first connection's reference and conn->sk
> back-pointer stranded. Concurrent CIS connects can likewise replace the

Memory safety probably should be enforced somewhat down the calls,
likely __iso_chan_add() should reject adding a different iso_conn to sk
if it already has one, since that looks like it leaks the
iso_conn_hold() reference and the back pointer association.

> socket's connection. Later teardown only detaches the current connection,
> so a callback on the old connection can race with socket release and
> access a freed socket.
> 
> KASAN reported:
> 
>   BUG: KASAN: slab-use-after-free in iso_sock_hold+0xf7/0x1b0
>   Call Trace:
>    iso_sock_hold+0xf7/0x1b0
>    iso_conn_del+0x7b/0x1d0
>    iso_connect_cfm+0x186/0x16a0
>    hci_conn_failed+0x154/0x280
>    hci_abort_conn_sync+0x3dc/0x7d0
>    hci_cmd_sync_work+0x173/0x300
> 
>   Allocated by task 121:
>    sk_alloc+0x2b/0x6d0
>    bt_sock_alloc+0x29/0x370
>    iso_sock_alloc.constprop.0+0x19/0x300
>    iso_sock_create+0x94/0x100
> 
>   Freed by task 121:
>    kfree+0x121/0x3c0
>    __sk_destruct+0x42b/0x540
>    iso_sock_release+0x29d/0x340
>    __sock_release+0xa1/0x260
> 
> Check admission under the socket lock and mark the connect operation in
> progress before publishing its destination. Keep that flag set across
> route lookup and connection setup, rejecting another connect with
> -EBADFD before it can change the destination or attach a connection.
> Clear the flag after either helper returns, including on failure, so a
> failed setup can be retried.
> 
> A separate flag is needed because the socket lock must be released before
> acquiring the HCI device lock, while BT_CONNECT requires an attached
> connection. Keep the existing state transitions and the deferred CIS
> completion through iso_sock_recvmsg().
> 
> Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> Cc: stable@vger.kernel.org
> Assisted-by: GPT-6 Astra
> Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> ---
>  net/bluetooth/iso.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 7657c2a0abbf..8113367c796e 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -61,6 +61,7 @@ enum {
>  	BT_SK_BIG_SYNC,
>  	BT_SK_PA_SYNC,
>  	BT_SK_KILLED,
> +	BT_SK_CONNECTING,
>  };
>  
>  struct iso_pinfo {
> @@ -1269,17 +1270,26 @@ static int iso_sock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>  	    addr->sa_family != AF_BLUETOOTH)
>  		return -EINVAL;
>  
> -	if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND)
> -		return -EBADFD;
> +	lock_sock(sk);
>  
> -	if (sk->sk_type != SOCK_SEQPACKET)
> -		return -EINVAL;
> +	if ((sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) ||
> +	    test_bit(BT_SK_CONNECTING, &iso_pi(sk)->flags)) {
> +		err = -EBADFD;
> +		goto done;
> +	}
> +
> +	if (sk->sk_type != SOCK_SEQPACKET) {
> +		err = -EINVAL;
> +		goto done;
> +	}
>  
>  	/* Check if the address type is of LE type */
> -	if (!bdaddr_type_is_le(sa->iso_bdaddr_type))
> -		return -EINVAL;
> +	if (!bdaddr_type_is_le(sa->iso_bdaddr_type)) {
> +		err = -EINVAL;
> +		goto done;
> +	}
>  
> -	lock_sock(sk);
> +	set_bit(BT_SK_CONNECTING, &iso_pi(sk)->flags);
>  
>  	bacpy(&iso_pi(sk)->dst, &sa->iso_bdaddr);
>  	iso_pi(sk)->dst_type = sa->iso_bdaddr_type;
> @@ -1291,16 +1301,18 @@ static int iso_sock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>  	else
>  		err = iso_connect_bis(sk);
>  
> -	if (err)
> -		return err;
> -
>  	lock_sock(sk);
>  
> +	clear_bit(BT_SK_CONNECTING, &iso_pi(sk)->flags);
> +	if (err)
> +		goto done;
> +
>  	if (!test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
>  		err = bt_sock_wait_state(sk, BT_CONNECTED,
>  					 sock_sndtimeo(sk, flags & O_NONBLOCK));
>  	}
>  
> +done:
>  	release_sock(sk);
>  	return err;
>  }

-- 
Pauli Virtanen