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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A race condition between asynchronous channel path events (vfio_ccw_chp_event) and device removal (vfio_ccw_mdev_remove) can cause a Use-After-Free of the private structure.
- [High] Unbounded growth of the CRW queue leads to kernel memory leaks and potential OOM.
- [High] Unsynchronized replacement and freeing of eventfd triggers in vfio_ccw_mdev_set_irqs races with their usage, leading to a Use-After-Free.
- [High] Incorrect teardown sequence in vfio_ccw_mdev_release_dev frees list elements before cancelling asynchronous work, causing a memory leak or UAF.
- [Medium] A 0-byte read operation on the CRW region inappropriately consumes and drops a CRW event.
--

--- Patch [10]: [PATCH v6 10/10] s390/vfio_ccw: implement a crw lock ---
commit a24849c415dabc5f49c5f9509d897bea1c9a72dc
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_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index f3015132d4b5f..7708eb4d6de0b 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
[ ... ]
> @@ -113,6 +108,16 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  	i = array_index_nospec(i, private->num_regions);
>  	region = private->region[i].data;
>  
> +	spin_lock_irqsave(&private->crw_lock, flags);
> +	crw = list_first_entry_or_null(&private->crw,
> +				       struct vfio_ccw_crw, next);
> +
> +	if (crw)
> +		list_del(&crw->next);
> +
> +	/* Drop CRW lock while copying to userspace */
> +	spin_unlock_irqrestore(&private->crw_lock, flags);
> +

[Severity: Medium]
This is a pre-existing issue, but does a 0-byte read operation on the CRW
region inappropriately consume and drop a CRW event?

If userspace performs a read operation on the CRW region with count == 0, the
driver proceeds to dequeue an event from private->crw here, performs a 0-byte
copy_to_user(), and then frees the CRW object. The event would be lost forever
without being delivered.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index faf2de074a10f..d7e93b9d491ca 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 is a pre-existing issue, but is there an unsynchronized replacement and
freeing of eventfd triggers in vfio_ccw_mdev_set_irqs() that races with their
usage here?

Userspace can call the VFIO_DEVICE_SET_IRQS ioctl to change or clear eventfds.
vfio_ccw_mdev_set_irqs() handles this and calls eventfd_ctx_put() without
holding any locks to serialize it against asynchronous readers like
vfio_ccw_crw_todo().

If a reader holds a pointer to private->crw_trigger while the ioctl drops the
reference, could the reader call eventfd_signal() on a freed object?

> @@ -296,7 +300,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 could unbounded growth of the CRW queue
lead to kernel memory leaks and potential OOM?

vfio_ccw_queue_crw() unconditionally allocates new struct vfio_ccw_crw objects
using kzalloc_obj(GFP_ATOMIC) and appends them to private->crw. Without bounds
checking or a limit on the number of elements, a stream of events could cause
the queue to grow infinitely, consuming un-reclaimable kernel memory.

[Severity: High]
This is a pre-existing issue, but is there a race condition between
asynchronous channel path events and device removal?

In vfio_ccw_chp_event(), the private structure is fetched using
dev_get_drvdata() without acquiring a reference count:

vfio_ccw_chp_event()
    struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);

Concurrently, if a user unbinds the device, vfio_ccw_mdev_remove() executes,
clearing the drvdata and dropping the final reference via vfio_put_device(),
which frees the private structure.

Could vfio_ccw_chp_event() then call vfio_ccw_queue_crw(), dereferencing the
freed pointer and causing a Use-After-Free?

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index ddfcd2b4296f1..2f28846fdf724 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
[ ... ]
> @@ -137,11 +138,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 is a pre-existing issue, but does this teardown sequence free list
elements before cancelling asynchronous work?

By iterating through private->crw and freeing all elements before calling
cancel_work_sync(&private->crw_work), the lifecycle ordering violates the
principle of stopping asynchronous work before freeing the resources it
operates on.

If an event concurrently calls vfio_ccw_queue_crw() and inserts an element
after the list is emptied but before cancel_work_sync(), could the element
be leaked?

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