Re: [PATCH v18 06/13] scsi: ibmvfc: add basic FPIN support

[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 e04c80050b981..a87beaeca13c8 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>
> [ ... ]
>
>> @@ -3264,6 +3268,277 @@ static const char *ibmvfc_get_link_state(enum ibmvfc_ae_link_state state)
>
> [ ... ]
>
>> +/**
>> + * ibmvfc_process_async_work - Process IBMVFC_AE_FPIN async CRQ from work queue
>> + * @work: pointer to work_struct
>> + */
>> +VISIBLE_IF_KUNIT void ibmvfc_process_async_work(struct work_struct *work)
>> +{
>> +	struct ibmvfc_async_work *aw;
>> +	struct ibmvfc_async_crq *crq;
>> +	struct ibmvfc_target *tgt;
>> +	struct ibmvfc_host *vhost;
>> +	struct fc_els_fpin *fpin;
>> +	unsigned long flags;
>> +
>> +	aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
>> +	vhost = aw->vhost;
>> +	crq = &aw->crq;
>> +
>> +	if (!crq->scsi_id && !crq->wwpn && !crq->node_name) {
>> +		if (crq->fpin_status == IBMVFC_AE_FPIN_LINK_CONGESTED ||
>> +		    crq->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;
>> +	}
>> +
>> +	spin_lock_irqsave(&vhost->host->host_lock, flags);
>> +	tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, crq->scsi_id,
>> +				 crq->wwpn, crq->node_name);
>> +	if (!tgt) {
>> +		/* Target not found in scsi_scrqs, search nvme_scrqs */
>> +		tgt = ibmvfc_find_target(&vhost->nvme_scrqs.targets,
>> +					 crq->scsi_id, crq->wwpn,
>> +					 crq->node_name);
>> +	}
>> +
>> +	if (tgt) {
>> +		kref_get(&tgt->kref);
>> +		spin_unlock_irqrestore(&vhost->host->host_lock, flags);
>> +	} else {
>> +		spin_unlock_irqrestore(&vhost->host->host_lock, flags);
>> +		dev_err_ratelimited(vhost->dev, "Invalid target for FPIN\n");
>> +		goto free;
>> +	}
>> +
>> +	fpin = ibmvfc_basic_fpin_to_desc(crq,
>> +				cpu_to_be64(fc_host_port_name(vhost->host)),
>> +				tgt->wwpn);
>> +send_fpin:
>> +	if (fpin) {
>> +		fc_host_fpin_rcv(vhost->host,
>> +				 sizeof(*fpin) + be32_to_cpu(fpin->desc_len),
>> +				 (char *)fpin, 0);
>> +		kfree(fpin);
>> +	} else
>> +		dev_err_ratelimited(vhost->dev, "FPIN event received, unable to process\n");
>> +
>> +	if (tgt)
>> +		kref_put(&tgt->kref, ibmvfc_release_tgt);
>
> [Severity: Critical]
> In ibmvfc_process_async_work() in drivers/scsi/ibmvscsi/ibmvfc-core.c, both
> tgt and fpin are declared on the stack without being initialized to NULL.
>
> When crq->scsi_id, crq->wwpn, and crq->node_name are all zero, the function
> jumps directly to the send_fpin label without ever initializing tgt. At
> send_fpin, checking if (tgt) and calling kref_put(&tgt->kref,
> ibmvfc_release_tgt) will operate on an uninitialized stack pointer on every
> zero-identifier FPIN event, including valid host-scope
> IBMVFC_AE_FPIN_LINK_CONGESTED and IBMVFC_AE_FPIN_CONGESTION_CLEARED events.
>
> Furthermore, if crq->fpin_status on that zero-identifier path is neither
> IBMVFC_AE_FPIN_LINK_CONGESTED nor IBMVFC_AE_FPIN_CONGESTION_CLEARED (such as
> the zero-identifier IBMVFC_AE_FPIN_PORT_CONGESTED case exercised by
> ibmvfc_async_fpin_test() in drivers/scsi/ibmvscsi/ibmvfc_kunit.c), fpin is
> also left uninitialized before jumping to send_fpin. If the uninitialized
> fpin pointer on the stack is non-zero, send_fpin will dereference
> fpin->desc_len, pass the garbage pointer to fc_host_fpin_rcv(), and pass it
> to kfree(fpin).
>
> Note that while the subsequent commit 6abed589f0fb ("scsi: ibmvfc: extend
> async event handlers for async sub-CRQ events") initializes fpin to NULL, tgt
> still remains uninitialized on the zero-identifier goto send_fpin path.
>
> Should both tgt and fpin be initialized to NULL at declaration in
> ibmvfc_process_async_work()?

Yes, tgt and fpin should be initialized to NULL. I'll update this in the
next revision.

-Dave