Re: [PATCH 14/20] scsi: ibmvfc: fix UAF and stall in NVMe LS abort callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: ibmvfc: ABA vulnerability in ls_abort due to lockless reallocation
- [Medium] scsi: ibmvfc: target kref leak on H_CLOSED during abort
--

commit 2b161f932735b7864868d34990303d50d9aa8f72
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

scsi: ibmvfc: fix UAF and stall in NVMe LS abort callback

This commit attempts to fix a use-after-free and a blocking stall in
ibmvfc_nvme_ls_abort() by introducing a dedicated async completion callback
and taking the host_lock around the event lookup. It also removes the
blocking wait_for_completion() call on the timeout workqueue.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> index 52e2621a43424..7e18b79ae4ccc 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c

[ ... ]

> @@ -202,34 +203,35 @@ static void ibmvfc_nvme_ls_abort(struct nvme_fc_local_port *lport,
>  {
>  	struct ibmvfc_host *vhost = lport->private;
>  	struct ibmvfc_target *tgt = rport->private;
> -	struct ibmvfc_event *evt;
> -	union ibmvfc_iu rsp;
> +	struct ibmvfc_event *evt, *abt_evt;
>  	unsigned long flags;
> -	u16 status = IBMVFC_MAD_CRQ_ERROR;
> +
> +	spin_lock_irqsave(&vhost->host->host_lock, flags);
> +
> +	/*
> +	 * If the original LS has already completed naturally, abt_evt will
> +	 * have been freed back to the pool (evt->free set to 1 under
> +	 * host_lock by ibmvfc_free_event()).  Nothing left to cancel.
> +	 */
> +	abt_evt = ls_abort->private;
> +	if (!abt_evt || atomic_read(&abt_evt->free)) {

[Severity: High]
Is this check vulnerable to an ABA problem?

If the original command completes and frees the event, evt->free is set to 1.
If a new command immediately reallocates it, evt->free is set to 0. This
reallocation happens under queue->l_lock, not host_lock.

When this code acquires host_lock and checks atomic_read(&abt_evt->free), it
will read 0 and assume it is still the original command. Could this result
in aborting the new command instead, or dereferencing a stale target pointer
in ibmvfc_init_ls_abort()?

> +		spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> +		return;
> +	}
>  
>  	evt = ibmvfc_get_event(&vhost->crq);
> -	if (!vhost->logged_in || !evt)
> +	if (!vhost->logged_in || !evt) {
> +		spin_unlock_irqrestore(&vhost->host->host_lock, flags);
>  		return;
> +	}
>  
> -	spin_lock_irqsave(&vhost->host->host_lock, flags);
>  	kref_get(&tgt->kref);
> -	ibmvfc_init_event(evt, ibmvfc_sync_nvme_completion, IBMVFC_MAD_FORMAT);
> -	ibmvfc_init_ls_abort(evt, ls_abort);
> -	evt->sync_iu = &rsp;
> -
> -	if (ibmvfc_send_event(evt, vhost, default_timeout))
> -		goto out;
> -
> -	spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> +	ibmvfc_init_event(evt, ibmvfc_nvme_ls_abort_done, IBMVFC_MAD_FORMAT);
> +	ibmvfc_init_ls_abort(evt, abt_evt);
> +	evt->tgt = tgt;
>  
> -	wait_for_completion(&evt->comp);
> -	status = be16_to_cpu(rsp.mad_common.status);
> -	spin_lock_irqsave(&vhost->host->host_lock, flags);
> -	ibmvfc_free_event(evt);
> -out:
> +	ibmvfc_send_event(evt, vhost, default_timeout);

[Severity: Medium]
Does ignoring the return value of ibmvfc_send_event() leak the target kref?

We call kref_get(&tgt->kref) before this point and delegate the kref_put()
to the completion callback. However, if ibmvfc_send_event() returns
SCSI_MLQUEUE_HOST_BUSY (which happens when rc == H_CLOSED), it frees the
event internally without executing the callback.

Will this cause the target structure memory to be leaked?

> +	spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> -	ibmvfc_dbg(vhost, "ls_abort: cancel failed with rc=%x\n", status);
> -	kref_put(&tgt->kref, ibmvfc_release_tgt);
>  }
>  
>  static void ibmvfc_nvme_done(struct ibmvfc_event *evt)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916230921.2204729-1-tyreld@linux.ibm.com?part=14