[dm-devel] [PATCH v3] multipathd: handle fpin events
Benjamin Marzinski
bmarzins at redhat.com
Thu Feb 10 21:02:15 UTC 2022
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
>
> Date: Thu, 10 Feb 2022 16:50:12 +0000 (UTC)
> From: scan-admin at coverity.com
> To: mwilck at suse.com
> 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.
>
>
> ________________________________________________________________________________________________________
> *** CID 375094: Memory - corruptions (OVERRUN)
> /multipathd/fpin_handlers.c: 339 in fpin_handle_els_frame()
> 333 struct fc_els_fpin *fpin = (struct fc_els_fpin *)&fc_event->event_data;
> 334 struct fc_tlv_desc *tlv;
> 335 uint32_t dtag;
> 336
> 337 els_cmd = (uint32_t)fc_event->event_data;
> 338 tlv = (struct fc_tlv_desc *)&fpin->fpin_desc[0];
> >>> CID 375094: Memory - corruptions (OVERRUN)
> >>> "tlv" evaluates to an address that is at byte offset 0 of an array of -4 bytes.
> 339 dtag = be32_to_cpu(tlv->desc_tag);
> 340 condlog(4, "Got CMD in add as 0x%x fpin_cmd 0x%x dtag 0x%x\n",
> 341 els_cmd, fpin->fpin_cmd, dtag);
> 342
> 343 if ((fc_event->event_code == FCH_EVT_LINK_FPIN) ||
> 344 (fc_event->event_code == FCH_EVT_LINKUP) ||
Um.. I don't think the array size is really -4 bytes. It's a zero-legth
array, and that seems to be tripping up coverity. This looks fine,
assuming a well formed fc_event structure. Otherwise we need some checking
like
offsetof(struct fc_els_fpin, fpin_desc) + sizeof(struct fc_tlv_desc) <= fc_event->event_data_len
to guarantee that the space actually exists for this.
-Ben
>
>
> ________________________________________________________________________________________________________
> To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yrlXjF1MXVk7PoaBOP4azsLepCe1Mn8gwfBXDBrelSIoRMU8O0U7o5n1FSIQj14Dq4St65JUh9ZnyC-2Fg157qes-2Ft9bX_PKeZaaewXacQHsuZ3T6aIqwwc4cZvX5RuFKlZH-2B-2F-2FW3e6H-2BXHVqahp7aWZNvTEgZriF6MZKau2Bf0lzbvkaTbX4e4aRrLiV588LPwISv-2Baiuvm-2BG4Up9iV7VtAl7qS-2B0T3-2Fqqv361QJbg5iOmqOQvmACpZbC7bxySCtES2vULs1HDMmm0iEnBDbGCNYF1xpA27h3e8fIXqCGYvqwYb-2B3OQ-3D-3D
>
> To manage Coverity Scan email notifications for "mwilck at suse.com", click https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yped04pjJnmXOsUBtKYNIXxFPY5t1N6AzTJMa-2BnAWi0npABKE4tMGq7bchotHs-2B2MqcQoJY2TJBjK3IaZ-2BUkHe2wCTAqHQyQFhpCFLiEJ5yDOXcRNksjcYeVNM9nq6-2Fe1c-3DJZ6i_PKeZaaewXacQHsuZ3T6aIqwwc4cZvX5RuFKlZH-2B-2F-2FW3e6H-2BXHVqahp7aWZNvTEgZ8ESLNy-2BD7eSZoQJj9Azw80YJzcErYNLrKcxKMYgGbPxYRwvkDgeGxz4WRfn-2B-2BuPaXZTSYImuaAoKoYTe4RaUajUfXcURMKMcXzDOS6kanowCj0jE9AfLqwwWVbaP-2BgeCbRXyI-2FvgEy6HVXWmfdW8qg-3D-3D
>
More information about the dm-devel
mailing list