Re: [GIT PULL v1 10/23] s390/vfio_ccw: Implement a crw lock

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

Pre-existing issues:
- [High] Missing synchronization when updating eventfd triggers leads to a =
potential Use-After-Free of the `eventfd_ctx` during `eventfd_signal()`.
- [High] Concurrent execution of asynchronous CSS hardware callbacks and md=
ev removal allows a Use-After-Free of the `vfio_ccw_private` structure.
- [Medium] The `eventfd_ctx` for device triggers is leaked when the device =
file descriptor is closed without explicitly disabling the IRQs.
--

--- Patch [10]: [GIT PULL v1 10/23] s390/vfio_ccw: Implement a crw lock ---
commit e6c7aa37724a25bba44b60d5d9939f115e04276a
Author: Eric Farman <[email protected]>

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.

Cc: [email protected]

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_=
drv.c
> index 757ff5b2556e3..ab6b518cc353f 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -118,11 +118,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;
> =20
>  	private =3D container_of(work, struct vfio_ccw_private, crw_work);
> =20
> +	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 is a pre-existing issue, but does this adequately protect against
concurrent modification of the eventfd context?

While this patch adds crw_lock around the read and signal path here in
vfio_ccw_crw_todo(), is the writer side in vfio_ccw_mdev_set_irqs()
also protected?

If vfio_ccw_mdev_set_irqs() replaces or unsets the eventfd without locks:

vfio_ccw_mdev_set_irqs()
    if (*ctx)
        eventfd_ctx_put(*ctx);
    *ctx =3D efdctx;
   =20
Could a concurrent execution of vfio_ccw_crw_todo() read the pointer,
have the context freed by vfio_ccw_mdev_set_irqs(), and then use it in
eventfd_signal(), resulting in a use-after-free?

[ ... ]

> @@ -286,6 +289,7 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_privat=
e *private,
>  			       unsigned int rsid)
>  {
>  	struct vfio_ccw_crw *crw;
> +	unsigned long flags;
> =20
>  	/*
>  	 * If unable to allocate a CRW, just drop the event and
>  	 * carry on.  The guest will either see a later one or
>  	 * learn when it issues its own store subchannel.
>  	 */
>  	crw =3D kzalloc_obj(*crw, GFP_ATOMIC);
>  	if (!crw)
>  		return;
> =20
>  	/*
>  	 * Build the CRW based on the inputs given to us.
>  	 */
>  	crw->crw.rsc =3D rsc;
>  	crw->crw.erc =3D erc;
>  	crw->crw.rsid =3D rsid;
> =20
> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	list_add_tail(&crw->next, &private->crw);
> +	spin_unlock_irqrestore(&private->crw_lock, flags);
>  	queue_work(vfio_ccw_work_q, &private->crw_work);
>  }

[Severity: High]
This is a pre-existing issue, but could vfio_ccw_queue_crw() dereference
a freed private pointer during device removal?

If an asynchronous CSS hardware callback like vfio_ccw_chp_event() fetches
the private struct locklessly, and concurrently the mdev is removed via
sysfs:

vfio_ccw_mdev_remove()
    dev_set_drvdata(&parent->dev, NULL);
    vfio_put_device(&private->vdev);
   =20
Since there is no synchronization stopping the CSS callback, could the
private structure be freed before vfio_ccw_chp_event() calls
vfio_ccw_queue_crw(), leading to a use-after-free when acquiring the
crw_lock here?

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_=
ops.c
> index 1df6d649565b8..5ce91285c7d52 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -131,6 +132,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_dev=
ice *vdev)
>  	struct vfio_ccw_private *private =3D
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  	struct vfio_ccw_crw *crw, *temp;
> +	unsigned long flags;
> =20
>  	/*
>  	 * Ensure these work items are fully drained, so none can
> @@ -146,10 +148,12 @@ static void vfio_ccw_mdev_release_dev(struct vfio_d=
evice *vdev)
>  	cancel_work_sync(&private->crw_work);
>  	flush_work(&private->notoper_work);
> =20
> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	list_for_each_entry_safe(crw, temp, &private->crw, next) {
>  		list_del(&crw->next);
>  		kfree(crw);
>  	}
> +	spin_unlock_irqrestore(&private->crw_lock, flags);
> =20
>  	kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
>  	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);

[Severity: Medium]
This isn't a bug introduced by this patch, but does closing the device leak
the eventfd contexts allocated for device triggers?

When userspace configures IRQs via VFIO_DEVICE_SET_IRQS, eventfd_ctx_fdget()
increments the reference count of the eventfd context.

If userspace closes the device without explicitly disabling the IRQs first,
vfio_ccw_mdev_release_dev() is invoked. Is it necessary to call
eventfd_ctx_put() here to release the references to private->io_trigger,
private->crw_trigger, and private->req_trigger?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803160924.2368=
[email protected]?part=3D10