Re: [PATCH v8 07/10] s390/vfio_ccw: cancel existing workqueues

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition between mdev_remove and CSS callbacks allows work to be queued after cancel_work_sync(), leading to a Use-After-Free.
- [High] Concurrent lockless modifications to the private->crw list cause list corruption.
- [High] Eventfd contexts for IRQ triggers are leaked upon device close and release.
- [High] Unsynchronized clearing of `private->completion` can lead to stack use-after-free in the interrupt handler.
--

--- Patch [7]: [PATCH v8 07/10] s390/vfio_ccw: cancel existing workqueues ---
commit 713bc2489172cb8218b7c8bfb484ab3caaed51a1
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: cancel existing workqueues

The initialization of the io_work and crw_work workqueues begs the
question of whether they should be un-initialized. Add the corresponding
cleanup tags in _release_dev to ensure work isn't dispatched after
the private struct is free'd.

Suggested-by: Matthew Rosato <mjrosato@linux.ibm.com>
Fixes: e5f84dbaea59 ("vfio: ccw: return I/O results asynchronously")
Fixes: 3f02cb2fd9d2 ("vfio-ccw: Wire up the CRW irq and CRW region")
Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 032a1cdf4df70..bc8eb485d03fd 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -131,6 +131,13 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  	struct vfio_ccw_crw *crw, *temp;
>  
> +	/*
> +	 * Ensure these work items are fully drained, so none can
> +	 * fire after being released.
> +	 */
> +	cancel_work_sync(&private->io_work);
> +	cancel_work_sync(&private->crw_work);

[Severity: High]
This is a pre-existing issue, but does this fully prevent the work from
being dispatched after the private struct is freed?

If vfio_ccw_mdev_remove() clears drvdata without holding sch->lock:

vfio_ccw_mdev_remove() {
    ...
    dev_set_drvdata(&parent->dev, NULL);
    ...
}

And a concurrent hardware channel path event triggers vfio_ccw_chp_event()
which fetches the private pointer locklessly:

vfio_ccw_chp_event() {
    ...
    struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
    ...
}

Could vfio_ccw_chp_event() queue private->crw_work after this
cancel_work_sync() call has already finished, leading to a use-after-free
when private is subsequently freed?

> +
>  	list_for_each_entry_safe(crw, temp, &private->crw, next) {

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to iterate and
delete items from private->crw without holding a lock here?

In vfio_ccw_queue_crw(), elements are added to the list while holding
sch->lock:

vfio_ccw_queue_crw() {
    ...
    list_add_tail(&crw->next, &private->crw);
    ...
}

However, vfio_ccw_crw_region_read() removes items without holding sch->lock
or any other synchronization:

vfio_ccw_crw_region_read() {
    ...
    if (crw)
        list_del(&crw->next);
    ...
}

Can concurrent asynchronous hardware events and userspace reads corrupt this
doubly-linked list?

>  		list_del(&crw->next);
>  		kfree(crw);

[ ... ]

> @@ -202,6 +209,14 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  
>  	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
> +
> +	/*
> +	 * Ensure these work items are drained, in the event the
> +	 * device is re-opened instead of released
> +	 */
> +	cancel_work_sync(&private->io_work);
> +	cancel_work_sync(&private->crw_work);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this code leak the
eventfd contexts for the IRQ triggers?

Userspace sets IRQ triggers via vfio_ccw_mdev_set_irqs(), which takes a
reference to the eventfd_ctx.

If userspace exits or crashes without explicitly disabling the IRQs, neither
vfio_ccw_mdev_close_device() nor vfio_ccw_mdev_release_dev() appear to call
eventfd_ctx_put() for io_trigger, crw_trigger, or req_trigger.

[Severity: High]
This is a pre-existing issue, but can the unsynchronized clearing of
private->completion lead to a stack use-after-free in the interrupt handler?

In vfio_ccw_sch_quiesce(), if wait_for_completion_timeout() times out, it
clears the pointer without holding sch->lock:

vfio_ccw_sch_quiesce() {
    ...
    private->completion = NULL;
    ...
}

Concurrently, fsm_irq() might read the non-NULL pointer:

fsm_irq() {
    ...
    if (private->completion)
        complete(private->completion);
    ...
}

If quiesce returns and destroys the on-stack completion frame before
complete() executes, could the interrupt handler corrupt the stack?

>  	vfio_ccw_unregister_dev_regions(private);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728013509.1551753-1-farman@linux.ibm.com?part=7