Hi Ben,
On Thu, 2022-02-10 at 15:02 -0600, Benjamin Marzinski wrote:
> On Thu, Feb 10, 2022 at 05:00:09PM +0000, Martin Wilck wrote:
> > Hi Muneendra,
> >
> > coverity found some defects in your patch. Please help me review
> > them,
> > see attachment. It's well possible that they're false positives,
> > but
> > please double-check.
> >
> > Thanks
> > Martin
> >
>
Thank you. Meanwhile I figured this out myself, and marked the defects
as false positives. I've pushed the patch to my "queue" branch. The
compilation errors have been resolved, too.
> > Date: Thu, 10 Feb 2022 16:50:12 +0000 (UTC)
> > From: scan-admin@xxxxxxxxxxxx
> > To: mwilck@xxxxxxxx
> > Subject: New Defects reported by Coverity Scan for
> > mwilck/multipath-tools
> >
> > Hi,
> >
> > Please find the latest report on new defect(s) introduced to
> > mwilck/multipath-tools found with Coverity Scan.
> >
> > 3 new defect(s) introduced to mwilck/multipath-tools found with
> > Coverity Scan.
> >
> >
> > New defect(s) Reported-by: Coverity Scan
> > Showing 3 of 3 defect(s)
> >
> >
> > ** CID 375096: Memory - corruptions (OVERRUN)
> > /multipathd/fpin_handlers.c: 161 in fpin_els_add_li_frame()
> >
> >
> > ___________________________________________________________________
> > _____________________________________
> > *** CID 375096: Memory - corruptions (OVERRUN)
> > /multipathd/fpin_handlers.c: 161 in fpin_els_add_li_frame()
> > 155 pthread_testcancel();
> > 156 els_mrg = calloc(1, sizeof(struct
> > els_marginal_list));
> > 157 if (els_mrg != NULL) {
> > 158 els_mrg->host_num = fc_event->host_no;
> > 159 els_mrg->event_code = fc_event->event_code;
> > 160 els_mrg->length = fc_event->event_datalen;
> > > > > CID 375096: Memory - corruptions (OVERRUN)
> > > > > Overrunning buffer pointed to by "&fc_event->event_data"
> > > > > of 4 bytes by passing it to a function which accesses it at
> > > > > byte offset 2047 using argument "fc_event->event_datalen"
> > > > > (which evaluates to 2048). [Note: The source code
> > > > > implementation of the function has been overridden by a
> > > > > builtin model.]
>
> fc_event->event_data is a u32, but that's just because the first 32
> bits
> of the event data is the els_cmd, right? The header makes it clear
> that
> event_data actually does hold event_datalen worth of space
>
> From /usr/include/scsi/scsi_netlink_fc.h
> -------------------------------------------
> * Note: if Vendor Unique message, &event_data will be start of
> * vendor unique payload, and the length of the payload is
> * per event_datalen
>
> The only thing that looks possibly suspect here to me is in
> fpin_fabric_notification_receiver() which calls
> fpin_els_add_li_frame()
>
> In fpin_fabric_notification_receiver() we guarantee that we read
> enough
> for plen to be correct by calling NLMSG_OK(), and we check that plen
> is
> big enough to hold fc_event before we start using that. After that,
> we
> just assume fc_event is well formed. However we could check that
>
> offsetof(struct fc_nl_event, event_data) + fc_event->event_data_len
> <= plen
>
> just to make sure that we really read enough space for all the event
> data.
>
> > 161 memcpy(els_mrg->payload, &(fc_event-
> > >event_data), fc_event->event_datalen);
> > 162 list_add_tail(&els_mrg->node,
> > &els_marginal_list_head);
> > 163 pthread_cond_signal(&fpin_li_cond);
> > 164 } else
> > 165 ret = -ENOMEM;
> > 166 pthread_cleanup_pop(1);
> >
> > ** CID 375095: Program hangs (LOCK)
> > /multipathd/fpin_handlers.c: 429 in fpin_els_li_consumer()
> >
> >
> > ___________________________________________________________________
> > _____________________________________
> > *** CID 375095: Program hangs (LOCK)
> > /multipathd/fpin_handlers.c: 429 in fpin_els_li_consumer()
> > 423 rcu_register_thread();
> > 424 pthread_cleanup_push(fpin_clean_marginal_dev_list,
> > NULL);
> > 425 INIT_LIST_HEAD(&marginal_list_head);
> > 426 pthread_cleanup_push(fpin_clean_els_marginal_list,
> > 427 (void
> > *)&marginal_list_head);
> > 428 for ( ; ; ) {
> > > > > CID 375095: Program hangs (LOCK)
> > > > > "pthread_mutex_lock" locks "fpin_li_mutex" while it is
> > > > > locked.
> > 429 pthread_mutex_lock(&fpin_li_mutex);
> > 430 pthread_cleanup_push(cleanup_mutex,
> > &fpin_li_mutex);
> > 431 pthread_testcancel();
> > 432 while (list_empty(&els_marginal_list_head))
> > 433 pthread_cond_wait(&fpin_li_cond,
> > &fpin_li_mutex);
> > 434
> >
> > ** CID 375094: Memory - corruptions (OVERRUN)
> > /multipathd/fpin_handlers.c: 339 in fpin_handle_els_frame()
>
> I can't see how we could double lock fpin_li_mutex here. This looks
> to me
> like coverity isn't understanding that the pthread_mutex_pop(1) that
> we
> must do before we loop is unlocking the mutex.
Exactly.
Martin
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
