Re: [PATCH v7 7/8] 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:
- [High] Unconditional deregistration of the uninitialized async sub-CRQ causes a NULL pointer dereference.

Pre-existing issues:
- [High] Buffer overflow due to out-of-bounds array writes and reads when processing unvalidated queue counts from the VIOS.
--

commit 908a7af2b04b0b17f4fc0a40814f0dacec88332a
Author: Dave Marquardt <davemarq@linux.ibm.com>
scsi: ibmvfc: register and use asynchronous sub CRQ for events

This commit wires the asynchronous sub-CRQ into the sub-CRQ lifecycle so it is
allocated, registered, and freed alongside the SCSI channel queues. It modifies
multi-queue initialization and teardown paths to accommodate the new queue and
advertises relevant capabilities to the VIOS.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index cc1ef82580608..adc730af67d2b 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

> @@ -5800,6 +5808,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);
>  
>  		ibmvfc_dbg(vhost, "Using %u SCSI channels\n",

[Severity: High]
This is a pre-existing issue, but can this out-of-bounds write occur if the
VIOS returns untrusted queue counts?

In ibmvfc_channel_setup_done(), the driver reads num_scsi_subq_channels and
num_nvme_subq_channels from the VIOS response into active_queues without
validating them against the dynamically allocated max_queues array size:

ibmvfc_channel_setup_done() {
    scsi->active_queues = be32_to_cpu(setup->num_scsi_subq_channels);
    nvme->active_queues = be32_to_cpu(setup->num_nvme_subq_channels);
    ...
}

The loop just above this patch hunk then directly uses this unvalidated value
to write to the scrqs array:

ibmvfc_channel_setup_done() {
    ...
    for (i = 0; i < scsi->active_queues; i++)
        scsi->scrqs[i].vios_cookie =
            be64_to_cpu(setup->channel_handles[i]);
    ...
}

Could a compromised or malfunctioning VIOS provide values that exceed the
locally allocated array size and cause a buffer overflow here?

[ ... ]

> @@ -7055,6 +7087,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);
> +
>  	if (!vhost->scsi_scrqs.scrqs)
>  		return;

[Severity: High]
Does this unconditional teardown cause a NULL pointer dereference when
multi-queue is disabled?

During module unload or reset, if multi-queue was disabled (mq_enabled=0) or
initialization failed, vhost->async_sub_crq is left zero-initialized.

Unconditionally calling ibmvfc_deregister_channel() routes to a cleanup path
that behaves as follows:

ibmvfc_deregister_channel() {
    ...
    /* Clean out the queue */
    memset(scrq->msgs.crq, 0, PAGE_SIZE);
    ...
}

Since msgs.crq aliases the unallocated msgs.handle (which is NULL), will this
memset() write 4KB of zeroes to a NULL pointer and cause a deterministic
kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-ibmvfc-fpin-support-v7-0-77d0b9809080@linux.ibm.com?part=7