Re: [bug report] cxgbit: add files for cxgbit.ko

[Varun Prakash <varun@chelsio.com>]

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;