Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4247-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240428054307.1178347-1-iam@sung-woo.kim> <CABBYNZKDouOrxG1XcSq2me0WW3A3yAbfdtonw2XJ54VZ8+Nbfg@mail.gmail.com>
 <CAJNyHpKzhFBJ3N0eF1x0icd7O1VkWbTA9k0Gkm8aCMonsagYwg@mail.gmail.com>
In-Reply-To: <CAJNyHpKzhFBJ3N0eF1x0icd7O1VkWbTA9k0Gkm8aCMonsagYwg@mail.gmail.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Wed, 1 May 2024 18:36:26 -0400
Message-ID: <CABBYNZ+Mr1c7UKa4Vey7Wmh-+WQaFU0P8AifBf7HDEb7G8PTJQ@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: L2CAP: Fix div-by-zero in l2cap_le_flowctl_init()
To: Sungwoo Kim <iam@sung-woo.kim>
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

Hi Sungwoo,

On Wed, May 1, 2024 at 6:23=E2=80=AFPM Sungwoo Kim <iam@sung-woo.kim> wrote=
:
>
> 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> =
wrote:
> > >
> > > Hello, could you review this bug and its patch?
> > >
> > > l2cap_le_flowctl_init() can cause both div-by-zero and an integer ove=
rflow.
> > >
> > > l2cap_le_flowctl_init()
> > >   chan->mps =3D min_t(u16, chan->imtu, chan->conn->mtu - L2CAP_HDR_SI=
ZE);
> > >   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 i=
s 4, it
> > > causes div-by-zero. If mtu is less than 4, it causes an integer overf=
low.
> >
> > 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?

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.


--=20
Luiz Augusto von Dentz