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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The `private->crw` list is manipulated without locks causing list corruption, and `cancel_work_sync()` is placed after list teardown.
- [Critical] Concurrent execution of CSS event handlers and `vfio_ccw_mdev_remove` leads to Use-After-Free of the `private` struct.
- [High] Eventfd contexts for IRQ triggers are permanently leaked when the device is closed.
- [High] Data race on `private->irb` leading to corrupted I/O status being exposed to userspace.
- [High] Unprotected access to eventfd triggers can lead to Use-After-Free during event signaling.
--

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

s390/vfio_ccw: cancel existing workqueues

This commit adds cancel_work_sync() calls in vfio_ccw_mdev_release_dev()
to ensure work items are canceled and not dispatched after the private struct
is freed.

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 032a1cdf4df70..9048c1e6e7f0c 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -136,6 +136,9 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  		kfree(crw);
>  	}

[Severity: Critical]
Is it safe to place cancel_work_sync(&private->crw_work) after the teardown
of the private->crw list?

If the list teardown loop runs first, any work item queued by a hardware event
after the loop but before cancellation could be leaked. Additionally, because
cancel_work_sync() blocks until running work finishes, placing it after the
list teardown means crw_work could run concurrently with the teardown and
access the list while it is being freed.

This is a pre-existing issue, but it appears the lack of locking when
manipulating private->crw in vfio_ccw_queue_crw() and
vfio_ccw_crw_region_read() could also lead to list corruption if executed
concurrently.

> +	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
vfio_ccw_mdev_release_dev() leak the eventfd contexts when tearing down the
device?

In vfio_ccw_mdev_set_irqs(), the driver takes references to eventfd_ctx
objects via eventfd_ctx_fdget(). However, these references do not appear to
be released via eventfd_ctx_put() during device cleanup here or in
vfio_ccw_mdev_close_device(). Could unprivileged userspace exploit this
resource leak by configuring IRQs and then closing the VFIO device fd?

[Severity: High]
This is a pre-existing issue, but can a data race occur on private->irb?

Hardware interrupts handled by fsm_irq() in vfio_ccw_fsm.c overwrite
private->irb while holding sch->lock. It then queues
vfio_ccw_sch_io_todo(), which reads private->irb locklessly before taking
io_mutex:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_io_todo() {
    ...
    irb = &private->irb;
    is_final = !(scsw_actl(&irb->scsw) & ...);
    ...
}

If a subsequent subchannel interrupt arrives asynchronously while
vfio_ccw_sch_io_todo() is reading the structure, might this expose torn or
corrupted I/O status to userspace?

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential
use-after-free when accessing the eventfd triggers?

When userspace disables an IRQ, vfio_ccw_mdev_set_irqs() frees the eventfd
context locklessly:

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

Concurrently, vfio_ccw_sch_io_todo() reads private->io_trigger locklessly
and passes it to eventfd_signal():

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

If the context is freed immediately after the workqueue reads the pointer,
will eventfd_signal() dereference freed memory?

[Severity: Critical]
This is a pre-existing issue, but does vfio_ccw_mdev_remove() race with
concurrent CSS event handlers?

vfio_ccw_mdev_remove() clears the drvdata and drops the VFIO device
reference, which frees the private structure:

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

Concurrently, vfio_ccw_chp_event() fetches private locklessly and
dereferences it:

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

If the removal completes before vfio_ccw_chp_event() accesses private,
could this result in a kernel crash due to accessing freed memory?

>  	kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
>  	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
>  	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);

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