Re: [PATCH v5 10/10] 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 write-side lock in `vfio_ccw_mdev_set_irqs` renders `crw_lock` ineffective, leading to Use-After-Free of `eventfd_ctx`.
- [High] TOCTOU race between `vfio_ccw_mdev_remove` and asynchronous channel path events leads to Use-After-Free of `vfio_ccw_private`.
--

--- Patch [10]: [PATCH v5 10/10] s390/vfio_ccw: implement a crw lock ---
commit b1ac07dbd7bc9e331720d0ba13ad54e4a3ae455a
Author: Eric Farman <farman@linux.ibm.com>
s390/vfio_ccw: implement a crw lock

This commit introduces a new crw_lock to serialize access to the Channel
Report Word (CRW) list elements in the vfio_ccw driver. It replaces implicit
synchronization with explicit spinlocks when queueing, reading, and freeing
CRW structures, as well as when notifying userspace via eventfd.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 4830f0dd9c3a3..437d7709b4a56 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -122,11 +122,14 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  void vfio_ccw_crw_todo(struct work_struct *work)
>  {
>  	struct vfio_ccw_private *private;
> +	unsigned long flags;
>  
>  	private = container_of(work, struct vfio_ccw_private, crw_work);
>  
> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	if (!list_empty(&private->crw) && private->crw_trigger)
>  		eventfd_signal(private->crw_trigger);
> +	spin_unlock_irqrestore(&private->crw_lock, flags);

[Severity: High]
This isn't a bug introduced by this patch, but does crw_lock fully protect
crw_trigger from concurrent modification?

In vfio_ccw_mdev_set_irqs(), the eventfd context can be updated and the old
context freed without acquiring crw_lock:

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

Because the write side in vfio_ccw_mdev_set_irqs() doesn't take the lock,
could a concurrent VFIO_DEVICE_SET_IRQS ioctl free the context while
eventfd_signal() is executing here, leading to a Use-After-Free?

[ ... ]
> @@ -290,6 +293,7 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
>  			       unsigned int rsid)
>  {
>  	struct vfio_ccw_crw *crw;
> +	unsigned long flags;
>  
>  	/*
>  	 * If unable to allocate a CRW, just drop the event and
> @@ -307,7 +311,9 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
>  	crw->crw.erc = erc;
>  	crw->crw.rsid = rsid;
>  
> +	spin_lock_irqsave(&private->crw_lock, flags);

[Severity: High]
This is a pre-existing issue, but could this lock access trigger a
Use-After-Free if the mdev is being concurrently removed?

The CSS core calls vfio_ccw_chp_event() asynchronously. It retrieves the
private pointer locklessly and then calls this function:

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

If vfio_ccw_mdev_remove() runs concurrently, it clears the driver data and
drops the final reference:

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

If the channel path event handler passes the NULL check and gets preempted
before mdev removal finishes, won't taking private->crw_lock here access
freed memory?

>  	list_add_tail(&crw->next, &private->crw);
> +	spin_unlock_irqrestore(&private->crw_lock, flags);
>  	queue_work(vfio_ccw_work_q, &private->crw_work);
>  }

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