Re: [PATCH v2 5/7] 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete Spectre v1 Mitigation / Speculative Out-of-Bounds Access
- [High] Broken Spectre v1 Mitigation / Speculative Out-of-Bounds Read in ioctl handler
--

--- Patch [5]: [PATCH v2 5/7] s390/vfio_ccw: ensure index for read/write regions are within range ---
commit 9523937d39e77d5858c1e23dd917c8844bcf3876
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.

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 45ec722d25ea6..e695cd985369d 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 does this actually fix the
speculative out-of-bounds access?

We clamp the local index here, but we pass the unmodified ppos pointer
down to the read handlers. The handlers then recompute the index from ppos
without array_index_nospec():

drivers/s390/cio/vfio_ccw_async.c:vfio_ccw_async_region_read() {
    unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
    ...
    region = private->region[i].data;
    if (copy_to_user(buf, (void *)region + pos, count))
}

Can a malicious guest exploit this by passing an out-of-bounds offset,
causing the read handler to speculatively recompute the out-of-bounds index
i and perform a speculative out-of-bounds read via private->region[i].data?

The same pattern exists in vfio_ccw_async_region_write(),
vfio_ccw_schib_region_read(), and vfio_ccw_crw_region_read().

>  	}

[ ... ]

> @@ -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);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but looking further down in
vfio_ccw_ops.c, is there a broken speculative mitigation in
vfio_ccw_mdev_ioctl_get_region_info()?

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_ioctl_get_region_info() {
    ...
    info->index = array_index_nospec(info->index,
                                     VFIO_CCW_NUM_REGIONS +
                                     private->num_regions);
    i = info->index - VFIO_CCW_NUM_REGIONS;
    info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index);
    info->size = private->region[i].size;
    ...
}

If info->index is maliciously out of bounds, array_index_nospec() will
speculatively force it to 0. Since VFIO_CCW_NUM_REGIONS is 1, i becomes -1.

Does this result in a deterministic speculative out-of-bounds read at
index -1 when evaluating private->region[i].size?

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