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@xxxxxxxxxxxx>
---
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
