Re: [PATCH v14 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 3 potential issue(s) to consider:
> - [Critical] scsi: ibmvfc: DMA Use-After-Free due to skipped sub-CRQ deregistration
> - [High] scsi: ibmvfc: data race on host state bitfields due to lockless mq_enabled write
> - [High] scsi: ibmvfc: missing bounds check on active_queues causes heap overflow
> --
>
> commit 4d5dd0876862baa994058a6b3ad652d84e0497a5
> 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.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index b12655f028879..594e5f136040a 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> [ ... ]
>> @@ -5817,6 +5824,7 @@ static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
>>  		vhost->do_enquiry = 0;
>>  		scsi->active_queues = be32_to_cpu(setup->num_scsi_subq_channels);
>>  		nvme->active_queues = be32_to_cpu(setup->num_nvme_subq_channels);
>
> [Severity: High]
> Does this code overflow scsi->scrqs[] or nvme->scrqs[] if the hypervisor
> returns a value for setup->num_scsi_subq_channels or
> setup->num_nvme_subq_channels that is larger than the allocated
> scsi->max_queues or nvme->max_queues?
>
> The active_queues values are read directly from the hypervisor response
> without any bounds checking, and are then used as limits in the
> initialization loops below.
>
> If a buggy or malicious hypervisor sends excessively large values, could it
> cause an out-of-bounds write into the scrqs array or cause page faults on
> the setup buffer?

I will clamp the values to the minimum of the hypervisor value and the
max_queues value of the ibmvfc_channel structures.

>> +		async->active_queues = (async->scrqs && setup->async_sub_crq_handle) ? 1 : 0;
>>  
>>  		if (flags & IBMVFC_CHANNELS_CANCELED) {
>>  			ibmvfc_dbg(vhost, "Channels Canceled\n");
>> @@ -5832,6 +5840,8 @@ 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]);
>> +		for (i = 0; i < async->active_queues; i++)
>> +			async->scrqs[i].vios_cookie = be64_to_cpu(setup->async_sub_crq_handle);
>>  
>>  		ibmvfc_dbg(vhost, "Using %u SCSI channels\n",
>>  			   scsi->active_queues);
> [ ... ]
>> @@ -6988,6 +7004,7 @@ static void ibmvfc_reg_sub_crqs(struct ibmvfc_host *vhost,
>>  			for (j = i; j > 0; j--)
>>  				ibmvfc_deregister_channel(vhost, channels, j - 1);
>>  			vhost->do_enquiry = 0;
>> +			vhost->mq_enabled = 0;
>
> [Severity: High]
> Since vhost->do_enquiry and vhost->mq_enabled are bitfields, these
> assignments compile into read-modify-write operations on the containing
> word.
>
> ibmvfc_reg_sub_crqs() is called by worker threads like
> ibmvfc_reenable_crq_queue() outside of the host_lock.
>
> If a tasklet concurrently processes an event and calls a completion handler
> like ibmvfc_channel_setup_done() (which executes under host_lock and writes
> to another bitfield in the same word, such as vhost->using_channels = 1),
> could this lockless read-modify-write overwrite the locked modifications
> and corrupt the host state machine flags?

Yes. I'll add holding the host lock while making these updates.

> [Severity: Critical]
> If channel registration fails here, vhost->mq_enabled is cleared to 0.
>
> Since async_scrqs is successfully registered earlier in
> ibmvfc_init_sub_crqs(), when teardown happens, ibmvfc_release_sub_crqs()
> will call ibmvfc_dereg_sub_crqs() for async_scrqs.
>
> However, ibmvfc_dereg_sub_crqs() returns immediately if
> !vhost->mq_enabled:
>
> drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_dereg_sub_crqs() {
>     if (!vhost->mq_enabled || !channels->scrqs)
>         return;
>     ...
> }
>
> Will this cause ibmvfc_dereg_sub_crqs() to bypass deregistering the async
> sub-CRQ, leaving the hypervisor DMA mappings and IRQ handlers active while
> the memory is subsequently freed by ibmvfc_release_channels()?

Fixed in other patch fixes.

-Dave