Re: [PATCH v4 6/9] s390/vfio_ccw: ensure index for read/write regions are within range

[Matthew Rosato]

On 7/25/26 11:27 AM, Eric Farman wrote:
> 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.
> 
> Fixes: db8e5d17ac03 ("vfio-ccw: add capabilities chain")
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Cornelia Huck <cohuck@xxxxxxxxxx>
> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
> ---
>  drivers/s390/cio/vfio_ccw_async.c | 16 ++++++++++++++++
>  drivers/s390/cio/vfio_ccw_chp.c   | 15 +++++++++++++++
>  drivers/s390/cio/vfio_ccw_ops.c   |  7 +++----
>  3 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_async.c b/drivers/s390/cio/vfio_ccw_async.c
> index 420d89ba7f83..4aff0b58fa5d 100644
> --- a/drivers/s390/cio/vfio_ccw_async.c
> +++ b/drivers/s390/cio/vfio_ccw_async.c
> @@ -8,6 +8,7 @@
>   */
>  
>  #include <linux/vfio.h>
> +#include <linux/nospec.h>
>  
>  #include "vfio_ccw_private.h"
>  
> @@ -24,11 +25,20 @@ static ssize_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,
>  		return -EINVAL;
>  
>  	mutex_lock(&private->io_mutex);
> +
> +	if (i >= private->num_regions) {
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	i = array_index_nospec(i, private->num_regions);

LGTM.

...

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 45ec722d25ea..032a1cdf4df7 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);
>  	}
> @@ -295,6 +296,7 @@ static ssize_t vfio_ccw_mdev_write(struct vfio_device *vdev,
>  		return vfio_ccw_mdev_write_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->write(private, buf, count,
>  							 ppos);
>  	}
> @@ -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);

...  These didn't at first, but then I realized that these functions in
vfio_ccw_ops.c each had pre-existing checks of index against
VFIO_CCW_NUM_REGIONS prior to this point.

Reviewed-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>