Re: [PATCH v3 6/8] 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:

Pre-existing issues:
- [Critical] Concurrent manipulation of the `private->crw` linked list without locking.
--

--- Patch [6]: [PATCH v3 6/8] s390/vfio_ccw: ensure index for read/write regions are within range ---
commit 53ce18d2d82864d2f220e720e9f85b015a5a665c
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: ensure index for read/write regions are within range

This commit ensures that region indexes for existing read/write regions
are properly clamped to the range of the capabilities chain using
array_index_nospec().

> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 38c176cf62957..bf678270c4572 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
[ ... ]
> @@ -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);

[Severity: Critical]
This is a pre-existing issue, but is it possible for concurrent reads to
corrupt the private->crw linked list here?

Looking at vfio_ccw_crw_region_read(), it appears the crw list element is
retrieved and removed from the list before acquiring any locks:

vfio_ccw_chp.c:vfio_ccw_crw_region_read() {
    ...
    crw = list_first_entry_or_null(&private->crw, struct vfio_ccw_crw, next);
    if (crw)
        list_del(&crw->next);
    
    mutex_lock(&private->io_mutex);
    ...
}

Could two concurrent pread() calls (e.g., from an untrusted QEMU guest)
enter this function simultaneously, fetch the exact same crw element, and
both execute list_del() and kfree()?

Additionally, could asynchronous hardware events from the CSS driver add
elements to the list simultaneously?

For example, vfio_ccw_queue_crw() calls:
    list_add_tail(&crw->next, &private->crw);

Could this unsynchronized addition run simultaneously with the VFS read path
list removals and lead to slab corruption?

>  	region = private->region[i].data;
>  
>  	if (crw)

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