Re: [PATCH v2 7/7] 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 workqueue cancellation during device teardown leading to Use-After-Free.
- [High] Data race and Use-After-Free on `eventfd_ctx` via lockless modification of `crw_trigger`.
--

--- Patch [7]: [PATCH v2 7/7] s390/vfio_ccw: implement a crw lock ---
commit a667ac37b9e546d1f3d6cceb44b21f4a47909da6
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.

Implement 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 bfb68de6e52ca..12edccb62cbf5 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 is a pre-existing issue, but could this access to private->crw_trigger
cause a use-after-free of the eventfd context?

Looking at vfio_ccw_mdev_set_irqs() in drivers/s390/cio/vfio_ccw_ops.c, it
replaces private->crw_trigger and frees the old context with
eventfd_ctx_put() without holding crw_lock or any other synchronizing lock:

vfio_ccw_mdev_set_irqs()
            if (*ctx)
                eventfd_ctx_put(*ctx);
            *ctx = efdctx;

If userspace calls the VFIO_DEVICE_SET_IRQS ioctl concurrently with
vfio_ccw_crw_todo(), could the eventfd context be freed before
eventfd_signal() is executed here?

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index d2475b3158cf8..b3699ffd2f1d7 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c

[ ... ]

> @@ -137,10 +138,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);

[Severity: High]
This isn't a bug introduced by this patch, but does this function need to
call cancel_work_sync() for crw_work and io_work before the device is
released?

Since private->crw_work is dynamically queued onto vfio_ccw_work_q during
asynchronous hardware events, it might be pending or executing when the
device is removed.

If vfio_ccw_mdev_release_dev() runs and the private structure is subsequently
freed by the VFIO core without explicitly canceling the work, could it
result in a use-after-free when vfio_ccw_crw_todo() dereferences the freed
private pointer?

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