| Message ID | 20240428054307.1178347-1-iam@sung-woo.kim (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bluetooth: L2CAP: Fix div-by-zero in l2cap_le_flowctl_init() | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | warning | WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?) #87: Here, mtu could be less than or equal to L2CAP_HDR_SIZE (4). If mtu is 4, it total: 0 errors, 1 warnings, 0 checks, 177 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13645866.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. |
| tedd_an/GitLint | fail | WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 40: B1 Line exceeds max length (199>80): "Code: e8 17 17 0c 00 66 41 89 9f 84 00 00 00 bf 01 00 00 00 41 b8 02 00 00 00 4c 89 fe 4c 89 e2 89 d9 e8 27 17 0c 00 44 89 f0 31 d2 <66> f7 f3 89 c3 ff c3 4d 8d b7 88 00 00 00 4c 89 f0 48 c1 e8 03 42" |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/CheckSmatch | fail | CheckSparse: FAIL: Segmentation fault (core dumped) make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139 make[4]: *** Deleting file 'net/bluetooth/hci_core.o' make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2 make[2]: *** [scripts/Makefile.build:485: net] Error 2 make[2]: *** Waiting for unfinished jobs.... Segmentation fault (core dumped) make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139 make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o' make[4]: *** Waiting for unfinished jobs.... Segmentation fault (core dumped) make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bpa10x.o] Error 139 make[4]: *** Deleting file 'drivers/bluetooth/bpa10x.o' make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2 make[2]: *** [scripts/Makefile.build:485: drivers] Error 2 make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2 make: *** [Makefile:240: __sub-make] Error 2 |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | fail | TestRunner_iso-tester: Total: 122, Passed: 121 (99.2%), Failed: 1, Not Run: 0 |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
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. > As shown, mtu is an input from an HCI device. So, any HCI device can > set mtu value to any value, such as lower than 4. > > To fix this, this patch adds a validation before subtractions. If MTU is > too small, the corresponding value will set by 0, and a warning message > will show up. > > However, I'm not sure that 0-ing the value is the best. It'd be great if > you could comment on this. > > Thank you, > Sungwoo. > > divide error: 0000 [#1] PREEMPT SMP KASAN NOPTI > CPU: 0 PID: 67 Comm: kworker/u5:0 Tainted: G W 6.9.0-rc5+ #20 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014 > Workqueue: hci0 hci_rx_work > RIP: 0010:l2cap_le_flowctl_init+0x19e/0x3f0 net/bluetooth/l2cap_core.c:547 > Code: e8 17 17 0c 00 66 41 89 9f 84 00 00 00 bf 01 00 00 00 41 b8 02 00 00 00 4c 89 fe 4c 89 e2 89 d9 e8 27 17 0c 00 44 89 f0 31 d2 <66> f7 f3 89 c3 ff c3 4d 8d b7 88 00 00 00 4c 89 f0 48 c1 e8 03 42 > RSP: 0018:ffff88810bc0f858 EFLAGS: 00010246 > RAX: 00000000000002a0 RBX: 0000000000000000 RCX: dffffc0000000000 > RDX: 0000000000000000 RSI: ffff88810bc0f7c0 RDI: ffffc90002dcb66f > RBP: ffff88810bc0f880 R08: aa69db2dda70ff01 R09: 0000ffaaaaaaaaaa > R10: 0084000000ffaaaa R11: 0000000000000000 R12: ffff88810d65a084 > R13: dffffc0000000000 R14: 00000000000002a0 R15: ffff88810d65a000 > FS: 0000000000000000(0000) GS:ffff88811ac00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000000020000100 CR3: 0000000103268003 CR4: 0000000000770ef0 > PKRU: 55555554 > Call Trace: > <TASK> > l2cap_le_connect_req net/bluetooth/l2cap_core.c:4902 [inline] > l2cap_le_sig_cmd net/bluetooth/l2cap_core.c:5420 [inline] > l2cap_le_sig_channel net/bluetooth/l2cap_core.c:5486 [inline] > l2cap_recv_frame+0xe59d/0x11710 net/bluetooth/l2cap_core.c:6809 > l2cap_recv_acldata+0x544/0x10a0 net/bluetooth/l2cap_core.c:7506 > hci_acldata_packet net/bluetooth/hci_core.c:3939 [inline] > hci_rx_work+0x5e5/0xb20 net/bluetooth/hci_core.c:4176 > process_one_work kernel/workqueue.c:3254 [inline] > process_scheduled_works+0x90f/0x1530 kernel/workqueue.c:3335 > worker_thread+0x926/0xe70 kernel/workqueue.c:3416 > kthread+0x2e3/0x380 kernel/kthread.c:388 > ret_from_fork+0x5c/0x90 arch/x86/kernel/process.c:147 > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244 > </TASK> > Modules linked in: > ---[ end trace 0000000000000000 ]--- > > Signed-off-by: Sungwoo Kim <iam@sung-woo.kim> > --- > net/bluetooth/l2cap_core.c | 94 +++++++++++++++++++++++++++++--------- > 1 file changed, 73 insertions(+), 21 deletions(-) > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index 84fc70862..472ddfb2e 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -541,10 +541,17 @@ static void l2cap_le_flowctl_init(struct l2cap_chan *chan, u16 tx_credits) > chan->sdu_last_frag = NULL; > chan->sdu_len = 0; > chan->tx_credits = tx_credits; > - /* Derive MPS from connection MTU to stop HCI fragmentation */ > - chan->mps = min_t(u16, chan->imtu, chan->conn->mtu - L2CAP_HDR_SIZE); > - /* Give enough credits for a full packet */ > - chan->rx_credits = (chan->imtu / chan->mps) + 1; > + > + if (chan->conn->mtu < L2CAP_HDR_SIZE) { > + BT_WARN("mtu is too small (mtu %d < %d)", chan->conn->mtu, L2CAP_HDR_SIZE); > + chan->mps = 0; > + chan->rx_credits = 0; > + } else { > + /* Derive MPS from connection MTU to stop HCI fragmentation */ > + chan->mps = min_t(u16, chan->imtu, chan->conn->mtu - L2CAP_HDR_SIZE); > + /* Give enough credits for a full packet */ > + chan->rx_credits = (chan->imtu / chan->mps) + 1; > + } > > skb_queue_head_init(&chan->tx_q); > } > @@ -2222,7 +2229,12 @@ static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, > BT_DBG("chan %p psm 0x%2.2x len %zu", chan, > __le16_to_cpu(chan->psm), len); > > - count = min_t(unsigned int, (conn->mtu - hlen), len); > + if (conn->mtu < hlen) { > + count = 0; > + BT_WARN("mtu is too small (mtu %d < len %d)", conn->mtu, hlen); > + } else { > + count = min_t(unsigned int, (conn->mtu - hlen), len); > + } > > skb = chan->ops->alloc_skb(chan, hlen, count, > msg->msg_flags & MSG_DONTWAIT); > @@ -2253,7 +2265,12 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, > > BT_DBG("chan %p len %zu", chan, len); > > - count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len); > + if (conn->mtu < L2CAP_HDR_SIZE) { > + BT_WARN("mtu is too small (mtu %d < %d)", conn->mtu, L2CAP_HDR_SIZE); > + count = 0; > + } else { > + count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len); > + } > > skb = chan->ops->alloc_skb(chan, L2CAP_HDR_SIZE, count, > msg->msg_flags & MSG_DONTWAIT); > @@ -2295,7 +2312,12 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, > if (chan->fcs == L2CAP_FCS_CRC16) > hlen += L2CAP_FCS_SIZE; > > - count = min_t(unsigned int, (conn->mtu - hlen), len); > + if (conn->mtu < hlen) { > + BT_WARN("mtu is too small (mtu %d < len %d)", conn->mtu, hlen); > + count = 0; > + } else { > + count = min_t(unsigned int, (conn->mtu - hlen), len); > + } > > skb = chan->ops->alloc_skb(chan, hlen, count, > msg->msg_flags & MSG_DONTWAIT); > @@ -2412,7 +2434,12 @@ static struct sk_buff *l2cap_create_le_flowctl_pdu(struct l2cap_chan *chan, > if (sdulen) > hlen += L2CAP_SDULEN_SIZE; > > - count = min_t(unsigned int, (conn->mtu - hlen), len); > + if (conn->mtu < hlen) { > + BT_WARN("mtu is too small (mtu %d < len %d)", conn->mtu, hlen); > + count = 0; > + } else { > + count = min_t(unsigned int, (conn->mtu - hlen), len); > + } > > skb = chan->ops->alloc_skb(chan, hlen, count, > msg->msg_flags & MSG_DONTWAIT); > @@ -3225,6 +3252,7 @@ static int l2cap_build_conf_req(struct l2cap_chan *chan, void *data, size_t data > void *ptr = req->data; > void *endptr = data + data_size; > u16 size; > + int hlen; > > BT_DBG("chan %p", chan); > > @@ -3275,14 +3303,19 @@ static int l2cap_build_conf_req(struct l2cap_chan *chan, void *data, size_t data > break; > > case L2CAP_MODE_ERTM: > + hlen = L2CAP_EXT_HDR_SIZE + L2CAP_SDULEN_SIZE + L2CAP_FCS_SIZE; > rfc.mode = L2CAP_MODE_ERTM; > rfc.max_transmit = chan->max_tx; > > __l2cap_set_ertm_timeouts(chan, &rfc); > > - size = min_t(u16, L2CAP_DEFAULT_MAX_PDU_SIZE, chan->conn->mtu - > - L2CAP_EXT_HDR_SIZE - L2CAP_SDULEN_SIZE - > - L2CAP_FCS_SIZE); > + if (chan->conn->mtu < hlen) { > + BT_WARN("mtu is too small (mtu %d < len %d)", chan->conn->mtu, hlen); > + size = 0; > + } else { > + size = min_t(u16, L2CAP_DEFAULT_MAX_PDU_SIZE, chan->conn->mtu - hlen); > + } > + > rfc.max_pdu_size = cpu_to_le16(size); > > l2cap_txwin_setup(chan); > @@ -3310,6 +3343,7 @@ static int l2cap_build_conf_req(struct l2cap_chan *chan, void *data, size_t data > break; > > case L2CAP_MODE_STREAMING: > + hlen = L2CAP_EXT_HDR_SIZE + L2CAP_SDULEN_SIZE + L2CAP_FCS_SIZE; > l2cap_txwin_setup(chan); > rfc.mode = L2CAP_MODE_STREAMING; > rfc.txwin_size = 0; > @@ -3317,9 +3351,12 @@ static int l2cap_build_conf_req(struct l2cap_chan *chan, void *data, size_t data > rfc.retrans_timeout = 0; > rfc.monitor_timeout = 0; > > - size = min_t(u16, L2CAP_DEFAULT_MAX_PDU_SIZE, chan->conn->mtu - > - L2CAP_EXT_HDR_SIZE - L2CAP_SDULEN_SIZE - > - L2CAP_FCS_SIZE); > + if (chan->conn->mtu < hlen) { > + BT_WARN("mtu is too small (mtu %d < len %d)", chan->conn->mtu, hlen); > + size = 0; > + } else { > + size = min_t(u16, L2CAP_DEFAULT_MAX_PDU_SIZE, chan->conn->mtu - hlen); > + } > rfc.max_pdu_size = cpu_to_le16(size); > > l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc), > @@ -3351,7 +3388,7 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data > void *endptr = data + data_size; > void *req = chan->conf_req; > int len = chan->conf_len; > - int type, hint, olen; > + int type, hint, olen, hlen; > unsigned long val; > struct l2cap_conf_rfc rfc = { .mode = L2CAP_MODE_BASIC }; > struct l2cap_conf_efs efs; > @@ -3496,6 +3533,7 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data > break; > > case L2CAP_MODE_ERTM: > + hlen = L2CAP_EXT_HDR_SIZE + L2CAP_SDULEN_SIZE + L2CAP_FCS_SIZE; > if (!test_bit(CONF_EWS_RECV, &chan->conf_state)) > chan->remote_tx_win = rfc.txwin_size; > else > @@ -3503,9 +3541,15 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data > > chan->remote_max_tx = rfc.max_transmit; > > - size = min_t(u16, le16_to_cpu(rfc.max_pdu_size), > - chan->conn->mtu - L2CAP_EXT_HDR_SIZE - > - L2CAP_SDULEN_SIZE - L2CAP_FCS_SIZE); > + if (chan->conn->mtu < hlen) { > + BT_WARN("mtu is too small (mtu %d < len %d)", > + chan->conn->mtu, hlen); > + size = 0; > + } else { > + size = min_t(u16, le16_to_cpu(rfc.max_pdu_size), > + chan->conn->mtu - hlen); > + } > + > rfc.max_pdu_size = cpu_to_le16(size); > chan->remote_mps = size; > > @@ -3534,9 +3578,17 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data > break; > > case L2CAP_MODE_STREAMING: > - size = min_t(u16, le16_to_cpu(rfc.max_pdu_size), > - chan->conn->mtu - L2CAP_EXT_HDR_SIZE - > - L2CAP_SDULEN_SIZE - L2CAP_FCS_SIZE); > + hlen = L2CAP_EXT_HDR_SIZE + L2CAP_SDULEN_SIZE + L2CAP_FCS_SIZE; > + > + if (chan->conn->mtu < hlen) { > + BT_WARN("mtu is too small (mtu %d < len %d)", > + chan->conn->mtu, hlen); > + size = 0; > + } else { > + size = min_t(u16, le16_to_cpu(rfc.max_pdu_size), > + chan->conn->mtu - hlen); > + } > + > rfc.max_pdu_size = cpu_to_le16(size); > chan->remote_mps = size; > > -- > 2.34.1 >
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? Best, Sungwoo.
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.
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index 84fc70862..472ddfb2e 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -541,10 +541,17 @@ static void l2cap_le_flowctl_init(struct l2cap_chan *chan, u16 tx_credits) chan->sdu_last_frag = NULL; chan->sdu_len = 0; chan->tx_credits = tx_credits; - /* Derive MPS from connection MTU to stop HCI fragmentation */ - chan->mps = min_t(u16, chan->imtu, chan->conn->mtu - L2CAP_HDR_SIZE); - /* Give enough credits for a full packet */ - chan->rx_credits = (chan->imtu / chan->mps) + 1; + + if (chan->conn->mtu < L2CAP_HDR_SIZE) { + BT_WARN("mtu is too small (mtu %d < %d)", chan->conn->mtu, L2CAP_HDR_SIZE); + chan->mps = 0; + chan->rx_credits = 0; + } else { + /* Derive MPS from connection MTU to stop HCI fragmentation */ + chan->mps = min_t(u16, chan->imtu, chan->conn->mtu - L2CAP_HDR_SIZE); + /* Give enough credits for a full packet */ + chan->rx_credits = (chan->imtu / chan->mps) + 1; + } skb_queue_head_init(&chan->tx_q); } @@ -2222,7 +2229,12 @@ static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan, BT_DBG("chan %p psm 0x%2.2x len %zu", chan, __le16_to_cpu(chan->psm), len); - count = min_t(unsigned int, (conn->mtu - hlen), len); + if (conn->mtu < hlen) { + count = 0; + BT_WARN("mtu is too small (mtu %d < len %d)", conn->mtu, hlen); + } else { + count = min_t(unsigned int, (conn->mtu - hlen), len); + } skb = chan->ops->alloc_skb(chan, hlen, count, msg->msg_flags & MSG_DONTWAIT); @@ -2253,7 +2265,12 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan, BT_DBG("chan %p len %zu", chan, len); - count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len); + if (conn->mtu < L2CAP_HDR_SIZE) { + BT_WARN("mtu is too small (mtu %d < %d)", conn->mtu, L2CAP_HDR_SIZE); + count = 0; + } else { + count = min_t(unsigned int, (conn->mtu - L2CAP_HDR_SIZE), len); + } skb = chan->ops->alloc_skb(chan, L2CAP_HDR_SIZE, count, msg->msg_flags & MSG_DONTWAIT); @@ -2295,7 +2312,12 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan, if (chan->fcs == L2CAP_FCS_CRC16) hlen += L2CAP_FCS_SIZE; - count = min_t(unsigned int, (conn->mtu - hlen), len); + if (conn->mtu < hlen) { + BT_WARN("mtu is too small (mtu %d < len %d)", conn->mtu, hlen); + count = 0; + } else { + count = min_t(unsigned int, (conn->mtu - hlen), len); + } skb = chan->ops->alloc_skb(chan, hlen, count, msg->msg_flags & MSG_DONTWAIT); @@ -2412,7 +2434,12 @@ static struct sk_buff *l2cap_create_le_flowctl_pdu(struct l2cap_chan *chan, if (sdulen) hlen += L2CAP_SDULEN_SIZE; - count = min_t(unsigned int, (conn->mtu - hlen), len); + if (conn->mtu < hlen) { + BT_WARN("mtu is too small (mtu %d < len %d)", conn->mtu, hlen); + count = 0; + } else { + count = min_t(unsigned int, (conn->mtu - hlen), len); + } skb = chan->ops->alloc_skb(chan, hlen, count, msg->msg_flags & MSG_DONTWAIT); @@ -3225,6 +3252,7 @@ static int l2cap_build_conf_req(struct l2cap_chan *chan, void *data, size_t data void *ptr = req->data; void *endptr = data + data_size; u16 size; + int hlen; BT_DBG("chan %p", chan); @@ -3275,14 +3303,19 @@ static int l2cap_build_conf_req(struct l2cap_chan *chan, void *data, size_t data break; case L2CAP_MODE_ERTM: + hlen = L2CAP_EXT_HDR_SIZE + L2CAP_SDULEN_SIZE + L2CAP_FCS_SIZE; rfc.mode = L2CAP_MODE_ERTM; rfc.max_transmit = chan->max_tx; __l2cap_set_ertm_timeouts(chan, &rfc); - size = min_t(u16, L2CAP_DEFAULT_MAX_PDU_SIZE, chan->conn->mtu - - L2CAP_EXT_HDR_SIZE - L2CAP_SDULEN_SIZE - - L2CAP_FCS_SIZE); + if (chan->conn->mtu < hlen) { + BT_WARN("mtu is too small (mtu %d < len %d)", chan->conn->mtu, hlen); + size = 0; + } else { + size = min_t(u16, L2CAP_DEFAULT_MAX_PDU_SIZE, chan->conn->mtu - hlen); + } + rfc.max_pdu_size = cpu_to_le16(size); l2cap_txwin_setup(chan); @@ -3310,6 +3343,7 @@ static int l2cap_build_conf_req(struct l2cap_chan *chan, void *data, size_t data break; case L2CAP_MODE_STREAMING: + hlen = L2CAP_EXT_HDR_SIZE + L2CAP_SDULEN_SIZE + L2CAP_FCS_SIZE; l2cap_txwin_setup(chan); rfc.mode = L2CAP_MODE_STREAMING; rfc.txwin_size = 0; @@ -3317,9 +3351,12 @@ static int l2cap_build_conf_req(struct l2cap_chan *chan, void *data, size_t data rfc.retrans_timeout = 0; rfc.monitor_timeout = 0; - size = min_t(u16, L2CAP_DEFAULT_MAX_PDU_SIZE, chan->conn->mtu - - L2CAP_EXT_HDR_SIZE - L2CAP_SDULEN_SIZE - - L2CAP_FCS_SIZE); + if (chan->conn->mtu < hlen) { + BT_WARN("mtu is too small (mtu %d < len %d)", chan->conn->mtu, hlen); + size = 0; + } else { + size = min_t(u16, L2CAP_DEFAULT_MAX_PDU_SIZE, chan->conn->mtu - hlen); + } rfc.max_pdu_size = cpu_to_le16(size); l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc), @@ -3351,7 +3388,7 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data void *endptr = data + data_size; void *req = chan->conf_req; int len = chan->conf_len; - int type, hint, olen; + int type, hint, olen, hlen; unsigned long val; struct l2cap_conf_rfc rfc = { .mode = L2CAP_MODE_BASIC }; struct l2cap_conf_efs efs; @@ -3496,6 +3533,7 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data break; case L2CAP_MODE_ERTM: + hlen = L2CAP_EXT_HDR_SIZE + L2CAP_SDULEN_SIZE + L2CAP_FCS_SIZE; if (!test_bit(CONF_EWS_RECV, &chan->conf_state)) chan->remote_tx_win = rfc.txwin_size; else @@ -3503,9 +3541,15 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data chan->remote_max_tx = rfc.max_transmit; - size = min_t(u16, le16_to_cpu(rfc.max_pdu_size), - chan->conn->mtu - L2CAP_EXT_HDR_SIZE - - L2CAP_SDULEN_SIZE - L2CAP_FCS_SIZE); + if (chan->conn->mtu < hlen) { + BT_WARN("mtu is too small (mtu %d < len %d)", + chan->conn->mtu, hlen); + size = 0; + } else { + size = min_t(u16, le16_to_cpu(rfc.max_pdu_size), + chan->conn->mtu - hlen); + } + rfc.max_pdu_size = cpu_to_le16(size); chan->remote_mps = size; @@ -3534,9 +3578,17 @@ static int l2cap_parse_conf_req(struct l2cap_chan *chan, void *data, size_t data break; case L2CAP_MODE_STREAMING: - size = min_t(u16, le16_to_cpu(rfc.max_pdu_size), - chan->conn->mtu - L2CAP_EXT_HDR_SIZE - - L2CAP_SDULEN_SIZE - L2CAP_FCS_SIZE); + hlen = L2CAP_EXT_HDR_SIZE + L2CAP_SDULEN_SIZE + L2CAP_FCS_SIZE; + + if (chan->conn->mtu < hlen) { + BT_WARN("mtu is too small (mtu %d < len %d)", + chan->conn->mtu, hlen); + size = 0; + } else { + size = min_t(u16, le16_to_cpu(rfc.max_pdu_size), + chan->conn->mtu - hlen); + } + rfc.max_pdu_size = cpu_to_le16(size); chan->remote_mps = size;
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. 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; As shown, mtu is an input from an HCI device. So, any HCI device can set mtu value to any value, such as lower than 4. To fix this, this patch adds a validation before subtractions. If MTU is too small, the corresponding value will set by 0, and a warning message will show up. However, I'm not sure that 0-ing the value is the best. It'd be great if you could comment on this. Thank you, Sungwoo. divide error: 0000 [#1] PREEMPT SMP KASAN NOPTI CPU: 0 PID: 67 Comm: kworker/u5:0 Tainted: G W 6.9.0-rc5+ #20 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014 Workqueue: hci0 hci_rx_work RIP: 0010:l2cap_le_flowctl_init+0x19e/0x3f0 net/bluetooth/l2cap_core.c:547 Code: e8 17 17 0c 00 66 41 89 9f 84 00 00 00 bf 01 00 00 00 41 b8 02 00 00 00 4c 89 fe 4c 89 e2 89 d9 e8 27 17 0c 00 44 89 f0 31 d2 <66> f7 f3 89 c3 ff c3 4d 8d b7 88 00 00 00 4c 89 f0 48 c1 e8 03 42 RSP: 0018:ffff88810bc0f858 EFLAGS: 00010246 RAX: 00000000000002a0 RBX: 0000000000000000 RCX: dffffc0000000000 RDX: 0000000000000000 RSI: ffff88810bc0f7c0 RDI: ffffc90002dcb66f RBP: ffff88810bc0f880 R08: aa69db2dda70ff01 R09: 0000ffaaaaaaaaaa R10: 0084000000ffaaaa R11: 0000000000000000 R12: ffff88810d65a084 R13: dffffc0000000000 R14: 00000000000002a0 R15: ffff88810d65a000 FS: 0000000000000000(0000) GS:ffff88811ac00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000020000100 CR3: 0000000103268003 CR4: 0000000000770ef0 PKRU: 55555554 Call Trace: <TASK> l2cap_le_connect_req net/bluetooth/l2cap_core.c:4902 [inline] l2cap_le_sig_cmd net/bluetooth/l2cap_core.c:5420 [inline] l2cap_le_sig_channel net/bluetooth/l2cap_core.c:5486 [inline] l2cap_recv_frame+0xe59d/0x11710 net/bluetooth/l2cap_core.c:6809 l2cap_recv_acldata+0x544/0x10a0 net/bluetooth/l2cap_core.c:7506 hci_acldata_packet net/bluetooth/hci_core.c:3939 [inline] hci_rx_work+0x5e5/0xb20 net/bluetooth/hci_core.c:4176 process_one_work kernel/workqueue.c:3254 [inline] process_scheduled_works+0x90f/0x1530 kernel/workqueue.c:3335 worker_thread+0x926/0xe70 kernel/workqueue.c:3416 kthread+0x2e3/0x380 kernel/kthread.c:388 ret_from_fork+0x5c/0x90 arch/x86/kernel/process.c:147 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244 </TASK> Modules linked in: ---[ end trace 0000000000000000 ]--- Signed-off-by: Sungwoo Kim <iam@sung-woo.kim> --- net/bluetooth/l2cap_core.c | 94 +++++++++++++++++++++++++++++--------- 1 file changed, 73 insertions(+), 21 deletions(-)