Re: [PATCH v7 10/10] s390/vfio_ccw: implement a crw lock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent VFIO_DEVICE_SET_IRQS ioctl can cause a Use-After-Free of eventfd_ctx in vfio_ccw_crw_region_read() and vfio_ccw_crw_todo().
- [High] Race condition between asynchronous channel path events and device removal leads to Use-After-Free of the `private` struct.
- [High] The `crw` list is emptied before `cancel_work_sync()` is called, which can lead to memory leaks of dynamically allocated `crw` objects.
--

--- Patch [10]: [PATCH v7 10/10] s390/vfio_ccw: implement a crw lock ---
commit 32947477038816f52fb49c7c2eb829ec1255cb5d
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.

Fixes: 3f02cb2fd9d2 ("vfio-ccw: Wire up the CRW irq and CRW region")
Cc: stable@vger.kernel.org
Cc: Farhan Ali <alifm@linux.ibm.com>

> 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;
>  
>  	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 can a concurrent
VFIO_DEVICE_SET_IRQS ioctl cause a use-after-free of the eventfd_ctx?

If an unprivileged userspace process calls the VFIO_DEVICE_SET_IRQS ioctl,
vfio_ccw_mdev_set_irqs() releases the eventfd_ctx without acquiring crw_lock
or other serializing locks:

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

Since private->crw_trigger is read under crw_lock here, but not held by
the writer, could the context be freed before or during eventfd_signal()?

> @@ -286,6 +289,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
> @@ -303,7 +307,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);
>  	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 is there a race condition between asynchronous
channel path events and device removal that could lead to a use-after-free of
the private struct?

A hardware event can trigger vfio_ccw_chp_event() which fetches the private
pointer locklessly:

	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
	...
	vfio_ccw_queue_crw(private, CRW_RSC_CPATH, ...);

Concurrently, if userspace writes to the mdev 'remove' sysfs attribute,
vfio_ccw_mdev_remove() clears the driver data and releases the device reference:

	dev_set_drvdata(&parent->dev, NULL);
	vfio_put_device(&private->vdev);

Could the private structure be freed while we are still executing here or in
queue_work()?

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 8ec6b175d9914..9cbb277b83fb2 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c

[ ... ]

> @@ -131,11 +132,14 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  	struct vfio_ccw_private *private =
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  	struct vfio_ccw_crw *crw, *temp;
> +	unsigned long flags;
>  
> +	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);
>  
>  	/* Should be empty, but just in case */
>  	cancel_work_sync(&private->io_work);

[Severity: High]
This isn't a bug introduced by this patch, but does emptying the crw list before
calling cancel_work_sync() violate the subsystem rule that background tasks must
be explicitly stopped before the queues they operate on are destroyed?

If a concurrent hardware event (like vfio_ccw_chp_event) allocates a crw and
calls queue_work() before or during the cancellation, could the newly allocated
crw be left on the list and leaked?

Additionally, if queue_work() executes after cancel_work_sync() finishes, could
the work execute on a freed private pointer?

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