Re: [PATCH v12 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:
> - [Medium] scsi: ibmvfc: unbalanced scsi_host_put() and UAF in kunit tests due to ignored scsi_host_get() result
> --
>
> commit 0f3f9d832e1029568dc3a6eb9097fdaa04016dcc
> Author: Dave Marquardt <davemarq@linux.ibm.com>
>
> scsi: ibmvfc: add basic FPIN support
>
> This commit implements support for a basic level of Fabric Performance
> Impact Notifications (FPIN) in the ibmvfc driver. It adds an async
> event handler for IBMVFC_AE_FPIN events that offloads FPIN processing
> to a dedicated workqueue, and introduces KUnit tests for validation.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index cf775f0673a0f..e0fa9b547cbfe 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>
> [ ... ]
>
>> @@ -7120,5 +7393,21 @@ static void __exit ibmvfc_module_exit(void)
>>  	fc_release_transport(ibmvfc_transport_template);
>>  }
>>  
>> +#if IS_ENABLED(CONFIG_KUNIT)
>> +VISIBLE_IF_KUNIT struct ibmvfc_host *ibmvfc_get_first_vhost(void)
>> +{
>> +	struct ibmvfc_host *vhost = NULL;
>> +
>> +	spin_lock(&ibmvfc_driver_lock);
>> +	if (!list_empty(&ibmvfc_head))
>> +		vhost = list_first_entry(&ibmvfc_head, struct ibmvfc_host, queue);
>> +	if (vhost)
>> +		scsi_host_get(vhost->host);
>
> [Severity: Medium]
> Does this code need to check the return value of scsi_host_get()? 
>
> If it fails (for example, if the module is being concurrently unloaded and
> the host state becomes SHOST_DEL before vhost is removed from ibmvfc_head),
> this function will ignore the failure and return an unreferenced vhost
> pointer to the caller.

Yes, this is possible. In v13, if scsi_host_get() returns NULL,
ibmvfc_get_first_vhost() will return NULL.

>> +	spin_unlock(&ibmvfc_driver_lock);
>> +	return vhost;
>> +}
>> +EXPORT_SYMBOL_IF_KUNIT(ibmvfc_get_first_vhost);
>> +#endif
>
> [ ... ]
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc_kunit.c b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>> new file mode 100644
>> index 0000000000000..7cc9d423d4a79
>> --- /dev/null
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc_kunit.c
>
> [ ... ]
>
>> +static void ibmvfc_async_fpin_test(struct kunit *test)
>> +{
>
> [ ... ]
>
>> +	KUNIT_EXPECT_EQ(test, pre[IBMVFC_AE_FPIN_CONGESTION_CLEARED],
>> +			post[IBMVFC_AE_FPIN_CONGESTION_CLEARED]);
>> +
>> +	kref_put(&tgt->kref, ibmvfc_release_tgt);
>> +	put_device(&rport->dev);
>> +	scsi_host_put(vhost->host);
>
> [Severity: Medium]
> Could this unconditional scsi_host_put() cause a refcount underflow if the
> earlier scsi_host_get() call failed?
>
> The test might also perform a use-after-free by accessing members of the
> unreferenced vhost if it was already freed concurrently by the unload path.

I assume you're referring to the earlier scsi_host_get() call as above
in ibmvfc_get_first_vhost(). Yes, this is currently a problem, but in
v13 this test will bail out as ibmvfc_get_first_vhost() will return NULL
if scsi_host_get() returns NULL.

-Dave