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 2 potential issue(s) to consider:

New issues:
- [Critical] Unconditional lifecycle management of `async_sub_crq` causes Use-After-Free, double-free, and DMA corruption via uninitialized addresses.

Pre-existing issues:
- [Critical] Missing bounds check on VIOS-provided queue counts leads to out-of-bounds heap writes.
--

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

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

This patch wires the async sub-CRQ into the sub-CRQ lifecycle so it is
allocated, registered, and freed alongside the SCSI channel queues. It also
fixes a variable-shadowing bug in channel registration and advertises
capabilities when multi-queue channels are enabled.

> 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
[ ... ]
> @@ -1006,6 +1010,7 @@ static int ibmvfc_reset_crq(struct ibmvfc_host *vhost)
>  	struct vio_dev *vdev = to_vio_dev(vhost->dev);
>  	struct ibmvfc_queue *crq = &vhost->crq;
>  
> +	ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
>  	ibmvfc_dereg_sub_crqs(vhost, &vhost->scsi_scrqs);
>  	ibmvfc_dereg_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]
Does this code unconditionally register the async sub-CRQ during a host reset,
even if multi-queue is disabled or the queue was never allocated? 

If a system boots with multi-queue disabled (or if its initialization fails),
the async_sub_crq remains unallocated. When a host reset occurs, this call
reaches h_reg_sub_crq() with an uninitialized msg_token (value 0):

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

Could this instruct the hypervisor to map and write to guest physical address 0,
causing DMA corruption?

>  	ibmvfc_reg_sub_crqs(vhost, &vhost->scsi_scrqs);
>  	ibmvfc_reg_sub_crqs(vhost, &vhost->nvme_scrqs);
[ ... ]
> @@ -5812,6 +5820,7 @@ static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
>  		for (i = 0; i < nvme->active_queues; i++)
>  			nvme->scrqs[i].vios_cookie =
>  				be64_to_cpu(setup->channel_handles[scsi->active_queues + i]);
> +		vhost->async_sub_crq.vios_cookie = be64_to_cpu(setup->async_sub_crq_handle);

[Severity: Critical]
This isn't a bug introduced by this patch, but is it possible for the
untrusted VIOS input in the channel setup response to overflow the queues
array?

In ibmvfc_channel_setup_done(), the driver sets the loop boundaries by reading
active_queues from setup->num_scsi_subq_channels and
setup->num_nvme_subq_channels. If the VIOS returns a count larger than
the array capacity (max_queues), does this loop blindly write
setup->channel_handles[i] past the end of the scrqs array, potentially
causing out-of-bounds writes to the kernel heap?

>  
>  		ibmvfc_dbg(vhost, "Using %u SCSI channels\n",
>  			   scsi->active_queues);
[ ... ]
> @@ -7070,6 +7104,9 @@ static void ibmvfc_release_channels(struct ibmvfc_host *vhost,
>  static void ibmvfc_release_sub_crqs(struct ibmvfc_host *vhost)
>  {
>  	ENTER;
> +	ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
> +	ibmvfc_free_queue(vhost, &vhost->async_sub_crq);
> +

[Severity: Critical]
If ibmvfc_alloc_channels() fails during ibmvfc_init_sub_crqs(), the
async_sub_crq is freed but its state is not cleared. When the driver
later unloads or releases queues, does this unconditional call free the
queue a second time?

Could this lead to a double free of the event pool array and a use-after-free
on the event queue list?

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