Re: [PATCH v7 06/10] s390/vfio_ccw: ensure index for read/write regions are within range

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Concurrent lockless modification and usage of `private->*_trigger` eventfd pointers can lead to use-after-free.
- [High] Concurrent lockless accesses and modifications to the `private->crw` list lead to list corruption, infinite loops, and potential use-after-free.
- [High] Work items `io_work` and `crw_work` are not unconditionally cancelled before freeing the device structure.
--

--- Patch [6]: [PATCH v7 06/10] s390/vfio_ccw: ensure index for read/write regions are within range ---
commit f8eafc3dc61a7fdadf00091fc10c59686336cf60
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: ensure index for read/write regions are within range

This patch adds bounds checking to the region indexes in the read/write paths.
The capability chain previously clamped the region indexes, but the existing
read/write regions lacked these checks, which are now enforced.

> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 38c176cf62957..f3015132d4b5f 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
[ ... ]
> @@ -97,6 +105,12 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  		list_del(&crw->next);
>  
>  	mutex_lock(&private->io_mutex);

[Severity: High]
This is a pre-existing issue, but is the private->crw list modified locklessly
here?

If an asynchronous host event handler queues a CRW using list_add_tail():

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_queue_crw() {
	...
	list_add_tail(&crw->next, &private->crw);
	queue_work(vfio_ccw_work_q, &private->crw_work);
	...
}

Could a concurrent read from userspace calling vfio_ccw_crw_region_read()
corrupt the list pointers when calling list_del() since neither operation
appears to hold a lock protecting private->crw?

> +	if (i >= private->num_regions) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	i = array_index_nospec(i, private->num_regions);
>  	region = private->region[i].data;
>  
>  	if (crw)

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 45ec722d25ea6..032a1cdf4df70 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
[ ... ]
> @@ -243,6 +243,7 @@ static ssize_t vfio_ccw_mdev_read(struct vfio_device *vdev,
>  		return vfio_ccw_mdev_read_io_region(private, buf, count, ppos);
>  	default:
>  		index -= VFIO_CCW_NUM_REGIONS;

[Severity: High]
This is a pre-existing issue, but are io_work and crw_work unconditionally
cancelled before freeing the device structure?

Looking at vfio_ccw_mdev_release_dev(), the private structure is freed:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_release_dev() {
	...
	kfree(private->cp.guest_cp);
	mutex_destroy(&private->io_mutex);
}

If a background event queues io_work or crw_work, and the work item is not
flushed before release, could vfio_ccw_crw_todo() dereference the freed
private pointer when accessing private->crw_trigger?

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_crw_todo() {
	private = container_of(work, struct vfio_ccw_private, crw_work);

	if (!list_empty(&private->crw) && private->crw_trigger)
	...
}

[Severity: Critical]
This is another pre-existing issue, but can concurrent modification and usage
of the private->*_trigger eventfd pointers lead to a use-after-free?

The VFIO_DEVICE_SET_IRQS ioctl can locklessly drop the reference and clear the
eventfd pointer:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_set_irqs() {
	...
		if (fd == -1) {
			if (*ctx)
				eventfd_ctx_put(*ctx);
			*ctx = NULL;
	...
}

If background work items like vfio_ccw_crw_todo() check the pointer and then
call eventfd_signal():

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_crw_todo() {
	...
	if (!list_empty(&private->crw) && private->crw_trigger)
		eventfd_signal(private->crw_trigger);
	...
}

Could the eventfd context be freed between the check and the signal call,
resulting in a dereference of the freed context?

> +		index = array_index_nospec(index, private->num_regions);
>  		return private->region[index].ops->read(private, buf, count,
>  							ppos);
>  	}

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