linux-kernel - Re: [PATCH] Bluetooth: L2CAP: Fix div-by-zero in l2cap_le_flowctl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJNyHpKzhFBJ3N0eF1x0icd7O1VkWbTA9k0Gkm8aCMonsagYwg@mail.gmail.com>
Date: Wed, 1 May 2024 18:23:43 -0400
From: Sungwoo Kim <iam@...g-woo.kim>
To: Luiz Augusto von Dentz <luiz.dentz@...il.com>
Cc: daveti@...due.edu, Marcel Holtmann <marcel@...tmann.org>, 
	Johan Hedberg <johan.hedberg@...il.com>, linux-bluetooth@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Bluetooth: L2CAP: Fix div-by-zero in l2cap_le_flowctl_init()

Dear Luiz,

On Mon, Apr 29, 2024 at 11:15 AM Luiz Augusto von Dentz
<luiz.dentz@...il.com> wrote:
>
> Hi Sungwoo,
>
> On Sun, Apr 28, 2024 at 1:43 AM Sungwoo Kim <iam@...g-woo.kim> wrote:
> >
> > Hello, could you review this bug and its patch?
> >
> > 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, 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.
>
> That is because it is not valid to have hdev->le_mtu < 0x001b (the
> range is 0x001b to 0xffff), so we should really look into checking
> that conn->mtu is actually valid.
>
> > 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;
>
> Yeah this assignment is incorrect and in fact we don't do that if
> le_mtu is zero so we probably should do some checks e.g. le_mtu >
> 0x001a, or perhaps we need to move the MTU directly to hci_conn so it
> can check there are enough buffers to serve the link so we stop the
> connection procedure earlier.

Let's say we moved MTU directly to hci_conn and already checked enough
buffers at the creation of hcon.
Then, what should happen if hdev->le_mtu is updated? (by a new
le_read_buffer_size cmd)
Should hcon->mtu be synced with hdev->le_mtu? Or hcon->mtu can keep
its old value?

Best,
Sungwoo.