Re: [PATCH v2 2/7] s390/vfio_ccw: limit the number of channel program segments

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] Returning a pointer (`ERR_PTR`) from a function that returns an `int`.

Pre-existing issues:
- [High] Concurrent unprotected access to channel program structures via ioctl reset and write paths.
- [High] Out-of-bounds read on `cp->guest_cp` when calculating the CCW chain length.
--

--- Patch [2]: [PATCH v2 2/7] s390/vfio_ccw: limit the number of channel program segments ---
commit 55497ee3c2db8e265add0b6679cc5ddd751ba7ce
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: limit the number of channel program segments

This commit imposes a limit on the number of channel program segments to
prevent excessive recursive processing. It tracks the count in the
channel_program structure and fails if it exceeds CCWCHAIN_COUNT_MAX.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 086d1b54bdb05..7abdebd8f7c3d 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c

[ ... ]

> @@ -441,6 +442,10 @@ static int ccwchain_handle_ccw(dma32_t cda, struct channel_program *cp)
>  	if (len < 0)
>  		return len;
>  
> +	/* Limit number of chains in a single channel program */
> +	if (cp->ccwchain_count >= CCWCHAIN_COUNT_MAX)
> +		return ERR_PTR(-EINVAL);

[Severity: Low]
Does this return a pointer from a function that returns an int?
ccwchain_handle_ccw() returns an int, but ERR_PTR(-EINVAL) returns a void
pointer, which could cause a type mismatch or compilation warning.

[ ... ]

[Severity: High]
This is a pre-existing issue, but can concurrent unprotected access to
channel program structures occur via the ioctl reset and write paths?

If a user-space process concurrently invokes ioctl(VFIO_DEVICE_RESET) and
write() on the VFIO device file descriptor:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_reset() {
    ...
    vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
    ...
}

This calls into the state machine to close and free the channel program
without acquiring private->io_mutex:

drivers/s390/cio/vfio_ccw_fsm.c:fsm_close() {
    ...
    cp_free(&private->cp);
    ...
}

Concurrently, vfio_ccw_mdev_write_io_region() acquires io_mutex and processes
IO, allocating and using private->cp. Could this data race lead to a
use-after-free?

[Severity: High]
This is a pre-existing issue, but is there an out-of-bounds read on
cp->guest_cp when calculating the CCW chain length?

If an unprivileged user provides a channel program filled with exactly 256
command-chained CCWs:

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_calc_length() {
    ...
    if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
        break;

    ccw++;
} while (cnt < CCWCHAIN_LEN_MAX + 1);

The loop executes a 257th iteration when cnt == 257, evaluating
!ccw_is_chain(ccw) on guest_cp[256], which points past the 256-element
guest_cp array.

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