Re: [PATCH v13 10/11] 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:
> - [High] scsi: ibmvfc: host state machine stall if ibmvfc_reg_sub_crqs() fails
> - [High] scsi: ibmvfc: NULL pointer deref in channel setup when mq_enabled=0
> --
>
> commit 3dd02df8d62690f440ff03238afa2613397c578a
> 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. Updates
> ibmvfc_set_login_info() to advertise related capabilities.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 76246002ac724..acc11f3a237e1 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> [ ... ]
>> @@ -5821,6 +5828,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]);
>> +		async->scrqs[0].vios_cookie = be64_to_cpu(setup->async_sub_crq_handle);
>
> [Severity: High]
> Can this cause a NULL pointer dereference when the module parameter mq_enabled
> is 0?
>
> If mq_enabled is 0, ibmvfc_init_sub_crqs() returns early and skips allocation,
> leaving async->scrqs as NULL but vhost->do_enquiry as 1.
>
> If the VIOS later advertises IBMVFC_CAN_SUPPORT_CHANNELS during NPIV login,
> ibmvfc_npiv_login_done() sees do_enquiry is true and issues a channel enquiry.
> When that completes, we reach this code and unconditionally dereference the
> unallocated async->scrqs array.

Yes, it is a problem! I will fix in v14. 

> [ ... ]
>> @@ -5880,6 +5889,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);
>> +		setup_buf->async_sub_crq_handle = cpu_to_be64(async->scrqs[0].cookie);
>>  	}
>
> [Severity: High]
> Similarly, can this unconditional access to async->scrqs[0].cookie trigger a
> NULL pointer dereference when mq_enabled is 0?

Yes.

> [ ... ]
>> @@ -7023,17 +7039,38 @@ static int ibmvfc_alloc_channels(struct ibmvfc_host *vhost,
> [ ... ]
>>  static void ibmvfc_init_sub_crqs(struct ibmvfc_host *vhost)
>>  {
>>  	ENTER;
>>  	if (!vhost->mq_enabled)
>>  		return;
>>  
>> -	if (ibmvfc_alloc_channels(vhost, &vhost->scsi_scrqs)) {
>> -		vhost->do_enquiry = 0;
>> -		vhost->mq_enabled = 0;
>> -		return;
>> -	}
>> +	/* register async_sub_crq channel */
>> +	if (ibmvfc_alloc_channels(vhost, &vhost->async_scrqs))
>> +		goto out;
>> +
>> +	ibmvfc_reg_sub_crqs(vhost, &vhost->async_scrqs);
>
> [Severity: High]
> Does this cause a host state machine stall if ibmvfc_reg_sub_crqs() fails?
>
> If ibmvfc_reg_sub_crqs() fails internally, it clears vhost->do_enquiry but
> leaves vhost->mq_enabled set to 1. Because mq_enabled remains true,
> ibmvfc_set_login_info() continues to advertise IBMVFC_CAN_USE_CHANNELS to
> the VIOS.
>
> When the VIOS responds with IBMVFC_CAN_SUPPORT_CHANNELS,
> ibmvfc_npiv_login_done() enters the multi-queue branch, but because
> do_enquiry is now 0, it skips calling ibmvfc_channel_enquiry().
>
> It also bypasses the fallback block that would normally set
> IBMVFC_HOST_ACTION_QUERY and wake up vhost->work_wait_q:
>
> drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_npiv_login_done() {
>     if (ibmvfc_check_caps(vhost, IBMVFC_CAN_SUPPORT_CHANNELS)) {
>         if (ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_SCSI))
>             vhost->do_scsi_login = 1;
>         if (ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_NVMEOF))
>             vhost->do_nvme_login = 1;
>         if (vhost->do_enquiry)
>             ibmvfc_channel_enquiry(vhost);
>     } else {
>         vhost->do_enquiry = 0;
>         ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_QUERY);
>         wake_up(&vhost->work_wait_q);
>     }
> }
>
> Will this cause the initialization process to stall indefinitely in
> IBMVFC_HOST_ACTION_INIT_WAIT?

Yes, this appears to cause a stall.

-Dave