* re: [SCSI] fnic: FIP VLAN Discovery Feature Support
@ 2014-01-08 9:57 Dan Carpenter
2014-01-08 12:47 ` Dan Carpenter
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2014-01-08 9:57 UTC (permalink / raw)
To: hiralpat; +Cc: linux-scsi
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [SCSI] fnic: FIP VLAN Discovery Feature Support
2014-01-08 9:57 [SCSI] fnic: FIP VLAN Discovery Feature Support Dan Carpenter
@ 2014-01-08 12:47 ` Dan Carpenter
0 siblings, 0 replies; 2+ messages in thread
From: Dan Carpenter @ 2014-01-08 12:47 UTC (permalink / raw)
To: Anantha Prakash T; +Cc: linux-scsi
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-01-08 12:47 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-08 9:57 [SCSI] fnic: FIP VLAN Discovery Feature Support Dan Carpenter
2014-01-08 12:47 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox