Re: [PATCH v14 10/11] scsi: ibmvfc: register and use asynchronous sub CRQ for events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] scsi: ibmvfc: DMA Use-After-Free due to skipped sub-CRQ deregistration
- [High] scsi: ibmvfc: data race on host state bitfields due to lockless mq_enabled write
- [High] scsi: ibmvfc: missing bounds check on active_queues causes heap overflow
--

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

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

Wire the async sub-CRQ into the sub-CRQ lifecycle so it is allocated,
registered, and freed alongside the SCSI channel queues.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index b12655f028879..594e5f136040a 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -5817,6 +5824,7 @@ static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
>  		vhost->do_enquiry = 0;
>  		scsi->active_queues = be32_to_cpu(setup->num_scsi_subq_channels);
>  		nvme->active_queues = be32_to_cpu(setup->num_nvme_subq_channels);

[Severity: High]
Does this code overflow scsi->scrqs[] or nvme->scrqs[] if the hypervisor
returns a value for setup->num_scsi_subq_channels or
setup->num_nvme_subq_channels that is larger than the allocated
scsi->max_queues or nvme->max_queues?

The active_queues values are read directly from the hypervisor response
without any bounds checking, and are then used as limits in the
initialization loops below.

If a buggy or malicious hypervisor sends excessively large values, could it
cause an out-of-bounds write into the scrqs array or cause page faults on
the setup buffer?

> +		async->active_queues = (async->scrqs && setup->async_sub_crq_handle) ? 1 : 0;
>  
>  		if (flags & IBMVFC_CHANNELS_CANCELED) {
>  			ibmvfc_dbg(vhost, "Channels Canceled\n");
> @@ -5832,6 +5840,8 @@ 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]);
> +		for (i = 0; i < async->active_queues; i++)
> +			async->scrqs[i].vios_cookie = be64_to_cpu(setup->async_sub_crq_handle);
>  
>  		ibmvfc_dbg(vhost, "Using %u SCSI channels\n",
>  			   scsi->active_queues);
[ ... ]
> @@ -6988,6 +7004,7 @@ static void ibmvfc_reg_sub_crqs(struct ibmvfc_host *vhost,
>  			for (j = i; j > 0; j--)
>  				ibmvfc_deregister_channel(vhost, channels, j - 1);
>  			vhost->do_enquiry = 0;
> +			vhost->mq_enabled = 0;

[Severity: High]
Since vhost->do_enquiry and vhost->mq_enabled are bitfields, these
assignments compile into read-modify-write operations on the containing
word.

ibmvfc_reg_sub_crqs() is called by worker threads like
ibmvfc_reenable_crq_queue() outside of the host_lock.

If a tasklet concurrently processes an event and calls a completion handler
like ibmvfc_channel_setup_done() (which executes under host_lock and writes
to another bitfield in the same word, such as vhost->using_channels = 1),
could this lockless read-modify-write overwrite the locked modifications
and corrupt the host state machine flags?

[Severity: Critical]
If channel registration fails here, vhost->mq_enabled is cleared to 0.

Since async_scrqs is successfully registered earlier in
ibmvfc_init_sub_crqs(), when teardown happens, ibmvfc_release_sub_crqs()
will call ibmvfc_dereg_sub_crqs() for async_scrqs.

However, ibmvfc_dereg_sub_crqs() returns immediately if
!vhost->mq_enabled:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_dereg_sub_crqs() {
    if (!vhost->mq_enabled || !channels->scrqs)
        return;
    ...
}

Will this cause ibmvfc_dereg_sub_crqs() to bypass deregistering the async
sub-CRQ, leaving the hypervisor DMA mappings and IRQ handlers active while
the memory is subsequently freed by ibmvfc_release_channels()?

>  			return;
>  		}
>  	}

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