Re: [SCSI] fnic: FIP VLAN Discovery Feature Support

[Dan Carpenter <dan.carpenter@oracle.com>]

Hiral isn't at Cisco any more.

regards,
dan carpenter

On Wed, Jan 08, 2014 at 12:57:41PM +0300, Dan Carpenter wrote:
> Hello Hiral Patel,
> 
> The patch d3c995f1dcf9: "[SCSI] fnic: FIP VLAN Discovery Feature
> Support" from Feb 25, 2013, leads to the following
> static checker warning:
> 
> 	drivers/scsi/fnic/fnic_fcs.c:278 is_fnic_fip_flogi_reject()
> 	warn: is it ok to set 'els_len' to negative?
> 
> This is some unpushable debug code on my Smatch system.  It is sort of
> a nonsense warning because Smatch has run into an "impossible" condition
> because of a bug in Smatch but also because of bug in
> fnic_fcoe_send_vlan_req().
> 
> drivers/scsi/fnic/fnic_fcs.c
>    251          fiph = (struct fip_header *)skb->data;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> fiph is untrusted data.  We don't trust the network.  This is a rule of
> Linux.
> 
>    252          op = ntohs(fiph->fip_op);
>    253          sub = fiph->fip_subcode;
>    254  
>    255          if (op != FIP_OP_LS)
>    256                  return 0;
>    257  
>    258          if (sub != FIP_SC_REP)
>    259                  return 0;
>    260  
>    261          rlen = ntohs(fiph->fip_dl_len) * 4;
>    262          if (rlen + sizeof(*fiph) > skb->len)
>    263                  return 0;
>    264  
>    265          desc = (struct fip_desc *)(fiph + 1);
>    266          dlen = desc->fip_dlen * FIP_BPW;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> desc->fip_dlen is u8 type so it could be a number between 0-255.  That's
> fine.
> FIP_BPW is 4.
> Here is the bug in Smatch.  It looks at the sender code and says that
> desc->fip_dlen is set to either 2 or 4.  So now dlen is in 8-16 range.
> 
>    267  
>    268          if (desc->fip_dtype == FIP_DT_FLOGI) {
>    269  
>    270                  shost_printk(KERN_DEBUG, lport->host,
>    271                            " FIP TYPE FLOGI: fab name:%llx "
>    272                            "vfid:%d map:%x\n",
>    273                            fip->sel_fcf->fabric_name, fip->sel_fcf->vfid,
>    274                            fip->sel_fcf->fc_map);
>    275                  if (dlen < sizeof(*els) + sizeof(*fh) + 1)
>                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 8-16 is always less than 29 so this condition is always true.
> 
>    276                          return 0;
>    277  
>    278                  els_len = dlen - sizeof(*els);
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> We are in an impossible condition and Smatch doesn't know the possible
> values of "dlen" any more so it generates the warning.
> 
>    279                  els = (struct fip_encaps *)desc;
>    280                  fh = (struct fc_frame_header *)(els + 1);
>    281                  els_dtype = desc->fip_dtype;
>    282  
>    283                  if (!fh)
>    284                          return 0;
>    285  
>    286                  /*
>    287                   * ELS command code, reason and explanation should be = Reject,
>    288                   * unsupported command and insufficient resource
>    289                   */
>    290                  els_op = *(u8 *)(fh + 1);
>    291                  if (els_op == ELS_LS_RJT) {
>    292                          shost_printk(KERN_INFO, lport->host,
>    293                                    "Flogi Request Rejected by Switch\n");
>    294                          return 1;
>    295                  }
>    296                  shost_printk(KERN_INFO, lport->host,
>    297                                  "Flogi Request Accepted by Switch\n");
>    298          }
>    299          return 0;
>    300  }
>    301  
>    302  static void fnic_fcoe_send_vlan_req(struct fnic *fnic)
>    303  {
>    304          struct fcoe_ctlr *fip = &fnic->ctlr;
>    305          struct fnic_stats *fnic_stats = &fnic->fnic_stats;
>    306          struct sk_buff *skb;
>    307          char *eth_fr;
>    308          int fr_len;
>    309          struct fip_vlan *vlan;
>    310          u64 vlan_tov;
>    311  
>    312          fnic_fcoe_reset_vlans(fnic);
>    313          fnic->set_vlan(fnic, 0);
>    314          FNIC_FCS_DBG(KERN_INFO, fnic->lport->host,
>    315                    "Sending VLAN request...\n");
>    316          skb = dev_alloc_skb(sizeof(struct fip_vlan));
>    317          if (!skb)
>    318                  return;
>    319  
>    320          fr_len = sizeof(*vlan);
>    321          eth_fr = (char *)skb->data;
>    322          vlan = (struct fip_vlan *)eth_fr;
>    323  
>    324          memset(vlan, 0, sizeof(*vlan));
>    325          memcpy(vlan->eth.h_source, fip->ctl_src_addr, ETH_ALEN);
>    326          memcpy(vlan->eth.h_dest, fcoe_all_fcfs, ETH_ALEN);
>    327          vlan->eth.h_proto = htons(ETH_P_FIP);
>    328  
>    329          vlan->fip.fip_ver = FIP_VER_ENCAPS(FIP_VER);
>    330          vlan->fip.fip_op = htons(FIP_OP_VLAN);
>    331          vlan->fip.fip_subcode = FIP_SC_VL_REQ;
>    332          vlan->fip.fip_dl_len = htons(sizeof(vlan->desc) / FIP_BPW);
>    333  
>    334          vlan->desc.mac.fd_desc.fip_dtype = FIP_DT_MAC;
>    335          vlan->desc.mac.fd_desc.fip_dlen = sizeof(vlan->desc.mac) / FIP_BPW;
> 
> 8 / 4 = 2.  We are setting ->fib_dlen to 2.
> 
>    336          memcpy(&vlan->desc.mac.fd_mac, fip->ctl_src_addr, ETH_ALEN);
>    337  
>    338          vlan->desc.wwnn.fd_desc.fip_dtype = FIP_DT_NAME;
>    339          vlan->desc.wwnn.fd_desc.fip_dlen = sizeof(vlan->desc.wwnn) / FIP_BPW;
>                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This is actually a second Smatch bug which it inherited from Sparse.  It
> doesn't understand packed structs correctly and think we are setting
> ->fib_dlen to 4 but we are setting it to 3.
> 
>    340          put_unaligned_be64(fip->lp->wwnn, &vlan->desc.wwnn.fd_wwn);
>    341          atomic64_inc(&fnic_stats->vlan_stats.vlan_disc_reqs);
>    342  
> 
> I suspect that the size calculation is not taking
> sizeof(struct fc_frame_header) into consideration properly in
> fnic_fcoe_send_vlan_req()?
> 
> regards,
> dan carpenter