Re: [PATCH v18 09/13] scsi: ibmvfc: extend async event handlers for async sub-CRQ events

[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 read in ibmvfc_process_async_work()
--

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

scsi: ibmvfc: extend async event handlers for async sub-CRQ events

This commit refactors async event handling in ibmvfc to support both main
CRQ and async sub-CRQ events via a tagged union struct
ibmvfc_async_crq_event, and adds ibmvfc_full_fpin_to_desc() and assoc_id
target matching.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index b9b571db44649..7406e7182777c 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -3483,33 +3528,59 @@ static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
>   */
>  VISIBLE_IF_KUNIT void ibmvfc_process_async_work(struct work_struct *work)
>  {
> +	struct ibmvfc_async_sub_crq *subq = NULL;
>  	struct ibmvfc_async_work *aw;
> -	struct ibmvfc_async_crq *crq;
> +	struct ibmvfc_async_crq *crq = NULL;
>  	struct ibmvfc_target *tgt;

[Severity: High]
Should tgt be initialized to NULL here?

When scsi_id, wwpn, node_name, and assoc_id are all zero,
ibmvfc_process_async_work() jumps directly to the send_fpin label before
tgt is assigned by ibmvfc_find_target():

	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;
	}

At the end of send_fpin, the cleanup checks:

	if (tgt)
		kref_put(&tgt->kref, ibmvfc_release_tgt);

Wouldn't that evaluate and potentially dereference an uninitialized tgt
pointer on the zero-identifier path?

>  	struct ibmvfc_host *vhost;
> -	struct fc_els_fpin *fpin;
> +	struct fc_els_fpin *fpin = NULL;
>  	unsigned long flags;
> +	__be64 assoc_id;
> +	__be64 node_name;
> +	__be64 scsi_id;
> +	__be64 wwpn;
> +	u8 fpin_status;
>  
>  	aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
>  	vhost = aw->vhost;
> -	crq = &aw->crq;
> +	if (aw->event.type == IBMVFC_ASYNC_CRQ_SUB) {
> +		subq = &aw->event.subq;
> +		scsi_id = 0;
> +		wwpn = subq->wwpn;
> +		fpin_status = subq->fpin_status;
> +		if (subq->flags & IBMVFC_ASYNC_ID_IS_ASSOC_ID) {
> +			node_name = 0;
> +			assoc_id = subq->id.assoc_id;
> +		} else if (subq->flags & IBMVFC_ASYNC_IS_FPIN_EXT) {
> +			node_name = 0;
> +			assoc_id = 0;
> +		} else {
> +			node_name = subq->id.node_name;
> +			assoc_id = 0;
> +		}
> +	} else {
> +		crq = &aw->event.async_crq;
> +		scsi_id = crq->scsi_id;
> +		wwpn = crq->wwpn;
> +		node_name = crq->node_name;
> +		fpin_status = crq->fpin_status;
> +		assoc_id = 0;
> +	}
>  
> -	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)
> +	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;
>  	}

[Severity: High]
Can this dereference a NULL crq pointer when handling an async sub-CRQ
event?

When aw->event.type is IBMVFC_ASYNC_CRQ_SUB, subq is populated and crq
remains NULL. If scsi_id, wwpn, node_name, and assoc_id are all zero for a
host-scope IBMVFC_AE_FPIN_LINK_CONGESTED or
IBMVFC_AE_FPIN_CONGESTION_CLEARED event, this block unconditionally calls
ibmvfc_basic_fpin_to_desc(crq, ...), which immediately dereferences
crq->fpin_status:

static struct fc_els_fpin *
ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq *crq, __be64 detecting_wwpn,
			  u64 wwpn)
{
	__be16 type;

	switch (crq->fpin_status) {

Should this path check if (crq) and dispatch to
ibmvfc_full_fpin_to_desc(subq, ...) when crq is NULL, matching the target
fallback path further down in ibmvfc_process_async_work()?

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