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

[Dave Marquardt <davemarq@linux.ibm.com>]

sashiko-bot@kernel.org writes:

> 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?

We've always assumed the VIOS acts sanely. This issue will not be fixed
at this time.

> [ ... ]
>
>> @@ -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?

A new check against scrq->msgs.handle will be added to
ibmvfc_deregister_channel() in the next revision of this patch,
preventing memset() against a NULL pointer.

-Dave