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

[Eric Farman <farman@linux.ibm.com>]

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@vger.kernel.org
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   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;
>