Re: [PATCH] Bluetooth: L2CAP: Fix div-by-zero in l2cap_le_flowctl_init()

[Sungwoo Kim]

Dear Luiz,

On Mon, Apr 29, 2024 at 11:15 AM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Sungwoo,
>
> On Sun, Apr 28, 2024 at 1:43 AM Sungwoo Kim <iam@xxxxxxxxxxxx> 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.