Re: [PATCH v4 9/9] s390/vfio_ccw: implement a crw lock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing cancellation of asynchronous work items (`crw_work`, `io_work`, `notoper_work`) during device release leads to a Use-After-Free of the `vfio_ccw_private` structure.
- [High] Unsynchronized modification of `crw_trigger` in the `SET_IRQS` ioctl causes a Time-of-Check to Time-of-Use (TOCTOU) race and Use-After-Free during eventfd signaling.
--

--- Patch [9]: [PATCH v4 9/9] s390/vfio_ccw: implement a crw lock ---
commit 5187e6c3172f066c510e9c2798199ac3a8915b42
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: implement a crw lock

Unlike the channel_program struct, which covers synchronous I/O
submissions and asynchronous interrupts, the CRW region relies
exclusively on asynchronous events coming from hardware. This patch
implements a lock to manage the list of those payloads to ensure
they are read cohesively.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 4830f0dd9c3a3..32f631d086205 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -125,8 +125,10 @@ void vfio_ccw_crw_todo(struct work_struct *work)
>  
>  	private = container_of(work, struct vfio_ccw_private, crw_work);
>  
> +	spin_lock(&private->crw_lock);
>  	if (!list_empty(&private->crw) && private->crw_trigger)
>  		eventfd_signal(private->crw_trigger);
> +	spin_unlock(&private->crw_lock);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does checking
private->crw_trigger under crw_lock fully protect against a Use-After-Free
during eventfd signaling?

If a guest userspace process concurrently calls the VFIO_DEVICE_SET_IRQS ioctl
to change or unset the crw_trigger while crw_work is running, the writer path
modifies private->crw_trigger locklessly:

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

If the ioctl thread frees the context right after vfio_ccw_crw_todo() reads it
but before calling eventfd_signal(), could the worker thread dereference a
freed pointer?

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 9242a37677a06..6735b5f4c07a2 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
[ ... ]
> @@ -139,10 +140,12 @@ 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;
>  
> +	spin_lock(&private->crw_lock);
>  	list_for_each_entry_safe(crw, temp, &private->crw, next) {
>  		list_del(&crw->next);
>  		kfree(crw);
>  	}
> +	spin_unlock(&private->crw_lock);
>  
>  	kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
>  	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);

[Severity: High]
This is a pre-existing issue, but does the device release function need to
cancel asynchronous work items before freeing the private structure?

If the administrator removes the mdev while an asynchronous hardware event is
queued, vfio_ccw_mdev_release_dev() will free private without canceling the
work items, such as crw_work, io_work, or notoper_work.

Could the queued work run and access the freed private pointer, like in
vfio_ccw_crw_todo():

    private = container_of(work, struct vfio_ccw_private, crw_work);
    
    spin_lock(&private->crw_lock);
    if (!list_empty(&private->crw) && private->crw_trigger)
        eventfd_signal(private->crw_trigger);

causing a Use-After-Free?

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