Re: [PATCH v6 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:
- [High] Data Race / List Corruption in private->crw
- [High] Use-After-Free due to missing cancel_work_sync in release handler
- [Low] Event Loss on Failed copy_to_user
--

--- Patch [6]: [PATCH v6 06/10] s390/vfio_ccw: ensure index for read/write regions are within range ---
commit f91d3ca5d2a5910d7f313cb125ab777b26c1f34e
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. This commit
ensures read/write region indexes are within range and clamped via
array_index_nospec.

> 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 can this list modification race with
concurrent additions?

Looking at vfio_ccw_crw_region_read() just above this diff, the crw list is
popped and modified without holding any locks:

	crw = list_first_entry_or_null(&private->crw, struct vfio_ccw_crw, next);
	if (crw)
		list_del(&crw->next);

Could this race with vfio_ccw_queue_crw(), which adds to the list using
list_add_tail(&crw->next, &private->crw)? If a hardware event asynchronously
occurs while a guest reads the CRW region, could this result in list
corruption?

> +	if (i >= private->num_regions) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	i = array_index_nospec(i, private->num_regions);
>  	region = private->region[i].data;
>  
>  	if (crw)
> @@ -109,6 +123,7 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  
>  	region->crw = 0;
>  
> +out:
>  	mutex_unlock(&private->io_mutex);
>  
>  	kfree(crw);

[Severity: Low]
This is a pre-existing issue, but does this unconditionally free the CRW
event even if the copy to userspace failed?

Looking at the end of vfio_ccw_crw_region_read():

	if (copy_to_user(buf, (void *)region + pos, count))
		ret = -EFAULT;
	else
		ret = count;

	region->crw = 0;

out:
	mutex_unlock(&private->io_mutex);

	kfree(crw);

If userspace provides an invalid buffer, copy_to_user() will fail and return
-EFAULT, but the event is freed rather than requeued. Can this lead to a
permanently lost Channel Report Word event?

> 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;
> +		index = array_index_nospec(index, private->num_regions);
>  		return private->region[index].ops->read(private, buf, count,
>  							ppos);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but looking at the lifetime
management in this file, is there a potential use-after-free during
device removal?

In vfio_ccw_mdev_release_dev(), the private struct and its embedded work items
are freed without calling cancel_work_sync():

	kmem_cache_free(vfio_ccw_io_region, private->io_region);
	kfree(private->cp.guest_cp);
	mutex_destroy(&private->io_mutex);

If an mdev is created but never opened by userspace, channel path events can
still trigger vfio_ccw_chp_event(), queuing private->crw_work. If the mdev is
subsequently removed without being opened, the flush_workqueue() in the close
handler is bypassed. Could a background work item then execute using the
freed vfio_ccw_private struct?

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