Re: [PATCH v9 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] Placing `cancel_work_sync(&private->io_work)` after the FSM CLOSE event in `vfio_ccw_mdev_close_device` fails to prevent `io_work` from executing concurrently with `cp_free`, leading to a use-after-free on `ccwchain` objects.
- [High] Missing synchronization against mdev removal in asynchronous CSS callbacks (`vfio_ccw_chp_event`, `vfio_ccw_sch_irq`) allows them to queue work on the `private` structure after it has been cancelled and freed.
- [High] The `private->crw` linked list is modified and accessed concurrently without any locking mechanism, leading to list corruption.
- [High] Lockless updates to eventfd contexts (`io_trigger`, `crw_trigger`) can lead to Use-After-Free.
--

--- Patch [7]: [PATCH v9 07/10] s390/vfio_ccw: cancel existing workqueues ---
commit 88059af216b0143cb97da41abcd9c5570bf7637f
Author: Eric Farman <farman@linux.ibm.com>
Subject: s390/vfio_ccw: cancel existing workqueues

This commit adds cleanup tags in _release_dev and _close_device to cancel
the io_work and crw_work workqueues. This ensures work items are not
dispatched after the private struct is freed or when the device is closed.

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 032a1cdf4df70..d361d1fde3a0d 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 missing synchronization against mdev removal
in asynchronous CSS callbacks allows them to queue work on the private
structure after it has been freed.

vfio_ccw_mdev_remove() clears the device pointer without holding sch->lock:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_remove() {
    ...
    dev_set_drvdata(&parent->dev, NULL);
    vfio_put_device(&private->vdev);
}

Concurrently, an incoming asynchronous channel path event retrieves the
private structure without synchronization:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_chp_event() {
    struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
    ...
}

If the interrupt handler thread is preempted, the removal process can
complete, freeing the private struct. When the handler resumes, it blindly
accesses private, resulting in a use-after-free.

Is there a way to serialize mdev removal with the hardware callbacks?

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

[Severity: High]
This is a pre-existing issue, but the private->crw linked list is modified
and accessed concurrently without any locking mechanism, which can lead to
list corruption.

vfio_ccw_queue_crw() appends elements to the list from interrupt context:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_queue_crw() {
    ...
    list_add_tail(&crw->next, &private->crw);
    queue_work(vfio_ccw_work_q, &private->crw_work);
}

Concurrently, vfio_ccw_crw_region_read() retrieves and deletes elements
from the list in process context without acquiring any locks:

drivers/s390/cio/vfio_ccw_chp.c:vfio_ccw_crw_region_read() {
    ...
    crw = list_first_entry_or_null(&private->crw, struct vfio_ccw_crw, next);
    if (crw)
        list_del(&crw->next);
}

Could this concurrent access cause list corruption or double frees when an
unprivileged user reads the CRW region while hardware events occur?

>  		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);

[Severity: High]
This isn't a bug introduced by this patch, but placing the cancellation
after the FSM CLOSE event leaves a race window open.

When a device is closed, vfio_ccw_mdev_close_device() triggers
vfio_ccw_fsm_event() which synchronously executes fsm_close(). This disables
the subchannel, drops the lock, and calls cp_free():

drivers/s390/cio/vfio_ccw_fsm.c:fsm_close() {
    ...
    spin_unlock_irq(&sch->lock);
    cp_free(&private->cp);
    return;
}

If a hardware interrupt queued io_work right before the subchannel was
disabled, the work item vfio_ccw_sch_io_todo() remains queued and will
execute concurrently. It calls cp_update_scsw(), which accesses the freed
cp structures:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_io_todo() {
    ...
    if (scsw_is_solicited(&irb->scsw)) {
        cp_update_scsw(&private->cp, &irb->scsw);
    ...
}

Could the cancellation be moved before the FSM event to ensure work items
are drained before the channel program is freed?

> +	cancel_work_sync(&private->crw_work);
> +

[Severity: High]
This is a pre-existing issue, but lockless updates to eventfd contexts can
lead to a use-after-free.

The io_trigger pointer is read locklessly in workqueue functions to signal
events:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_io_todo() {
    ...
    if (private->io_trigger)
        eventfd_signal(private->io_trigger);
}

Concurrently, the VFIO_DEVICE_SET_IRQS ioctl can free the context before
setting the pointer to NULL:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_set_irqs() {
    ...
    if (fd == -1) {
        if (*ctx)
            eventfd_ctx_put(*ctx);
        *ctx = NULL;
    ...
}

If the workqueue thread reads the pointer just before the ioctl drops the
reference, it will call eventfd_signal() on a freed context. Should these
eventfd updates be protected by a lock or RCU?

>  	vfio_ccw_unregister_dev_regions(private);
>  }

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