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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: ibmvfc: NULL crq dereference in ibmvfc_process_async_work()
- [High] scsi: ibmvfc: uninitialized tgt pointer in ibmvfc_process_async_work()
--

commit e623a6d56430928cd690d92425a90ffd4dc7ba74
Author: Dave Marquardt <davemarq@linux.ibm.com>

scsi: ibmvfc: add interrupt routine for asynchronous sub CRQ

This commit adds ibmvfc_interrupt_async_subq() and supporting helpers to
drain asynchronous sub-CRQ entries onto a temporary list and dispatch them
through ibmvfc_handle_async().

> 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?

> +		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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-ibmvfc-fpin-support-v18-0-d683b74736a3@linux.ibm.com?part=10