* re: [SCSI] csiostor: Chelsio FCoE offload driver
@ 2014-04-16 15:33 Dan Carpenter
2014-04-16 15:37 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2014-04-16 15:33 UTC (permalink / raw)
To: naresh; +Cc: linux-scsi
Hello Naresh Kumar Inna,
The patch a3667aaed569: "[SCSI] csiostor: Chelsio FCoE offload
driver" from Nov 15, 2012, leads to the following static checker
warning:
drivers/scsi/csiostor/csio_mb.c:1534 csio_mb_isr_handler()
warn: was 'sizeof(*mbp)' intended?
drivers/scsi/csiostor/csio_mb.c
1451 int
1452 csio_mb_isr_handler(struct csio_hw *hw)
1453 {
1454 struct csio_mbm *mbm = &hw->mbm;
^^^
This struct is fairly large.
1455 struct csio_mb *mbp = mbm->mcurrent;
1456 __be64 *cmd;
1457 uint32_t ctl, cim_cause, pl_cause;
1458 int i;
1459 uint32_t ctl_reg = PF_REG(hw->pfn, CIM_PF_MAILBOX_CTRL);
[ snip ]
1530 /*
1531 * Enqueue event to EventQ. Events processing happens
1532 * in Event worker thread context
1533 */
1534 if (csio_enqueue_evt(hw, CSIO_EVT_MBX, mbp, sizeof(mbp)))
^^^^^^^^^^
This is equivalent to sizeof(long) when sizeof(*mbp) was probably
intended.
1535 CSIO_INC_STATS(hw, n_evt_drop);
1536
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [SCSI] csiostor: Chelsio FCoE offload driver
2014-04-16 15:33 [SCSI] csiostor: Chelsio FCoE offload driver Dan Carpenter
@ 2014-04-16 15:37 ` Dan Carpenter
2015-02-26 9:49 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2014-04-16 15:37 UTC (permalink / raw)
To: linux-scsi
Naresh's email is dead.
regards,
dan carpenter
On Wed, Apr 16, 2014 at 06:33:24PM +0300, Dan Carpenter wrote:
> Hello Naresh Kumar Inna,
>
> The patch a3667aaed569: "[SCSI] csiostor: Chelsio FCoE offload
> driver" from Nov 15, 2012, leads to the following static checker
> warning:
>
> drivers/scsi/csiostor/csio_mb.c:1534 csio_mb_isr_handler()
> warn: was 'sizeof(*mbp)' intended?
>
> drivers/scsi/csiostor/csio_mb.c
> 1451 int
> 1452 csio_mb_isr_handler(struct csio_hw *hw)
> 1453 {
> 1454 struct csio_mbm *mbm = &hw->mbm;
> ^^^
> This struct is fairly large.
>
> 1455 struct csio_mb *mbp = mbm->mcurrent;
> 1456 __be64 *cmd;
> 1457 uint32_t ctl, cim_cause, pl_cause;
> 1458 int i;
> 1459 uint32_t ctl_reg = PF_REG(hw->pfn, CIM_PF_MAILBOX_CTRL);
>
> [ snip ]
>
> 1530 /*
> 1531 * Enqueue event to EventQ. Events processing happens
> 1532 * in Event worker thread context
> 1533 */
> 1534 if (csio_enqueue_evt(hw, CSIO_EVT_MBX, mbp, sizeof(mbp)))
> ^^^^^^^^^^
> This is equivalent to sizeof(long) when sizeof(*mbp) was probably
> intended.
>
> 1535 CSIO_INC_STATS(hw, n_evt_drop);
> 1536
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [SCSI] csiostor: Chelsio FCoE offload driver
2014-04-16 15:37 ` Dan Carpenter
@ 2015-02-26 9:49 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2015-02-26 9:49 UTC (permalink / raw)
To: linux-scsi, Praveen Madhavan, Hariprasad Shenai
Some of the Chelsio people should add themselves to the MAINTAINERS
file for this driver? I have a new static checker warning:
drivers/scsi/csiostor/csio_mb.c:928 csio_fcoe_vnp_alloc_init_mb()
warn: variable dereferenced before check 'vnport_wwnn' (see line 926)
"vnport_wwnn" can't be NULL so we should probably just delete the check.
Anyway, I was looking at old csiostor bugs in my in box and this one
from last November is more serious.
On Wed, Apr 16, 2014 at 06:37:16PM +0300, Dan Carpenter wrote:
> Naresh's email is dead.
>
> regards,
> dan carpenter
>
> On Wed, Apr 16, 2014 at 06:33:24PM +0300, Dan Carpenter wrote:
> > Hello Naresh Kumar Inna,
> >
> > The patch a3667aaed569: "[SCSI] csiostor: Chelsio FCoE offload
> > driver" from Nov 15, 2012, leads to the following static checker
> > warning:
> >
> > drivers/scsi/csiostor/csio_mb.c:1534 csio_mb_isr_handler()
> > warn: was 'sizeof(*mbp)' intended?
> >
> > drivers/scsi/csiostor/csio_mb.c
> > 1451 int
> > 1452 csio_mb_isr_handler(struct csio_hw *hw)
> > 1453 {
> > 1454 struct csio_mbm *mbm = &hw->mbm;
> > ^^^
> > This struct is fairly large.
> >
> > 1455 struct csio_mb *mbp = mbm->mcurrent;
> > 1456 __be64 *cmd;
> > 1457 uint32_t ctl, cim_cause, pl_cause;
> > 1458 int i;
> > 1459 uint32_t ctl_reg = PF_REG(hw->pfn, CIM_PF_MAILBOX_CTRL);
> >
> > [ snip ]
> >
> > 1530 /*
> > 1531 * Enqueue event to EventQ. Events processing happens
> > 1532 * in Event worker thread context
> > 1533 */
> > 1534 if (csio_enqueue_evt(hw, CSIO_EVT_MBX, mbp, sizeof(mbp)))
> > ^^^^^^^^^^
> > This is equivalent to sizeof(long) when sizeof(*mbp) was probably
> > intended.
Definitely the original code is buggy. It's possible that sizeof(*mbp)
was intended as I said before but this is really weird to pass "mbp"
here so I'm not sure. Someone should test this.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* re: [SCSI] csiostor: Chelsio FCoE offload driver
@ 2013-02-06 13:09 Dan Carpenter
2013-02-08 7:34 ` Naresh Kumar Inna
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2013-02-06 13:09 UTC (permalink / raw)
To: Naresh Kumar Inna; +Cc: linux-scsi, kbuild
Hopefully, you recieved an email about this last November, but this
is a follow up because the bug is still there.
Smatch complains about a buffer overflow in this:
drivers/scsi/csiostor/csio_rnode.c:872 csio_rnode_fwevt_handler()
error: buffer overflow '(rn)->stats.n_evt_fw' 22 <= 26
859 void
860 csio_rnode_fwevt_handler(struct csio_rnode *rn, uint8_t fwevt)
861 {
862 struct csio_lnode *ln = csio_rnode_to_lnode(rn);
863 enum csio_rn_ev evt;
864
865 evt = CSIO_FWE_TO_RNFE(fwevt);
866 if (!evt) {
Events greater than PROTO_ERR_IMPL_LOGO are invalid.
867 csio_ln_err(ln, "ssni:x%x Unhandled FW Rdev event: %d\n",
868 csio_rn_flowid(rn), fwevt);
869 CSIO_INC_STATS(rn, n_evt_unexp);
870 return;
871 }
872 CSIO_INC_STATS(rn, n_evt_fw[fwevt]);
It looks like new events were added and the size of the n_evt_fw[]
array wasn't updated to hold them. Everything after RSCN_DEV_LOST
causes memory corruption.
RSCN_DEV_LOST = 0x16,
SCR_ACC_RCVD = 0x17,
ADISC_RJT_RCVD = 0x18,
LOGO_SNT = 0x19,
PROTO_ERR_IMPL_LOGO = 0x1a,
There is a related bug in the lnode version of this code which
Smatch does not catch.
drivers/scsi/csiostor/csio_lnode.c
1555 /* save previous event for debugging */
1556 ln->prev_evt = ln->cur_evt;
1557 ln->cur_evt = rdev_wr->event_cause;
1558 CSIO_INC_STATS(ln, n_evt_fw[rdev_wr->event_cause]);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Memory corruption.
1559
1560 /* Translate all the fabric events to lnode SM events */
1561 evt = CSIO_FWE_TO_LNE(rdev_wr->event_cause);
1562 if (evt) {
Valid events handled here but we already corrupted memory three
lines earlier.
1563 csio_ln_dbg(ln,
1564 "Posting event to lnode event:%d "
1565 "cause:%d flowid:x%x\n", evt,
1566 rdev_wr->event_cause, rdev_flowid);
1567 csio_post_event(&ln->sm, evt);
1568 }
1569
I wasn't a part of the discussion in November, but the fix for this
seems trivial. I'm probably missing something?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [SCSI] csiostor: Chelsio FCoE offload driver
2013-02-06 13:09 Dan Carpenter
@ 2013-02-08 7:34 ` Naresh Kumar Inna
0 siblings, 0 replies; 5+ messages in thread
From: Naresh Kumar Inna @ 2013-02-08 7:34 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-scsi@vger.kernel.org, kbuild@01.org
On 2/6/2013 6:39 PM, Dan Carpenter wrote:
> Hopefully, you recieved an email about this last November, but this
> is a follow up because the bug is still there.
>
I don't recollect getting that email. Thanks for reporting nevertheless.
There are some patches lined up for supporting new features and some bug
fixes. I will ensure those patches address the issues you have reported.
Thanks,
Naresh.
> Smatch complains about a buffer overflow in this:
>
> drivers/scsi/csiostor/csio_rnode.c:872 csio_rnode_fwevt_handler()
> error: buffer overflow '(rn)->stats.n_evt_fw' 22 <= 26
>
> 859 void
> 860 csio_rnode_fwevt_handler(struct csio_rnode *rn, uint8_t fwevt)
> 861 {
> 862 struct csio_lnode *ln = csio_rnode_to_lnode(rn);
> 863 enum csio_rn_ev evt;
> 864
> 865 evt = CSIO_FWE_TO_RNFE(fwevt);
> 866 if (!evt) {
>
> Events greater than PROTO_ERR_IMPL_LOGO are invalid.
>
> 867 csio_ln_err(ln, "ssni:x%x Unhandled FW Rdev event: %d\n",
> 868 csio_rn_flowid(rn), fwevt);
> 869 CSIO_INC_STATS(rn, n_evt_unexp);
> 870 return;
> 871 }
> 872 CSIO_INC_STATS(rn, n_evt_fw[fwevt]);
>
> It looks like new events were added and the size of the n_evt_fw[]
> array wasn't updated to hold them. Everything after RSCN_DEV_LOST
> causes memory corruption.
>
> RSCN_DEV_LOST = 0x16,
> SCR_ACC_RCVD = 0x17,
> ADISC_RJT_RCVD = 0x18,
> LOGO_SNT = 0x19,
> PROTO_ERR_IMPL_LOGO = 0x1a,
>
> There is a related bug in the lnode version of this code which
> Smatch does not catch.
>
> drivers/scsi/csiostor/csio_lnode.c
> 1555 /* save previous event for debugging */
> 1556 ln->prev_evt = ln->cur_evt;
> 1557 ln->cur_evt = rdev_wr->event_cause;
> 1558 CSIO_INC_STATS(ln, n_evt_fw[rdev_wr->event_cause]);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Memory corruption.
>
> 1559
> 1560 /* Translate all the fabric events to lnode SM events */
> 1561 evt = CSIO_FWE_TO_LNE(rdev_wr->event_cause);
> 1562 if (evt) {
>
> Valid events handled here but we already corrupted memory three
> lines earlier.
>
> 1563 csio_ln_dbg(ln,
> 1564 "Posting event to lnode event:%d "
> 1565 "cause:%d flowid:x%x\n", evt,
> 1566 rdev_wr->event_cause, rdev_flowid);
> 1567 csio_post_event(&ln->sm, evt);
> 1568 }
> 1569
>
> I wasn't a part of the discussion in November, but the fix for this
> seems trivial. I'm probably missing something?
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-02-26 9:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-16 15:33 [SCSI] csiostor: Chelsio FCoE offload driver Dan Carpenter
2014-04-16 15:37 ` Dan Carpenter
2015-02-26 9:49 ` Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2013-02-06 13:09 Dan Carpenter
2013-02-08 7:34 ` Naresh Kumar Inna
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).