mbox series

[v2,0/1] Bluetooth: ISO: Allow binding a PA sync socket

Message ID 20231018143435.3388-1-iulia.tanasescu@nxp.com
Headers show
Series Bluetooth: ISO: Allow binding a PA sync socket | expand

Message

Iulia Tanasescu Oct. 18, 2023, 2:34 p.m. UTC 
A Broadcast Sink has no way to know how many BISes are available for
sync or their indexes, before establishing PA sync and reading the base
struct.

PA sync is established by calling listen with DEFER_SETUP and accepting
a PA sync socket.

This patch makes it possible to bind a PA sync socket to a number of
BISes before issuing the BIG Create Sync command.

This patch version fixes review comments.

Iulia Tanasescu (1):
  Bluetooth: ISO: Allow binding a PA sync socket

 net/bluetooth/iso.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)


base-commit: 2fddfadb0478a11952d4a3d543273f28d2c23c03

Comments

Pauli Virtanen Oct. 18, 2023, 5:05 p.m. UTC | #1 
Hi,

ke, 2023-10-18 kello 17:34 +0300, Iulia Tanasescu kirjoitti:
> This makes it possible to bind a PA sync socket to a number of BISes
> before issuing the BIG Create Sync command.
> 
> Signed-off-by: Iulia Tanasescu <iulia.tanasescu@nxp.com>
> ---
>  net/bluetooth/iso.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 07b80e97aead..f20238c4702f 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -813,6 +813,37 @@ static int iso_sock_bind_bc(struct socket *sock, struct sockaddr *addr,
>  	return 0;
>  }
>  
> +static int iso_sock_bind_pa_sk(struct sock *sk, struct sockaddr_iso *sa,
> +			       int addr_len)
> +{
> +	int err = 0;
> +
> +	if (sk->sk_type != SOCK_SEQPACKET) {
> +		err = -EINVAL;
> +		goto done;
> +	}
> +
> +	if (addr_len <= sizeof(*sa)) {
> +		err = -EINVAL;
> +		goto done;
> +	}
> +

This does not seem to check addr_len is big enough, sizeof(*sa) won't
count the sa->iso_bc flexible array member. 

That sa->iso_bc->bc_num_bis <= ISO_MAX_NUM_BIS is not checked, so
memcpy may write out of bounds.

The values in sa come from user, so may be invalid.

iso_sock_bind_bc seems to have similar issue.

Sorry for second round comments.

> +	iso_pi(sk)->bc_num_bis = sa->iso_bc->bc_num_bis;
> +
> +	for (int i = 0; i < iso_pi(sk)->bc_num_bis; i++)
> +		if (sa->iso_bc->bc_bis[i] < 0x01 ||
> +		    sa->iso_bc->bc_bis[i] > 0x1f) {
> +			err = -EINVAL;
> +			goto done;
> +		}
> +
> +	memcpy(iso_pi(sk)->bc_bis, sa->iso_bc->bc_bis,
> +	       iso_pi(sk)->bc_num_bis);
> +
> +done:
> +	return err;
> +}
> +
>  static int iso_sock_bind(struct socket *sock, struct sockaddr *addr,
>  			 int addr_len)
>  {
> @@ -828,6 +859,15 @@ static int iso_sock_bind(struct socket *sock, struct sockaddr *addr,
>  
>  	lock_sock(sk);
>  
> +	/* Allow the user to bind a PA sync socket to a number
> +	 * of BISes to sync to.
> +	 */
> +	if (sk->sk_state == BT_CONNECT2 &&
> +	    test_bit(BT_SK_PA_SYNC, &iso_pi(sk)->flags)) {
> +		err = iso_sock_bind_pa_sk(sk, sa, addr_len);
> +		goto done;
> +	}
> +
>  	if (sk->sk_state != BT_OPEN) {
>  		err = -EBADFD;
>  		goto done;