Re: [PATCH v3 3/8] s390/vfio_ccw: fix out of bounds check on CCW array

[Matthew Rosato]

On 7/23/26 1:47 PM, Eric Farman wrote:
> The routine ccwchain_calc_length() counts the number of channel
> command words (CCWs) that are chained together in a single channel
> program, and rejects anything larger than CCWCHAIN_LEN_MAX (256) CCWs.
> 
> The loop itself is "do..while (count < 257)", and while the logic in
> is_cpa_within_range() correctly adjusts between the 0-index array of
> CCWs and the count of CCWs starting at 1, this means it would look
> at a possible 257th CCW before ending the loop and (correctly)
> returning an error.
> 
> Fix this by restructuring the loop to break as soon as 256 CCWs
> (thus indexes 0-255) are examined, without looking at memory
> outside the range.
> 
> Fixes: 0a19e61e6d4c ("vfio: ccw: introduce channel program interfaces")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 

So, I think your approach is correct, but I tried for a bit to come up
with a more concise solution because the while(1) was bothering me.
The best I've managed to cook up is something like:

static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
{
	struct ccw1 *ccw = cp->guest_cp;
	int cnt;

	for (cnt = 1; cnt <= CCWCHAIN_LEN_MAX; cnt++, ccw++) {
		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
			return cnt;
	}

	return -EINVAL;
}

Assume CCWCHAIN_LEN_MAX=2
We enter with cnt=1, ccw=guest_cp[0], and 1 <=2 so we enter loop
either return 1 or loop, assume loop so we increment
now cnt=2, ccw=guest_cp[1], 2 <= 2 so we enter loop
either return 2 or loop, assume loop so we increment
now cnt=3, ccw=guest_cp[2]*, 3 <= 2 so we break out of loop and return -EINVAL

*So the only catch is that we will increment ccw past the end, but unlike the
original implementation we will break out of the loop before ever attempting to
use that ccw pointer at that point.

What do you think?

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 1c2890d139c6..258a99930c6a 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -393,11 +393,14 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>  		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
>  			break;
>  
> -		ccw++;
> -	} while (cnt < CCWCHAIN_LEN_MAX + 1);
> +		/* Exit the loop when we reach the maximum */
> +		if (cnt >= CCWCHAIN_LEN_MAX) {
> +			cnt = -EINVAL;
> +			break;
> +		}
>  
> -	if (cnt == CCWCHAIN_LEN_MAX + 1)
> -		cnt = -EINVAL;
> +		ccw++;
> +	} while (1);
>  
>  	return cnt;
>  }