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

[Eric Farman]

On 7/24/26 3:01 PM, Matthew Rosato wrote:
> On 7/23/26 1:47 PM, 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 | 4 ++++
> >   drivers/s390/cio/vfio_ccw_chp.c   | 4 ++++
> >   drivers/s390/cio/vfio_ccw_ops.c   | 7 +++----
> >   3 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/s390/cio/vfio_ccw_async.c b/drivers/s390/cio/vfio_ccw_async.c
> > index 420d89ba7f83..f213f62f6641 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,6 +25,8 @@ static ssize_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,
> >   		return -EINVAL;
> >   
> >   	mutex_lock(&private->io_mutex);
> > +
> > +	i = array_index_nospec(i, private->num_regions);
> >   	region = private->region[i].data;
> >   	if (copy_to_user(buf, (void *)region + pos, count))
>
> Couldn't using array_index_nospec cause us to accidentally operate on
> region[0] if ppos were ever out of range?

Hrm, yeah as I keep pondering this I think you're right. The parent 
routines (vfio_ccw_mdev_read(), for example) checks that it's not above 
num_regions + NUM_REGIONS, but there's no check against the subset in 
any of these. I'll add them; thanks.

> By comparison, vfio_pci_ioctl_get_region_info() does an explicit check
> first, so to do something similar here and elsewhere in this patch:
>
> if (i >= private->num_regions)
>     return -EINVAL;
> i = array_index_nospec(i, private->num_regions);
>
> (Note that for this case and vfio_ccw_async_region_write my pseudocode
> doesn't account for io_mutex)
>
> >   		ret = -EFAULT;
> > @@ -48,6 +51,7 @@ static ssize_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
> >   	if (!mutex_trylock(&private->io_mutex))
> >   		return -EAGAIN;
> >   
> > +	i = array_index_nospec(i, private->num_regions);
> >   	region = private->region[i].data;
> >   	if (copy_from_user((void *)region + pos, buf, count)) {
> >   		ret = -EFAULT;
> > diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> > index 38c176cf6295..bf678270c457 100644
> > --- a/drivers/s390/cio/vfio_ccw_chp.c
> > +++ b/drivers/s390/cio/vfio_ccw_chp.c
> > @@ -9,6 +9,7 @@
> >    */
> >   
> >   #include <linux/slab.h>
> > +#include <linux/nospec.h>
> >   #include <linux/vfio.h>
> >   #include "vfio_ccw_private.h"
> >   
> > @@ -26,6 +27,8 @@ static ssize_t vfio_ccw_schib_region_read(struct vfio_ccw_private *private,
> >   		return -EINVAL;
> >   
> >   	mutex_lock(&private->io_mutex);
> > +
> > +	i = array_index_nospec(i, private->num_regions);
> >   	region = private->region[i].data;
> >   
> >   	if (cio_update_schib(sch)) {
> > @@ -97,6 +100,7 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
> >   		list_del(&crw->next);
> >   
> >   	mutex_lock(&private->io_mutex);
> > +	i = array_index_nospec(i, private->num_regions);
> >   	region = private->region[i].data;
> >   
> >   	if (crw)
> > 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);
> >   
> >   		info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index);
> >   		info->size = private->region[i].size;