lkml.org 
[lkml]   [2024]   [May]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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@sung-woo.kim> wrote:
>
> Dear Luiz,
>
> On Mon, Apr 29, 2024 at 11:15 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Sungwoo,
> >
> > On Sun, Apr 28, 2024 at 1:43 AM 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 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

\
 
 \ /
  Last update: 2024-05-27 18:12    [W:0.065 / U:0.952 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site