Re: [PATCH v5 03/10] s390/vfio_ccw: fix out of bounds check on CCW array

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Hardware Status Modifier skips allow the channel subsystem to fetch and execute CCWs from out-of-bounds host memory because VFIO prematurely truncates the allocated chain.
--

--- Patch [3]: [PATCH v5 03/10] s390/vfio_ccw: fix out of bounds check on CCW array ---
commit 7c8a75a862f0fe5c95ff1ad636c202fa276e66c2
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: fix out of bounds check on CCW array

This commit fixes an out-of-bounds array access in ccwchain_calc_length().
It restructures the loop to exit safely once the maximum number of CCWs
(256) have been processed, preventing the code from evaluating memory
outside the valid range.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c

[ ... ]

> @@ -377,11 +377,9 @@ static void ccwchain_cda_free(struct ccwchain *chain, int idx)
>  static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>  {
>  	struct ccw1 *ccw = cp->guest_cp;
> -	int cnt = 0;
> -
> -	do {
> -		cnt++;
> +	int cnt;
>  
> +	for (cnt = 1; cnt <= CCWCHAIN_LEN_MAX; cnt++, ccw++) {
>  		/*
>  		 * We want to keep counting if the current CCW has the
>  		 * command-chaining flag enabled, or if it is a TIC CCW
> @@ -391,15 +389,10 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>  		 * after the TIC, depending on the results of its operation.
>  		 */
>  		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
> -			break;
> -
> -		ccw++;
> -	} while (cnt < CCWCHAIN_LEN_MAX + 1);
> -
> -	if (cnt == CCWCHAIN_LEN_MAX + 1)
> -		cnt = -EINVAL;
> +			return cnt;

[Severity: Critical]
This is a pre-existing issue, but does this termination condition fully
account for hardware status modifier skips?

If a guest constructs a channel program where a CCW specifies chaining
and executes a command that generates a status modifier, and the
immediately following CCW explicitly clears the chaining flag, will
ccwchain_calc_length() stop counting at the second CCW?

If the calculation stops there, it appears the subsystem would allocate a host
buffer sized for exactly two CCWs. When executed, the hardware would see
the status modifier, skip the second CCW, and fetch a third CCW from
memory immediately following the allocated buffer.

Could this allow the channel subsystem to fetch and execute an untranslated,
out-of-bounds CCW from host memory if the guest places one adjacent to the
allocation?

> +	}
>  
> -	return cnt;
> +	return -EINVAL;
>  }
>  
>  static int tic_target_chain_exists(struct ccw1 *tic, struct channel_program *cp)

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