Re: [PATCH] iio: Fix the sorting functionality in iio_gts_build_avail_time_table

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 13 Mar 2024 10:57:35 -0500
Chenyuan Yang <chenyuan0y@gmail.com> wrote:

> The sorting in iio_gts_build_avail_time_table is not working as intended.
For function names, add () so
The sorting in iio_gts_build_avail_time_table() is not working as intended.

I can fix this up whilst applying but will be waiting for a RB or related
from Matti before queuing this up.

I'm assuming there are no existing drivers in tree that hit this problem?
If there are reply saying which ones so I can assess whether to rush this
fix in or not.

Thanks,

Jonathan


> It could result in an out-of-bounds access when the time is zero.
> 
> Here are more details:
> 
> 1. When the gts->itime_table[i].time_us is zero, e.g., the time
> sequence is `3, 0, 1`, the inner for-loop will not terminate and do
> out-of-bound writes. This is because once `times[j] > new`, the value
> `new` will be added in the current position and the `times[j]` will be
> moved to `j+1` position, which makes the if-condition always hold.
> Meanwhile, idx will be added one, making the loop keep running without
> termination and out-of-bound write.
> 2. If none of the gts->itime_table[i].time_us is zero, the elements
> will just be copied without being sorted as described in the comment
> "Sort times from all tables to one and remove duplicates".
> 
> For more details, please refer to https://lore.kernel.org/all/6dd0d822-046c-4dd2-9532-79d7ab96ec05@gmail.com.
> 
> Reported-by: Chenyuan Yang <chenyuan0y@gmail.com>
> Suggested-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Fixes: 38416c28e16890b52fdd5eb73479299ec3f062f3 ("iio: light: Add gain-time-scale helpers")
> Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> ---
> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> index 7653261d2dc2..61714a8d5717 100644
> --- a/drivers/iio/industrialio-gts-helper.c
> +++ b/drivers/iio/industrialio-gts-helper.c
> @@ -375,7 +375,7 @@ static int iio_gts_build_avail_time_table(struct iio_gts *gts)
>  	for (i = gts->num_itime - 1; i >= 0; i--) {
>  		int new = gts->itime_table[i].time_us;
>  
> -		if (times[idx] < new) {
> +		if (idx == 0 || times[idx - 1] < new) {
>  			times[idx++] = new;
>  			continue;
>  		}
> @@ -385,9 +385,10 @@ static int iio_gts_build_avail_time_table(struct iio_gts *gts)
>  				memmove(&times[j + 1], &times[j],
>  					(idx - j) * sizeof(int));
>  				times[j] = new;
> -				idx++;
> +				break;
>  			}
>  		}
> +		idx++;
>  	}
>  
>  	/* create a list of times formatted as list of IIO_VAL_INT_PLUS_MICRO */
>