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.
