Re: [PATCH v9 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:
- [Critical] Race condition causing linked list corruption on `private->crw`.
- [Critical] Missing background work cancellation leading to Use-After-Free.
--

--- Patch [6]: [PATCH v9 06/10] s390/vfio_ccw: ensure index for read/write regions are within range ---
commit a96682323321d1f6bd6354e74ae1c8c0bbb835a1
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: Critical]
This is a pre-existing issue, but is it safe to dequeue from private->crw
locklessly here?

In drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_queue_crw(), the hardware event
path appends to this list without locks:

    list_add_tail(&crw->next, &private->crw);

Could a concurrent userspace read on the CRW region race with a hardware
event and corrupt the linked list pointers?

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


[Severity: Critical]
This is another pre-existing issue, but does vfio_ccw_mdev_release_dev()
need to cancel the background work queues during teardown?

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

During device release, if fsm_close() disables the subchannel immediately
and skips vfio_ccw_sch_quiesce(), could private->crw_work and
private->io_work still be queued?

Since vfio_ccw_mdev_release_dev() lacks cancel_work_sync() calls, would any
pending work items execute after the private struct has been freed by the
VFIO core, leading to a use-after-free?

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