Hi Sungwoo, On Wed, May 1, 2024 at 12:32 AM Sungwoo Kim <iam@xxxxxxxxxxxx> wrote: > > 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 overflow. > > l2cap_le_flowctl_init() > chan->mps = min_t(u16, chan->imtu, chan->conn->mtu - L2CAP_HDR_SIZE); > chan->rx_credits = (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 = __le16_to_cpu(rp->le_mtu); > > l2cap_conn_add() > conn->mtu = 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/01/2014 > Workqueue: hci0 hci_rx_work > RIP: 0010:l2cap_le_flowctl_init+0x19e/0x3f0 net/bluetooth/l2cap_core.c:547 > Code: e8 17 17 0c 00 66 41 89 9f 84 00 00 00 bf 01 00 00 00 41 b8 02 00 00 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:0000000000000000 > 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@xxxxxxxxxxxx> > --- > 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 *hdev, 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_mtu) > + 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 at a > * time, we return -EBUSY if there is any connection attempt running. > */ > 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_mtu) > + 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_mtu, > hdev->acl_pkts, hdev->iso_mtu, hdev->iso_pkts); > > + if (hdev->le_mtu < HCI_MIN_LE_MTU || HCI_MAX_LE_MTU < hdev->le_mtu) > + 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 > -- Luiz Augusto von Dentz