Re: [PATCH v18 10/13] scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ

[Dave Marquardt <davemarq@linux.ibm.com>]

sashiko-bot@kernel.org writes:

>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 7406e7182777c..5dba084086d5f 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> @@ -4456,6 +4456,106 @@ static struct ibmvfc_crq *ibmvfc_next_scrq(struct ibmvfc_queue *scrq)
>
> [ ... ]
>
>> +static void ibmvfc_queue_async_subq_entry(struct ibmvfc_host *vhost,
>> +					  struct ibmvfc_async_sub_crq *crq,
>> +					  struct list_head *doneq)
>> +{
>> +	struct ibmvfc_async_drain_entry *entry;
>> +
>> +	entry = kzalloc_obj(*entry, GFP_ATOMIC);
>> +	if (entry) {
>> +		entry->event.type = IBMVFC_ASYNC_CRQ_SUB;
>> +		entry->event.subq = *crq;
>
> [Severity: High]
> Can dispatching an IBMVFC_ASYNC_CRQ_SUB FPIN event here lead to a NULL
> pointer dereference in ibmvfc_process_async_work()?
>
> When ibmvfc_drain_async_subq() passes entry->event to ibmvfc_handle_async()
> for an IBMVFC_AE_FPIN event, it queues ibmvfc_process_async_work() with
> aw->event.type == IBMVFC_ASYNC_CRQ_SUB.
>
> In drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_process_async_work(), crq is
> initialized to NULL and remains NULL on the IBMVFC_ASYNC_CRQ_SUB branch:
>
> 	struct ibmvfc_async_sub_crq *subq = NULL;
> 	struct ibmvfc_async_work *aw;
> 	struct ibmvfc_async_crq *crq = NULL;
> 	...
> 	if (aw->event.type == IBMVFC_ASYNC_CRQ_SUB) {
> 		subq = &aw->event.subq;
> 		scsi_id = 0;
> 		wwpn = subq->wwpn;
> 		...
> 	}
>
> If the sub-CRQ FPIN event has all-zero target identifiers (for example, a
> host-scope IBMVFC_AE_FPIN_LINK_CONGESTED or
> IBMVFC_AE_FPIN_CONGESTION_CLEARED event with zero wwpn and zero
> node_name/assoc_id, or with IBMVFC_ASYNC_IS_FPIN_EXT set and wwpn == 0),
> ibmvfc_process_async_work() unconditionally calls ibmvfc_basic_fpin_to_desc()
> with crq == NULL instead of ibmvfc_full_fpin_to_desc(subq, ...):
>
> 	if (!scsi_id && !wwpn && !node_name && !assoc_id) {
> 		if (fpin_status == IBMVFC_AE_FPIN_LINK_CONGESTED ||
> 		    fpin_status == IBMVFC_AE_FPIN_CONGESTION_CLEARED)
> 			fpin = ibmvfc_basic_fpin_to_desc(crq,
> 					cpu_to_be64(fc_host_port_name(vhost->host)), 0);
> 		goto send_fpin;
> 	}
>
> ibmvfc_basic_fpin_to_desc() then immediately dereferences crq->fpin_status.
> Should this path check if (crq) vs else ibmvfc_full_fpin_to_desc(subq, ...)
> in the same way as the !tgt fallback below it?

Fixed in the next revision of patch 9.

>> +		list_add_tail(&entry->list, doneq);
>> +	} else
>> +		dev_err_ratelimited(vhost->dev,
>> +				    "Couldn't allocate async drain entry\n");
>> +	crq->valid = 0;
>> +	wmb();	/* complete write */
>> +}
>> +
>> +static void ibmvfc_drain_async_subq(struct ibmvfc_queue *scrq)
>> +{
>
> [ ... ]
>
>> +	list_for_each_entry_safe(entry, temp, &doneq, list) {
>> +		list_del(&entry->list);
>> +		spin_lock_irqsave(&vhost->host->host_lock, flags);
>> +		ibmvfc_handle_async(&entry->event, vhost);
>
> [Severity: High]
> When ibmvfc_handle_async() queues ibmvfc_process_async_work() for an
> IBMVFC_AE_FPIN event with all-zero target identifiers, can tgt be used
> uninitialized at the send_fpin label?
>
> In drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_process_async_work(), tgt is
> declared without initialization, and the all-zero identifier check jumps
> over ibmvfc_find_target() directly to send_fpin:
>
> 	struct ibmvfc_target *tgt;
> 	...
> 	if (!scsi_id && !wwpn && !node_name && !assoc_id) {
> 		if (fpin_status == IBMVFC_AE_FPIN_LINK_CONGESTED ||
> 		    fpin_status == IBMVFC_AE_FPIN_CONGESTION_CLEARED)
> 			fpin = ibmvfc_basic_fpin_to_desc(crq,
> 					cpu_to_be64(fc_host_port_name(vhost->host)), 0);
> 		goto send_fpin;
> 	}
> 	...
> send_fpin:
> 	...
> 	if (tgt)
> 		kref_put(&tgt->kref, ibmvfc_release_tgt);
>
> Wouldn't that evaluate if (tgt) and call kref_put() on an uninitialized
> stack pointer?

Fixed in the next revision of patch 6.

-Dave