Re: [PATCH v14 07/11] scsi: ibmvfc: extend async event handlers for async sub-CRQ events

[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: UAF and module unload crash due to untracked async work
> --
>
> commit e22506651b699f9db9776870465be492a807fad7
> Author: Dave Marquardt <davemarq@linux.ibm.com>
>
> scsi: ibmvfc: extend async event handlers for async sub-CRQ events
>
> This commit refactors async event handling to support both traditional async
> CRQs and new asynchronous sub-queue CRQs. It introduces a tagged union
> struct ibmvfc_async_crq_event to hold either event type and updates the
> corresponding handlers and FPIN processing logic.
>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> index 9c710897cd546..87ee19057551e 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
>
> [ ... ]
>
>> @@ -3629,14 +3711,14 @@ static void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
>>  			}
>>  			INIT_WORK(&aw->async_work_s, ibmvfc_process_async_work);
>>  			aw->vhost = vhost;
>> -			aw->crq = *crq;
>> +			aw->event = *ae;
>>  			schedule_work(&aw->async_work_s);
>
> [Severity: High]
> Does this code leave the scheduled work item untracked without a cancelation
> path during module unload or device removal?
>
> Looking at ibmvfc_remove(), it executes without flushing the system workqueue
> or canceling these specific work items. It then calls ibmvfc_free_mem() which
> unconditionally destroys vhost->tgt_pool via mempool_destroy():

This is the same issue as patch 4, and is being addressed.

> drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_remove() {
>     ...
>     flush_work(&vhost->rport_add_work_q);
>     fc_remove_host(vhost->host);
>     scsi_remove_host(vhost->host);
>     ...
>     ibmvfc_free_mem(vhost);
>     spin_lock(&ibmvfc_driver_lock);
>     list_del(&vhost->queue);
>     spin_unlock(&ibmvfc_driver_lock);
>     scsi_host_put(vhost->host);
> }
>
> If the work executes after the target pool is destroyed,
> ibmvfc_process_async_work() looks up a target, takes a reference, and
> eventually calls ibmvfc_release_tgt(), which triggers a use-after-free
> by calling mempool_free() on the already-destroyed pool:
>
> drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_release_tgt() {
>     struct ibmvfc_target *tgt = container_of(kref, struct ibmvfc_target, kref);
>     mempool_free(tgt, tgt->vhost->tgt_pool);
> }
>
> Furthermore, since scsi_host_get() does not hold a module reference, rmmod
> could complete while the work is still queued. Could this lead to the system
> workqueue executing unmapped memory and causing a kernel panic?

Also fixed by using a vhost-specific workqueue, which will be done in
the next revision.

-Dave