Re: [PATCH v14 04/11] scsi: ibmvfc: add basic FPIN support

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

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] scsi: ibmvfc: missing flush of dynamically allocated system_wq items causes UAF
> --
>
> commit 170496a5dbc89bab0a780529ef0e294023ca569c
> Author: Dave Marquardt <davemarq@linux.ibm.com>
>
> scsi: ibmvfc: add basic FPIN support
>
> Implement support for a basic level of Fabric Performance Impact
> Notifications (FPIN) in the ibmvfc driver to enable monitoring of
> fabric congestion and link integrity events.
>
> Add async event handler for IBMVFC_AE_FPIN events that offloads FPIN
> processing via schedule_work(). Convert VIOS FPIN messages to standard
> fc_els_fpin structures and pass them to fc_host_fpin_rcv() for processing
> by the FC transport layer.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 0fe6b6f4260ba..871decab29856 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> [ ... ]
>> @@ -3261,6 +3266,255 @@ static const char *ibmvfc_get_link_state(enum ibmvfc_ae_link_state state)
>>  	return "";
>>  }
>>  
> [ ... ]
>> +VISIBLE_IF_KUNIT void ibmvfc_process_async_work(struct work_struct *work)
>> +{
>> +	struct ibmvfc_async_work *aw;
>> +	struct ibmvfc_async_crq *crq;
>> +	struct ibmvfc_target *tgt;
>> +	struct ibmvfc_host *vhost;
>> +	struct fc_els_fpin *fpin;
>> +	unsigned long flags;
>> +
>> +	aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
>> +	vhost = aw->vhost;
>> +	crq = &aw->crq;
> [ ... ]
>> +	fpin = ibmvfc_basic_fpin_to_desc(crq, tgt->wwpn);
>> +	if (fpin) {
>> +		fc_host_fpin_rcv(tgt->vhost->host,
>> +				 sizeof(*fpin) + be32_to_cpu(fpin->desc_len),
>> +				 (char *)fpin, 0);
>> +		kfree(fpin);
>> +	} else
>> +		dev_err_ratelimited(vhost->dev, "FPIN event received, unable to process\n");
>> +
>> +	kref_put(&tgt->kref, ibmvfc_release_tgt);
>
> [Severity: High]
> Can this untracked async worker drop its reference and trigger the release
> function on a destroyed mempool?
>
> If ibmvfc_remove() has already completed and destroyed vhost->tgt_pool,
> this final kref_put() triggers ibmvfc_release_tgt():
>
> drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_release_tgt() {
> 	...
> 	mempool_free(tgt, tgt->vhost->tgt_pool);
> }
>
> This accesses the destroyed target mempool, causing a Use-After-Free.

Agreed.

>> + free:
>> +	scsi_host_put(aw->vhost->host);
>> +	kfree(aw);
>> +}
>> +EXPORT_SYMBOL_IF_KUNIT(ibmvfc_process_async_work);
>> +
>>  /**
>>   * ibmvfc_handle_async - Handle an async event from the adapter
>>   * @crq:	crq to process
> [ ... ]
>> @@ -3361,12 +3616,28 @@ static void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
>>  	case IBMVFC_AE_HALT:
>>  		ibmvfc_link_down(vhost, IBMVFC_HALTED);
>>  		break;
>> +	case IBMVFC_AE_FPIN:
>> +		if (vhost->state == IBMVFC_HOST_OFFLINE)
>> +			break;
>> +		aw = kzalloc_obj(*aw, GFP_ATOMIC);
>> +		if (aw) {
>> +			if (!scsi_host_get(vhost->host)) {
>> +				kfree(aw);
>> +				break;
>> +			}
>> +			INIT_WORK(&aw->async_work_s, ibmvfc_process_async_work);
>> +			aw->vhost = vhost;
>> +			aw->crq = *crq;
>> +			schedule_work(&aw->async_work_s);
>
> [Severity: High]
> Does this code risk a Use-After-Free during teardown?
>
> Since aw is queued to the global system_wq without being tracked,
> there doesn't seem to be a way to flush it in ibmvfc_remove().
>
> If ibmvfc_remove() runs concurrently with this pending work item, it calls
> ibmvfc_free_mem() which unconditionally destroys the target mempool:
>
> drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_free_mem() {
> 	...
> 	mempool_destroy(vhost->tgt_pool);
> 	...
> }
>
> This could lead to a crash when ibmvfc_process_async_work() completes and
> drops its reference.
>
> Additionally, because it is untracked on the global workqueue, it can execute
> after the module is unloaded, leading to a fatal instruction page fault.

Either ibmvfc needs to track its work items on the system work queues
and flush them all, or use its own work queue and ensure the work is all
done before calling ibmvfc_free_mem().

-Dave