Bhanu Gollapudi wrote: > As per FC-BB-5 rev-2 section 7.8.6.2, malformed FIP frame shall be discarded. > Drop Discovery Adv, ELS and CVL's with duplicate critical descriptors.
I guess duplicate descriptors means the frame is malformed. Good catch. > Signed-off-by: Bhanu Prakash Gollapudi <[email protected]> > --- > drivers/scsi/fcoe/libfcoe.c | 33 +++++++++++++++++++++++++++++++++ > 1 files changed, 33 insertions(+), 0 deletions(-) > > diff --git a/drivers/scsi/fcoe/libfcoe.c b/drivers/scsi/fcoe/libfcoe.c > index 82e9455..7a56634 100644 > --- a/drivers/scsi/fcoe/libfcoe.c > +++ b/drivers/scsi/fcoe/libfcoe.c > @@ -633,6 +633,7 @@ static int fcoe_ctlr_parse_adv(struct fcoe_ctlr *fip, > unsigned long t; > size_t rlen; > size_t dlen; > + u32 dupl_desc = 0; > > memset(fcf, 0, sizeof(*fcf)); > fcf->fka_period = msecs_to_jiffies(FCOE_CTLR_DEF_FKA); > @@ -649,6 +650,16 @@ static int fcoe_ctlr_parse_adv(struct fcoe_ctlr *fip, > dlen = desc->fip_dlen * FIP_BPW; > if (dlen < sizeof(*desc) || dlen > rlen) > return -EINVAL; > + /* Drop Adv if there are duplicate critical descriptors */ > + if (desc->fip_dtype < FIP_DT_LIMIT) { > + if (dupl_desc & (1 << desc->fip_dtype)) { > + LIBFCOE_FIP_DBG(fip, "Duplicate Critical " > + "Descriptors in FIP adv\n"); > + return -EINVAL; > + } else { > + dupl_desc |= (1 << desc->fip_dtype); > + } > + } > switch (desc->fip_dtype) { > case FIP_DT_PRI: > if (dlen != sizeof(struct fip_pri_desc)) > @@ -837,6 +848,7 @@ static void fcoe_ctlr_recv_els(struct fcoe_ctlr *fip, > struct sk_buff *skb) > size_t els_len = 0; > size_t rlen; > size_t dlen; > + u32 dupl_desc = 0; > > fiph = (struct fip_header *)skb->data; > sub = fiph->fip_subcode; > @@ -852,6 +864,16 @@ static void fcoe_ctlr_recv_els(struct fcoe_ctlr *fip, > struct sk_buff *skb) > dlen = desc->fip_dlen * FIP_BPW; > if (dlen < sizeof(*desc) || dlen > rlen) > goto drop; > + /* Drop ELS if there are duplicate critical descriptors */ > + if (desc->fip_dtype < FIP_DT_LIMIT) { We should check somewhere a build-time check here) that DT_LIMIT is never more than 31. Someday it could grow. Maybe add a BUILD_BUG_ON(FIP_DT_LIMIT >= 32); just after the declaration of dupl_desc. Or, do it the way I did in clr_vlink, which doesn't need those checks because the initialization of desc_mask will fail if any of the required descriptor numbers are greater than 31. I think that would be better because you can check both that there are no duplicates and that all the descriptors you expected are there. We can replace this with: if (desc->fip_type < 32 && !(desc_mask & (1U << desc->fip_type))) { ... Another way to get rid of the check for 32 is to use DECLARE_BITMAP() and use set_bit(), clear_bit(), and test_bit(). These are just as efficient on small maps, and we can size the map to FIP_DT_LIMIT. I should've done that when I coded desc_mask. Or, if you feel I'm being too picky, I don't see a big problem in the way you have it. > + if (dupl_desc & (1 << desc->fip_dtype)) { > + LIBFCOE_FIP_DBG(fip, "Duplicate Critical " > + "Descriptors in FIP ELS\n"); > + goto drop; > + } else { No need for else following goto. (minor) > + dupl_desc |= (1 << desc->fip_dtype); > + } > + } > switch (desc->fip_dtype) { > case FIP_DT_MAC: > if (dlen != sizeof(struct fip_mac_desc)) > @@ -946,6 +968,7 @@ static void fcoe_ctlr_recv_clr_vlink(struct fcoe_ctlr > *fip, > struct fcoe_fcf *fcf = fip->sel_fcf; > struct fc_lport *lport = fip->lp; > u32 desc_mask; > + u32 dupl_desc = 0; > > LIBFCOE_FIP_DBG(fip, "Clear Virtual Link received\n"); > > @@ -963,6 +986,16 @@ static void fcoe_ctlr_recv_clr_vlink(struct fcoe_ctlr > *fip, > dlen = desc->fip_dlen * FIP_BPW; > if (dlen > rlen) > return; > + /* Drop CVL if there are duplicate critical descriptors */ > + if (desc->fip_dtype < FIP_DT_LIMIT) { Here we already have desc_mask. The bit should be 1 if the descriptor was required and not already received. If it isn't expected we should fail the first time we see it. > + if (dupl_desc & (1 << desc->fip_dtype)) { > + LIBFCOE_FIP_DBG(fip, "Duplicate Critical " > + "Descriptors in FIP CVL\n"); > + return; > + } else { else not needed after return. I'd prefer to drop the indent on the next line. > + dupl_desc |= (1 << desc->fip_dtype); Parens not needed on right side of |=. > + } > + } > switch (desc->fip_dtype) { > case FIP_DT_MAC: > mp = (struct fip_mac_desc *)desc; _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
