Re: [PATCH v9 8/9] scsi: ibmvfc: register and use asynchronous sub CRQ for events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Unconditional registration of `async_sub_crq` without checking `vhost->mq_enabled` causes DMA Use-After-Free and memory corruption.
--

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

scsi: ibmvfc: register and use asynchronous sub CRQ for events

This commit wires the async sub-CRQ into the sub-CRQ lifecycle, so it is
allocated, registered, and freed alongside the SCSI channel queues. It
adds calls to register and deregister the async sub-CRQ in the reset and
reenable paths.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -986,6 +989,7 @@ static int ibmvfc_reenable_crq_queue(struct ibmvfc_host *vhost)
>  	spin_unlock(vhost->crq.q_lock);
>  	spin_unlock_irqrestore(&vhost->host->host_lock, flags);
>  
> +	ibmvfc_register_channel(vhost, &vhost->scsi_scrqs, -1);

[Severity: Critical]
Does this code unconditionally register the async sub-CRQ without checking
if vhost->mq_enabled is set?

If mq_enabled is true initially but ibmvfc_init_sub_crqs() fails (for
example, due to a channel allocation failure), async_sub_crq is freed via
ibmvfc_free_queue(). This unmaps the DMA address and frees the page, but
scrq->msg_token retains the stale DMA address, and vhost->mq_enabled is
set to 0.

When a queue re-enable or CRQ reset occurs, this unconditional call to
ibmvfc_register_channel() passes the stale msg_token to the hypervisor:

ibmvfc_register_channel() {
    ...
    rc = h_reg_sub_crq(vdev->unit_address, scrq->msg_token, PAGE_SIZE,
                       &scrq->cookie, &scrq->hw_irq);
    ...
}

Can this cause the hypervisor to write async events into a freed memory page,
resulting in a DMA use-after-free regression? Furthermore, if mq_enabled was
false from the start, could this pass a DMA address of 0 to the hypervisor,
resulting in IOMMU faults or hypervisor errors?

>  	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
>  
[ ... ]
> @@ -1042,6 +1047,7 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)
>  	spin_unlock(vhost->crq.q_lock);
>  	spin_unlock_irqrestore(&vhost->host->host_lock, flags);
>  
> +	ibmvfc_register_channel(vhost, &vhost->scsi_scrqs, -1);

[Severity: Critical]
This unconditional registration appears to raise the same DMA use-after-free
concerns during a CRQ reset as noted in ibmvfc_reenable_crq_queue() above.
Could this be guarded by checking vhost->mq_enabled?

>  	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-ibmvfc-fpin-support-v8-0-b27183b055af@linux.ibm.com?part=8