Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4230-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240501042936.2579739-1-iam@sung-woo.kim>
In-Reply-To: <20240501042936.2579739-1-iam@sung-woo.kim>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Wed, 1 May 2024 09:56:32 -0400
Message-ID: <CABBYNZK-KtTg4XrwK0_FaTXT_=TfS5yD1=TF2a=Mq2ojRDs=Ag@mail.gmail.com>
Subject: Re: [PATCH v2] Bluetooth: L2CAP: Fix div-by-zero in l2cap_le_flowctl_init()
To: Sungwoo Kim <iam@sung-woo.kim>
Cc: daveti@purdue.edu, benquike@gmail.com, 
	Marcel Holtmann <marcel@holtmann.org>, Johan Hedberg <johan.hedberg@gmail.com>, 
	"David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, 
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, linux-bluetooth@vger.kernel.org, 
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Sungwoo,

On Wed, May 1, 2024 at 12:32=E2=80=AFAM Sungwoo Kim <iam@sung-woo.kim> wrot=
e:
>
> Hi Luiz, could you review this patch?
>
> This patch prevents a div-by-zero error and potential int overflow by
> adding a range check for MTU in hci_cc_le_read_buffer_size() and
> hci_cc_le_read_buffer_size_v2().
> Also, hci_connect_le() will refuse to allocate hcon if the MTU is not in
> the valid range.
>
> Bug description:
>
> l2cap_le_flowctl_init() can cause both div-by-zero and an integer overflo=
w.
>
> l2cap_le_flowctl_init()
>   chan->mps =3D min_t(u16, chan->imtu, chan->conn->mtu - L2CAP_HDR_SIZE);
>   chan->rx_credits =3D (chan->imtu / chan->mps) + 1;  <- div-by-zero
>
> Here, chan->conn->mtu could be less than or equal to L2CAP_HDR_SIZE (4).
> If mtu is 4, it causes div-by-zero. If mtu is less than 4, it causes an
> integer overflow.
>
> How mtu could have such low value:
>
> hci_cc_le_read_buffer_size()
>   hdev->le_mtu =3D __le16_to_cpu(rp->le_mtu);
>
> l2cap_conn_add()
>   conn->mtu =3D hcon->hdev->le_mtu;
>
> As shown, mtu is an input from an HCI device. So, any HCI device can
> set mtu value to any value, such as lower than 4.
> According to the spec v5.4 7.8.2 LE Read Buffer Size command, the value
> should be fall in [0x001b, 0xffff].
>
> Thank you,
> Sungwoo.
>
> divide error: 0000 [#1] PREEMPT SMP KASAN NOPTI
> CPU: 0 PID: 67 Comm: kworker/u5:0 Tainted: G        W          6.9.0-rc5+=
 #20
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/0=
1/2014
> Workqueue: hci0 hci_rx_work
> RIP: 0010:l2cap_le_flowctl_init+0x19e/0x3f0 net/bluetooth/l2cap_core.c:54=
7
> Code: e8 17 17 0c 00 66 41 89 9f 84 00 00 00 bf 01 00 00 00 41 b8 02 00 0=
0 00 4c 89 fe 4c 89 e2 89 d9 e8 27 17 0c 00 44 89 f0 31 d2 <66> f7 f3 89 c3=
 ff c3 4d 8d b7 88 00 00 00 4c 89 f0 48 c1 e8 03 42
> RSP: 0018:ffff88810bc0f858 EFLAGS: 00010246
> RAX: 00000000000002a0 RBX: 0000000000000000 RCX: dffffc0000000000
> RDX: 0000000000000000 RSI: ffff88810bc0f7c0 RDI: ffffc90002dcb66f
> RBP: ffff88810bc0f880 R08: aa69db2dda70ff01 R09: 0000ffaaaaaaaaaa
> R10: 0084000000ffaaaa R11: 0000000000000000 R12: ffff88810d65a084
> R13: dffffc0000000000 R14: 00000000000002a0 R15: ffff88810d65a000
> FS:  0000000000000000(0000) GS:ffff88811ac00000(0000) knlGS:0000000000000=
000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000020000100 CR3: 0000000103268003 CR4: 0000000000770ef0
> PKRU: 55555554
> Call Trace:
>  <TASK>
>  l2cap_le_connect_req net/bluetooth/l2cap_core.c:4902 [inline]
>  l2cap_le_sig_cmd net/bluetooth/l2cap_core.c:5420 [inline]
>  l2cap_le_sig_channel net/bluetooth/l2cap_core.c:5486 [inline]
>  l2cap_recv_frame+0xe59d/0x11710 net/bluetooth/l2cap_core.c:6809
>  l2cap_recv_acldata+0x544/0x10a0 net/bluetooth/l2cap_core.c:7506
>  hci_acldata_packet net/bluetooth/hci_core.c:3939 [inline]
>  hci_rx_work+0x5e5/0xb20 net/bluetooth/hci_core.c:4176
>  process_one_work kernel/workqueue.c:3254 [inline]
>  process_scheduled_works+0x90f/0x1530 kernel/workqueue.c:3335
>  worker_thread+0x926/0xe70 kernel/workqueue.c:3416
>  kthread+0x2e3/0x380 kernel/kthread.c:388
>  ret_from_fork+0x5c/0x90 arch/x86/kernel/process.c:147
>  ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
>  </TASK>
> Modules linked in:
> ---[ end trace 0000000000000000 ]---
>
> Signed-off-by: Sungwoo Kim <iam@sung-woo.kim>
> ---
> v1 -> v2:
> - Reply with an error code if a given MTU is not valid.
> - Refuse hcon allocation if MTU is not still valid.
>
>  include/net/bluetooth/hci.h | 6 ++++++
>  net/bluetooth/hci_conn.c    | 4 ++++
>  net/bluetooth/hci_event.c   | 6 ++++++
>  3 files changed, 16 insertions(+)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 5c12761cb..a7bc07e9c 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1666,6 +1666,12 @@ struct hci_cp_le_set_event_mask {
>         __u8     mask[8];
>  } __packed;
>
> +/* BLUETOOTH CORE SPECIFICATION Version 5.4 | Vol 4, Part E
> + * 7.8.2 LE Read Buffer Size command
> + */
> +#define HCI_MIN_LE_MTU 0x001b
> +#define HCI_MAX_LE_MTU 0xFFFF

Don't think we really need the MAX value here since it is that same as
the maximum field can assume so doing x > MAX is sort of useless as it
loops around if that happens.

>  #define HCI_OP_LE_READ_BUFFER_SIZE     0x2002
>  struct hci_rp_le_read_buffer_size {
>         __u8     status;
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 05346250f..0b86a7452 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -1277,6 +1277,10 @@ struct hci_conn *hci_connect_le(struct hci_dev *hd=
ev, bdaddr_t *dst,
>                 return ERR_PTR(-EOPNOTSUPP);
>         }
>
> +       /* Check the mtu is valid */
> +       if (hdev->le_mtu < HCI_MIN_LE_MTU || HCI_MAX_LE_MTU < hdev->le_mt=
u)
> +               return ERR_PTR(-ECONNREFUSED);
> +

That probably needs to be done on hci_conn_add if we want to capture
both incoming and outgoing.

>         /* Since the controller supports only one LE connection attempt a=
t a
>          * time, we return -EBUSY if there is any connection attempt runn=
ing.
>          */
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 4a27e4a17..a8563cbe2 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1263,6 +1263,9 @@ static u8 hci_cc_le_read_buffer_size(struct hci_dev=
 *hdev, void *data,
>
>         BT_DBG("%s le mtu %d:%d", hdev->name, hdev->le_mtu, hdev->le_pkts=
);
>
> +       if (hdev->le_mtu < HCI_MIN_LE_MTU || HCI_MAX_LE_MTU < hdev->le_mt=
u)
> +               return HCI_ERROR_INVALID_PARAMETERS;

Value 0x00 means 'No dedicated LE Buffer exists. Use the
HCI_Read_Buffer_Size command.' so it is not invalid to return it.

>         return rp->status;
>  }
>
> @@ -3821,6 +3824,9 @@ static u8 hci_cc_le_read_buffer_size_v2(struct hci_=
dev *hdev, void *data,
>         BT_DBG("%s acl mtu %d:%d iso mtu %d:%d", hdev->name, hdev->acl_mt=
u,
>                hdev->acl_pkts, hdev->iso_mtu, hdev->iso_pkts);
>
> +       if (hdev->le_mtu < HCI_MIN_LE_MTU || HCI_MAX_LE_MTU < hdev->le_mt=
u)
> +               return HCI_ERROR_INVALID_PARAMETERS;

Ditto, this should really be:

if (hdev->le_mtu && hdev->le_mtu < HCI_MIN_LE_MTU)

>         return rp->status;
>  }
>
> --
> 2.34.1
>


--=20
Luiz Augusto von Dentz