Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4246-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240428054307.1178347-1-iam@sung-woo.kim> <CABBYNZKDouOrxG1XcSq2me0WW3A3yAbfdtonw2XJ54VZ8+Nbfg@mail.gmail.com>
In-Reply-To: <CABBYNZKDouOrxG1XcSq2me0WW3A3yAbfdtonw2XJ54VZ8+Nbfg@mail.gmail.com>
From: Sungwoo Kim <iam@sung-woo.kim>
Date: Wed, 1 May 2024 18:23:43 -0400
Message-ID: <CAJNyHpKzhFBJ3N0eF1x0icd7O1VkWbTA9k0Gkm8aCMonsagYwg@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: L2CAP: Fix div-by-zero in l2cap_le_flowctl_init()
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: daveti@purdue.edu, Marcel Holtmann <marcel@holtmann.org>, 
	Johan Hedberg <johan.hedberg@gmail.com>, linux-bluetooth@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Dear Luiz,

On Mon, Apr 29, 2024 at 11:15=E2=80=AFAM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sungwoo,
>
> On Sun, Apr 28, 2024 at 1:43=E2=80=AFAM Sungwoo Kim <iam@sung-woo.kim> wr=
ote:
> >
> > Hello, could you review this bug and its patch?
> >
> > l2cap_le_flowctl_init() can cause both div-by-zero and an integer overf=
low.
> >
> > 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, 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 overflo=
w.
>
> 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 =3D __le16_to_cpu(rp->le_mtu);
> >
> > l2cap_conn_add()
> >   conn->mtu =3D 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.