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] [day] [month] [year] [list]

Message-ID: <CABBYNZ+Mr1c7UKa4Vey7Wmh-+WQaFU0P8AifBf7HDEb7G8PTJQ@mail.gmail.com>
Date: Wed, 1 May 2024 18:36:26 -0400
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Sungwoo Kim <iam@...g-woo.kim>
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()

Hi Sungwoo,

On Wed, May 1, 2024 at 6:23 PM Sungwoo Kim <iam@...g-woo.kim> wrote:
>
> 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?

What now, why would we read it again? These commands are only suppose
to be send during init phase and if you do this sort of thing as an
event without a command then it is also against the spec and we shall
probably just ignore it if there is no command pending.

> Best,
> Sungwoo.



-- 
Luiz Augusto von Dentz