Re: [PATCH v8 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent access to the `private->crw` list without locking can lead to double-free and list corruption.
- [High] `private->io_work` and `private->crw_work` are not explicitly canceled before the `private` structure is freed, leading to a Use-After-Free.
--

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

s390/vfio_ccw: ensure index for read/write regions are within range
    
The introduction of the capability chain rightly clamped the
region indexes to the range of the capabilities itself, but
neglected to do so for the existing read/write regions which
should also be 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);

[Severity: High]
This is a pre-existing issue, but does concurrent access to the private->crw
list without locking lead to double-free and list corruption here?

The private->crw list is modified by list_add_tail() in vfio_ccw_queue_crw()
(invoked by channel path hardware event contexts):

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);
	...
}

If a hardware event fires while userspace is concurrently removing an element
locklessly via list_del() in vfio_ccw_crw_region_read(), will the overlapping
list modifications corrupt the list?

>  
>  	mutex_lock(&private->io_mutex);
> +	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

[Severity: High]
This is a pre-existing issue, but are private->io_work and private->crw_work
explicitly canceled before the private structure is freed, or can this lead
to a use-after-free?

During device teardown, vfio_ccw_mdev_release_dev() frees the private structure
without explicitly canceling io_work and crw_work using cancel_work_sync():

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_release_dev() {
	struct vfio_ccw_private *private =
		container_of(vdev, struct vfio_ccw_private, vdev);
	...
	mutex_destroy(&private->io_mutex);
}

Although vfio_ccw_sch_quiesce() calls flush_workqueue(), it does so before
calling cio_disable_subchannel(). If an interrupt arrives after the flush but
before the hardware is disabled, it could re-queue the work. When private is
later freed, will the pending work execute on freed memory?

> @@ -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;
> +		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/20260728013509.1551753-1-farman@linux.ibm.com?part=6