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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
