Re: [PATCH v5 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] Lockless operations on the `private->crw` linked list can lead to list corruption, Use-After-Free, and double free.
- [High] Missing cancellation of background work items (`crw_work` and `io_work`) during device teardown allows them to execute on freed memory.
--

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

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

This commit clamps region indexes for read/write operations to be within
the bounds of private->num_regions. It also uses array_index_nospec() to
prevent speculative out-of-bounds access.

> 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 this locklessly dequeue elements from
private->crw and cause list corruption or use-after-free?

The userspace read syscall path calls vfio_ccw_crw_region_read(), which
dequeues elements from private->crw using list_del():

drivers/s390/cio/vfio_ccw_chp.c:vfio_ccw_crw_region_read() {
	...
	crw = list_first_entry_or_null(&private->crw, struct vfio_ccw_crw, next);
	if (crw)
		list_del(&crw->next);
	...
}

Concurrently, the asynchronous hardware event path locklessly appends elements
to private->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 an unprivileged guest with VFIO access trigger this race by executing
a read() syscall on the CRW region concurrently with a hardware asynchronous
channel path event to crash the host kernel?

>  
>  	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;

[ ... ]

> 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

[ ... ]

> @@ -338,11 +340,8 @@ static int vfio_ccw_mdev_ioctl_get_region_info(struct vfio_device *vdev,
>  		    VFIO_CCW_NUM_REGIONS + private->num_regions)
>  			return -EINVAL;
>  
> -		info->index = array_index_nospec(info->index,
> -						 VFIO_CCW_NUM_REGIONS +
> -						 private->num_regions);
> -
>  		i = info->index - VFIO_CCW_NUM_REGIONS;
> +		i = array_index_nospec(i, private->num_regions);

[Severity: High]
This is a pre-existing issue, but does device teardown safely cancel background
work items before freeing memory?

Asynchronous events can queue crw_work (via vfio_ccw_chp_event) or io_work
(via fsm_irq). When the device is torn down, fsm_close() conditionally flushes
the workqueue only on an error/busy path, leaving pending work active on the
success path:

drivers/s390/cio/vfio_ccw_fsm.c:fsm_close() {
	...
	ret = cio_disable_subchannel(sch);
	if (ret == -EBUSY)
		ret = vfio_ccw_sch_quiesce(sch);
	...
}

Then, vfio_ccw_mdev_release_dev() frees the private structure and destroys the
mutex without invoking cancel_work_sync() for crw_work and io_work:

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

Can this lead to a use-after-free on the vfio_ccw_private structure if the
pending work executes on the freed private pointer after it is destroyed by
the VFIO core?

>  
>  		info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index);
>  		info->size = private->region[i].size;

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