Re: [PATCH v1 5/6] 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 1 potential issue(s) to consider:
- [High] Flawed usage of `array_index_nospec()` incorrectly clamps out-of-bounds indices to 0 before a subtraction, leading to a speculative integer underflow and out-of-bounds array access.
--

--- Patch [5]: [PATCH v1 5/6] s390/vfio_ccw: ensure index for read/write regions are within range ---
commit f00821fcd31baacca490cf51ca9a9206a56fdf45
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 clamped the region indexes to the
range of the capabilities itself, but neglected to do so for the existing
read/write regions. This commit enforces the same checks for those regions.

> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -245,6 +245,8 @@ static ssize_t vfio_ccw_mdev_read(struct vfio_device *vdev,
>  	if (index >= VFIO_CCW_NUM_REGIONS + private->num_regions)
>  		return -EINVAL;
>  
> +	index = array_index_nospec(index, VFIO_CCW_NUM_REGIONS + private->num_regions);
> +
>  	switch (index) {
>  	case VFIO_CCW_CONFIG_REGION_INDEX:
>  		return vfio_ccw_mdev_read_io_region(private, buf, count, ppos);
>  	default:
>  		index -= VFIO_CCW_NUM_REGIONS;
>  		return private->region[index].ops->read(private, buf, count,
>  							ppos);

[Severity: High]
Does this introduce a speculative out-of-bounds access regression?

If an attacker provides an out-of-bounds index, the architectural bounds
check is bypassed during speculative execution. The array_index_nospec()
mitigation will clamp the out-of-bounds index to 0 to resolve data
dependencies safely.

However, if the CPU mispredicts the switch statement and executes the
default case, the clamped index of 0 is subtracted by
VFIO_CCW_NUM_REGIONS. This causes an integer underflow, leading to a
speculative read of private->region with a negative index, followed by
a speculative indirect call via ops->read().

Could we perform the array_index_nospec() masking after the subtraction,
specifically clamping against private->num_regions instead?

[ ... ]

> @@ -297,6 +299,8 @@ static ssize_t vfio_ccw_mdev_write(struct vfio_device *vdev,
>  	if (index >= VFIO_CCW_NUM_REGIONS + private->num_regions)
>  		return -EINVAL;
>  
> +	index = array_index_nospec(index, VFIO_CCW_NUM_REGIONS + private->num_regions);
> +
>  	switch (index) {
>  	case VFIO_CCW_CONFIG_REGION_INDEX:
>  		return vfio_ccw_mdev_write_io_region(private, buf, count, ppos);
>  	default:
>  		index -= VFIO_CCW_NUM_REGIONS;
>  		return private->region[index].ops->write(private, buf, count,
>  							 ppos);

[Severity: High]
Will this suffer from the same speculative integer underflow as the read
path?

If array_index_nospec() clamps an out-of-bounds index to 0, the
subsequent subtraction will result in an underflow, allowing a
speculative out-of-bounds read and an indirect call through ops->write().

Additionally, it looks like this identical anti-speculation pattern is
already present in other locations such as
vfio_ccw_mdev_ioctl_get_region_info(),
intel_vgpu_ioctl_get_region_info(), and vfio_pci_ioctl_get_region_info().

Do those locations also need to be updated?

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