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

[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.

>> +		setup_buf->async_sub_crq_handle = cpu_to_be64(vhost->async_sub_crq.cookie);
>>  	}
>>  
>>  	ibmvfc_init_event(evt, ibmvfc_channel_setup_done, IBMVFC_MAD_FORMAT);
> [ ... ]
>> @@ -6949,7 +6953,9 @@ static void ibmvfc_reg_sub_crqs(struct ibmvfc_host *vhost,
>>  	for (i = 0; i < channels->max_queues; i++) {
>>  		if (ibmvfc_register_channel(vhost, channels, i)) {
>
> [Severity: High]
> This is a pre-existing issue, but does ibmvfc_register_channel() silently
> mask critical failures?
>
> Looking at drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_register_channel(),
> if request_irq() fails, the code jumps to the irq_failed label where it
> executes:
>
> rc = plpar_hcall_norets(H_FREE_SUB_CRQ, vdev->unit_address, scrq->cookie);
>
> If the hypervisor successfully frees the sub-CRQ, rc is overwritten with 0
> (H_SUCCESS). 
>
> The function then returns 0, falsely indicating success to the caller,
> which leaves the channel marked active but without an interrupt handler.

Good catch. In v7 I've changed the call to record the returned value in
a different variable, hcall_rc, so the original rc can be returned to
the caller of ibmvfc_register_channel().

>>  			for (j = i; j > 0; j--)
>> -				ibmvfc_deregister_channel(vhost, channels, j - 1);
>> +				ibmvfc_deregister_channel(
>> +					vhost, channels, j - 1);
>> +
>>  			vhost->do_enquiry = 0;
>>  			return;
>>  		}
> [ ... ]
>> @@ -7004,10 +7010,27 @@ static int ibmvfc_alloc_channels(struct ibmvfc_host *vhost,
>>  
>>  static void ibmvfc_init_sub_crqs(struct ibmvfc_host *vhost)
>>  {
> [ ... ]
>> +	/* register async_sub_crq channel */
>> +	if (ibmvfc_register_channel(vhost, &vhost->scsi_scrqs, -1)) {
>> +		ibmvfc_free_queue(vhost, &vhost->async_sub_crq);
>> +		vhost->do_enquiry = 0;
>> +		vhost->mq_enabled = 0;
>> +		return;
>> +	}
>> +
>>  	if (ibmvfc_alloc_channels(vhost, &vhost->scsi_scrqs)) {
>>  		vhost->do_enquiry = 0;
>>  		vhost->mq_enabled = 0;
>>  		return;
>>  	}
>
> [Severity: High]
> Does this error path leak the newly added async_sub_crq?

Yes. In v7 I've restructured ibmvfc_init_sub_crqs() to handle errors
correctly and not leak async_sub_crq.

> Looking at drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_init_sub_crqs(),
> if ibmvfc_alloc_channels() fails during SCSI channel initialization, the
> function returns early.
>
> This skips deregistering or freeing the successfully allocated
> async_sub_crq.
>
>> @@ -7050,6 +7073,9 @@ static void ibmvfc_release_sub_crqs(struct ibmvfc_host *vhost)
>>  	if (!vhost->scsi_scrqs.scrqs)
>>  		return;
>
> [Severity: High]
> Could this early return bypass async_sub_crq cleanup entirely?
>
> Looking at drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_release_sub_crqs(),
> if scsi_scrqs allocation failed earlier, !vhost->scsi_scrqs.scrqs evaluates
> to true, causing an immediate return. 
>
> When the module unloads, the memory for async_sub_crq is freed but its
> IRQ remains registered, potentially causing a use-after-free panic if the
> hardware triggers an interrupt.

v7 of this patch fixes this problem.

>> +	ibmvfc_deregister_channel(vhost, &vhost->scsi_scrqs, -1);
>> +	ibmvfc_free_queue(vhost, &vhost->async_sub_crq);
>> +
>>  	ibmvfc_dereg_sub_crqs(vhost, &vhost->scsi_scrqs);
>
> [Severity: High]
> Are there lifecycle regressions during host adapter resets for async_sub_crq?
>
> Looking at drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_reset_crq(),
> when the host adapter undergoes a reset, ibmvfc_reset_crq() and
> ibmvfc_reenable_crq_queue() explicitly deregister and re-register the SCSI
> and NVMe sub-CRQs. 
>
> However, async_sub_crq appears to be omitted from these sequences.
>
> Across a reset, the async_sub_crq registration with the hypervisor becomes
> stale, which would permanently break the receipt of asynchronous events
> until the driver is reloaded. 
>
> Should async_sub_crq be included in the reset path?

Yes. v7 adds code to ibmvfc_reset_crq() and ibmvfc_reenable_crq_queue()
to deregister and reregister the async sub CRQ.

-Dave