* [bug report] cxgbit: add files for cxgbit.ko
@ 2018-01-23 14:05 Dan Carpenter
2018-01-29 14:49 ` Varun Prakash
2018-01-30 2:38 ` Dan Carpenter
0 siblings, 2 replies; 4+ messages in thread
From: Dan Carpenter @ 2018-01-23 14:05 UTC (permalink / raw)
To: target-devel
Hello Varun Prakash,
The patch 9730ffcb8957: "cxgbit: add files for cxgbit.ko" from Apr
20, 2016, leads to the following static checker warning:
drivers/target/iscsi/cxgbit/cxgbit_target.c:1443 cxgbit_lro_skb_merge()
error: buffer overflow 'ssi->frags' 17 <= 255
drivers/target/iscsi/cxgbit/cxgbit_target.c
1425 static void
1426 cxgbit_lro_skb_merge(struct cxgbit_sock *csk, struct sk_buff *skb, u8 pdu_idx)
1427 {
1428 struct sk_buff *hskb = csk->lro_hskb;
1429 struct cxgbit_lro_pdu_cb *hpdu_cb = cxgbit_skb_lro_pdu_cb(hskb, 0);
1430 struct cxgbit_lro_pdu_cb *pdu_cb = cxgbit_skb_lro_pdu_cb(skb, pdu_idx);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Smatch marks pdu_cb as tainted because it comes from skb->data
1431 struct skb_shared_info *hssi = skb_shinfo(hskb);
1432 struct skb_shared_info *ssi = skb_shinfo(skb);
1433 unsigned int len = 0;
1434
1435 if (pdu_cb->flags & PDUCBF_RX_HDR) {
1436 u8 hfrag_idx = hssi->nr_frags;
1437
1438 hpdu_cb->flags |= pdu_cb->flags;
1439 hpdu_cb->seq = pdu_cb->seq;
1440 hpdu_cb->hdr = pdu_cb->hdr;
1441 hpdu_cb->hlen = pdu_cb->hlen;
1442
1443 memcpy(&hssi->frags[hfrag_idx], &ssi->frags[pdu_cb->hfrag_idx],
^^^^^^^^^^^^^^^^^
how do we know this is within bounds?
1444 sizeof(skb_frag_t));
1445
1446 get_page(skb_frag_page(&hssi->frags[hfrag_idx]));
1447 hssi->nr_frags++;
1448 hpdu_cb->frags++;
1449 hpdu_cb->hfrag_idx = hfrag_idx;
1450
1451 len = hssi->frags[hfrag_idx].size;
1452 hskb->len += len;
1453 hskb->data_len += len;
1454 hskb->truesize += len;
1455 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] cxgbit: add files for cxgbit.ko
2018-01-23 14:05 [bug report] cxgbit: add files for cxgbit.ko Dan Carpenter
@ 2018-01-29 14:49 ` Varun Prakash
2018-01-30 2:38 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Varun Prakash @ 2018-01-29 14:49 UTC (permalink / raw)
To: target-devel
On Tue, Jan 23, 2018 at 05:05:50PM +0300, Dan Carpenter wrote:
> Hello Varun Prakash,
>
> The patch 9730ffcb8957: "cxgbit: add files for cxgbit.ko" from Apr
> 20, 2016, leads to the following static checker warning:
>
> drivers/target/iscsi/cxgbit/cxgbit_target.c:1443 cxgbit_lro_skb_merge()
> error: buffer overflow 'ssi->frags' 17 <= 255
>
> drivers/target/iscsi/cxgbit/cxgbit_target.c
> 1425 static void
> 1426 cxgbit_lro_skb_merge(struct cxgbit_sock *csk, struct sk_buff *skb, u8 pdu_idx)
> 1427 {
> 1428 struct sk_buff *hskb = csk->lro_hskb;
> 1429 struct cxgbit_lro_pdu_cb *hpdu_cb = cxgbit_skb_lro_pdu_cb(hskb, 0);
> 1430 struct cxgbit_lro_pdu_cb *pdu_cb = cxgbit_skb_lro_pdu_cb(skb, pdu_idx);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Smatch marks pdu_cb as tainted because it comes from skb->data
>
> 1431 struct skb_shared_info *hssi = skb_shinfo(hskb);
> 1432 struct skb_shared_info *ssi = skb_shinfo(skb);
> 1433 unsigned int len = 0;
> 1434
> 1435 if (pdu_cb->flags & PDUCBF_RX_HDR) {
> 1436 u8 hfrag_idx = hssi->nr_frags;
> 1437
> 1438 hpdu_cb->flags |= pdu_cb->flags;
> 1439 hpdu_cb->seq = pdu_cb->seq;
> 1440 hpdu_cb->hdr = pdu_cb->hdr;
> 1441 hpdu_cb->hlen = pdu_cb->hlen;
> 1442
> 1443 memcpy(&hssi->frags[hfrag_idx], &ssi->frags[pdu_cb->hfrag_idx],
> ^^^^^^^^^^^^^^^^^
> how do we know this is within bounds?
pdu_cb->hfrag_idx is assigned value in the following function
cxgbit_lro_add_packet_gl()
pdu_cb->hfrag_idx = skb_shinfo(skb)->nr_frags;
There is one more check for nr_frags in
cxgbit_lro_receive()
if ((gl && (((skb_shinfo(skb)->nr_frags + gl->nfrags) >
MAX_SKB_FRAGS) || (lro_cb->pdu_totallen >= LRO_FLUSH_LEN_MAX))) ||
(lro_cb->pdu_idx >= MAX_SKB_FRAGS)) {
cxgbit_lro_flush(lro_mgr, skb);
goto start_lro;
}
>
> 1444 sizeof(skb_frag_t));
> 1445
> 1446 get_page(skb_frag_page(&hssi->frags[hfrag_idx]));
> 1447 hssi->nr_frags++;
> 1448 hpdu_cb->frags++;
> 1449 hpdu_cb->hfrag_idx = hfrag_idx;
> 1450
> 1451 len = hssi->frags[hfrag_idx].size;
> 1452 hskb->len += len;
> 1453 hskb->data_len += len;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] cxgbit: add files for cxgbit.ko
2018-01-23 14:05 [bug report] cxgbit: add files for cxgbit.ko Dan Carpenter
2018-01-29 14:49 ` Varun Prakash
@ 2018-01-30 2:38 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2018-01-30 2:38 UTC (permalink / raw)
To: target-devel
On Mon, Jan 29, 2018 at 08:19:24PM +0530, Varun Prakash wrote:
> On Tue, Jan 23, 2018 at 05:05:50PM +0300, Dan Carpenter wrote:
> > Hello Varun Prakash,
> >
> > The patch 9730ffcb8957: "cxgbit: add files for cxgbit.ko" from Apr
> > 20, 2016, leads to the following static checker warning:
> >
> > drivers/target/iscsi/cxgbit/cxgbit_target.c:1443 cxgbit_lro_skb_merge()
> > error: buffer overflow 'ssi->frags' 17 <= 255
> >
> > drivers/target/iscsi/cxgbit/cxgbit_target.c
> > 1425 static void
> > 1426 cxgbit_lro_skb_merge(struct cxgbit_sock *csk, struct sk_buff *skb, u8 pdu_idx)
> > 1427 {
> > 1428 struct sk_buff *hskb = csk->lro_hskb;
> > 1429 struct cxgbit_lro_pdu_cb *hpdu_cb = cxgbit_skb_lro_pdu_cb(hskb, 0);
> > 1430 struct cxgbit_lro_pdu_cb *pdu_cb = cxgbit_skb_lro_pdu_cb(skb, pdu_idx);
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Smatch marks pdu_cb as tainted because it comes from skb->data
> >
> > 1431 struct skb_shared_info *hssi = skb_shinfo(hskb);
> > 1432 struct skb_shared_info *ssi = skb_shinfo(skb);
> > 1433 unsigned int len = 0;
> > 1434
> > 1435 if (pdu_cb->flags & PDUCBF_RX_HDR) {
> > 1436 u8 hfrag_idx = hssi->nr_frags;
> > 1437
> > 1438 hpdu_cb->flags |= pdu_cb->flags;
> > 1439 hpdu_cb->seq = pdu_cb->seq;
> > 1440 hpdu_cb->hdr = pdu_cb->hdr;
> > 1441 hpdu_cb->hlen = pdu_cb->hlen;
> > 1442
> > 1443 memcpy(&hssi->frags[hfrag_idx], &ssi->frags[pdu_cb->hfrag_idx],
> > ^^^^^^^^^^^^^^^^^
> > how do we know this is within bounds?
>
> pdu_cb->hfrag_idx is assigned value in the following function
>
> cxgbit_lro_add_packet_gl()
> pdu_cb->hfrag_idx = skb_shinfo(skb)->nr_frags;
>
> There is one more check for nr_frags in
> cxgbit_lro_receive()
>
> if ((gl && (((skb_shinfo(skb)->nr_frags + gl->nfrags) >
> MAX_SKB_FRAGS) || (lro_cb->pdu_totallen >= LRO_FLUSH_LEN_MAX))) ||
> (lro_cb->pdu_idx >= MAX_SKB_FRAGS)) {
> cxgbit_lro_flush(lro_mgr, skb);
> goto start_lro;
> }
Ah. Thanks. That's tricky for static analysis to follow....
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* [bug report] cxgbit: add files for cxgbit.ko
@ 2022-01-21 15:21 Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2022-01-21 15:21 UTC (permalink / raw)
To: varun; +Cc: target-devel
Hello Varun Prakash,
The patch 9730ffcb8957: "cxgbit: add files for cxgbit.ko" from Apr
20, 2016, leads to the following Smatch static checker warning:
drivers/target/iscsi/cxgbit/cxgbit_cm.c:561 __cxgbit_free_cdev_np()
warn: inconsistent refcounting 'cnp->kref.refcount.refs.counter'
inc on: 548
dec on: 542
drivers/target/iscsi/cxgbit/cxgbit_cm.c
517 static int
518 __cxgbit_free_cdev_np(struct cxgbit_device *cdev, struct cxgbit_np *cnp)
519 {
520 int stid, ret;
521 bool ipv6 = false;
522
523 stid = cxgbit_np_hash_del(cdev, cnp);
524 if (stid < 0)
525 return -EINVAL;
526 if (!test_bit(CDEV_STATE_UP, &cdev->flags))
527 return -EINVAL;
528
529 if (cnp->np->np_sockaddr.ss_family == AF_INET6)
530 ipv6 = true;
531
532 cxgbit_get_cnp(cnp);
get here
533 cxgbit_init_wr_wait(&cnp->com.wr_wait);
534 ret = cxgb4_remove_server(cdev->lldi.ports[0], stid,
535 cdev->lldi.rxq_ids[0], ipv6);
536
537 if (ret > 0)
538 ret = net_xmit_errno(ret);
539
540 if (ret) {
541 cxgbit_put_cnp(cnp);
put here
542 return ret;
543 }
544
545 ret = cxgbit_wait_for_reply(cdev, &cnp->com.wr_wait,
546 0, 10, __func__);
547 if (ret == -ETIMEDOUT)
548 return ret;
Should this error path put as well?
549
550 if (ipv6 && cnp->com.cdev) {
551 struct sockaddr_in6 *sin6;
552
553 sin6 = (struct sockaddr_in6 *)&cnp->com.local_addr;
554 cxgb4_clip_release(cdev->lldi.ports[0],
555 (const u32 *)&sin6->sin6_addr.s6_addr,
556 1);
557 }
558
559 cxgb4_free_stid(cdev->lldi.tids, stid,
560 cnp->com.local_addr.ss_family);
561 return 0;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-21 15:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-23 14:05 [bug report] cxgbit: add files for cxgbit.ko Dan Carpenter
2018-01-29 14:49 ` Varun Prakash
2018-01-30 2:38 ` Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2022-01-21 15:21 Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.