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

[Eric Farman <farman@linux.ibm.com>]

On 7/24/26 12:49 PM, Matthew Rosato wrote:
> 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@vger.kernel.org
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   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.

Ah, yes, it was bothering me too, but I guess I forgot about it.

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

This seems reasonable, and is definitely nicer to read than what I had 
before (which contained artifacts of the old/busted setup). I'm running 
tests on this now, but will include in a v4.

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