Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 15.0 \(3693.40.0.1.81\))
Subject: Re: [PATCH] Bluetooth: ensure valid channel mode when creating l2cap
 conn on LE
From:   Marcel Holtmann <marcel@holtmann.org>
In-Reply-To: <20220112101731.77010-1-gav@thegavinli.com>
Date:   Wed, 19 Jan 2022 18:06:23 +0100
Cc:     Johan Hedberg <johan.hedberg@gmail.com>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        linux-bluetooth@vger.kernel.org, Gavin Li <git@thegavinli.com>
Content-Transfer-Encoding: 8BIT
Message-Id: <43EE8877-ACDA-4873-9723-FC8AE004F0A4@holtmann.org>
References: <20220112101731.77010-1-gav@thegavinli.com>
To:     gav@thegavinli.com
Precedence: bulk

Hi Gavin,

> After creating a socket(AF_INET, SOCK_STREAM, BTPROTO_L2CAP) socket and
> connect()'ing to a LE device with default settings (no setsockopt), upon
> the first sendmsg, the following BUG occurs because chan->mode==L2CAP_MODE_ERTM,
> causing l2cap_ertm_send() -> __set_retrans_timer() -> schedule_delayed_work()
> on l2cap_chan.retrans_timer, which was never initialized because
> l2cap_ertm_init() was never called to initialize it.
> 
>  Call Trace:
>   queue_delayed_work_on+0x36/0x40
>   l2cap_ertm_send.isra.0+0x14d/0x2d0 [bluetooth]
>   l2cap_tx+0x361/0x510 [bluetooth]
>   l2cap_chan_send+0xb26/0xb50 [bluetooth]
>   l2cap_sock_sendmsg+0xc9/0x100 [bluetooth]
>   sock_sendmsg+0x5e/0x60
>   sock_write_iter+0x97/0x100
>   new_sync_write+0x1d3/0x1f0
>   vfs_write+0x1b4/0x270
>   ksys_write+0xaf/0xe0
>   do_syscall_64+0x33/0x40
>   entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> This patch ensures that when connecting to a LE device, chan->mode will
> always be corrected to L2CAP_MODE_LE_FLOWCTL if it is invalid for LE.
> 
> Signed-off-by: Gavin Li <git@thegavinli.com>
> ---
> net/bluetooth/l2cap_sock.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 160c016a5dfb9..58c06ef32656c 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -78,6 +78,17 @@ static int l2cap_validate_le_psm(u16 psm)
> 	return 0;
> }
> 
> +static bool l2cap_mode_supports_le(u8 mode)
> +{
> +	switch (mode) {
> +		case L2CAP_MODE_LE_FLOWCTL:
> +		case L2CAP_MODE_EXT_FLOWCTL:
> +			return true;
> +		default:
> +			return false;
> +	}
> +}
> +

please use correct coding style.


> static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> {
> 	struct sock *sk = sock->sk;
> @@ -161,7 +172,7 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> 		break;
> 	}
> 
> -	if (chan->psm && bdaddr_type_is_le(chan->src_type))
> +	if (chan->psm && bdaddr_type_is_le(la.l2_bdaddr_type) && !l2cap_mode_supports_le(chan->mode))
> 		chan->mode = L2CAP_MODE_LE_FLOWCTL;
> 
> 	chan->state = BT_BOUND;
> @@ -240,7 +251,7 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr,
> 			return -EINVAL;
> 	}
> 
> -	if (chan->psm && bdaddr_type_is_le(chan->src_type) && !chan->mode)
> +	if (chan->psm && bdaddr_type_is_le(la.l2_bdaddr_type) && !l2cap_mode_supports_le(chan->mode))
> 		chan->mode = L2CAP_MODE_LE_FLOWCTL;

And you need to add a proper multi-line if clause here.

Regards

Marcel