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

On 8/21/26 8:44 AM, Dave Marquardt wrote:
> [email protected] writes:
> 
>> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>>
>> New issues:
>> - [High] The newly added async_sub_crq is leaked and leaves its IRQ registered if ibmvfc_alloc_channels() fails during SCSI channel initialization.
>> - [High] The async_sub_crq is completely omitted from the CRQ reset and re-enable sequences, breaking its lifecycle.
>>
>> Pre-existing issues:
>> - [Critical] NULL pointer dereference in ibmvfc_channel_setup() when mq_enabled == 0.
>> - [High] Missing bounds checks on hypervisor-provided queue counts can lead to out-of-bounds array accesses.
>> - [High] Error codes from request_irq() are unconditionally clobbered, silently masking critical failures.
>> --
>>
>> --- Patch [7]: [PATCH v6 7/8] scsi: ibmvfc: register and use asynchronous sub CRQ for events ---
>> commit 10af0529db6ade6b831181ddc695f2e8713b38ee
>> Author: Dave Marquardt <[email protected]>
>>
>> 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
>>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> [ ... ]
>>> @@ -5792,6 +5794,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]);
>>
>> [Severity: High]
>> This is a pre-existing issue, but is it possible for an unexpectedly large
>> response from the hypervisor to cause out-of-bounds array accesses here?
> 
> Up to this point in the driver history, we have trusted the answeres
> from PHYP, the IBM PowerVM hypervisor.
> 
>> Looking at drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_channel_setup_done(),
>> the unvalidated active_queues count returned by the VIOS is used directly
>> as a loop limit to write into the driver's nvme->scrqs and scsi->scrqs
>> arrays.
>>
>> Could a malformed or compromised response corrupt adjacent memory?
>>
>>> +		vhost->async_sub_crq.vios_cookie = be64_to_cpu(setup->async_sub_crq_handle);
>>>  
>>>  		ibmvfc_dbg(vhost, "Using %u SCSI channels\n",
>>>  			   scsi->active_queues);
>> [ ... ]
>>> @@ -5851,6 +5854,7 @@ static void ibmvfc_channel_setup(struct ibmvfc_host *vhost)
>>>  		for (i = 0; i < nvme_channels; i++)
>>>  			setup_buf->channel_handles[scsi_channels + i] =
>>>  				cpu_to_be64(nvme->scrqs[i].cookie);
>>
>> [Severity: Critical]
>> This is a pre-existing issue, but does this loop risk a NULL pointer
>> dereference when mq_enabled is 0?
>>
>> Looking at drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_channel_setup(),
>> if the driver is loaded with mq_enabled=0, ibmvfc_init_sub_crqs() returns
>> early without allocating scsi_scrqs.scrqs. 
>>
>> However, if a modern VIOS supports channels, the enquiry succeeds and sets
>> max_vios_scsi_channels > 0.
>>
>> This would cause ibmvfc_channel_setup() to compute scsi_channels > 0 and
>> unconditionally execute this loop, accessing the unallocated
>> scsi->scrqs[i].cookie array.
> 
> I will study this more, but it will not be fixed in the v7 patch series.

If mq=0 we do not do enquiry or channel setup, and fall back to legacy CRQ only
mode. This is a false-positive.

-Tyrel