[PATCH v2] iio: light: Add check for array bounds in veml6075_read_int_time_ms

[PATCH v2] iio: light: Add check for array bounds in veml6075_read_int_time_ms

[Karan Sanghavi]

The array contains only 5 elements, but the index calculated by
veml6075_read_int_time_index can range from 0 to 7,
which could lead to out-of-bounds access. The check prevents this issue.

Coverity Issue
CID 1574309: (#1 of 1): Out-of-bounds read (OVERRUN)
overrun-local: Overrunning array veml6075_it_ms of 5 4-byte
elements at element index 7 (byte offset 31) using
index int_index (which evaluates to 7)

Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>
---
 drivers/iio/light/veml6075.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/veml6075.c b/drivers/iio/light/veml6075.c
index 05d4c0e9015d..21de193ca09d 100644
--- a/drivers/iio/light/veml6075.c
+++ b/drivers/iio/light/veml6075.c
@@ -210,8 +210,8 @@ static int veml6075_read_int_time_ms(struct veml6075_data *data, int *val)
 
 	guard(mutex)(&data->lock);
 	int_index = veml6075_read_int_time_index(data);
-	if (int_index < 0)
-		return int_index;
+	if (int_index < 0 || int_index >= ARRAY_SIZE(veml6075_it_ms))
+		return -EINVAL;
 
 	*val = veml6075_it_ms[int_index];
 
-- 
2.43.0

Re: [PATCH v2] iio: light: Add check for array bounds in veml6075_read_int_time_ms

[Javier Carrasco]

On Mon Feb 3, 2025 at 5:05 PM CET, Karan Sanghavi wrote:
> The array contains only 5 elements, but the index calculated by
> veml6075_read_int_time_index can range from 0 to 7,
> which could lead to out-of-bounds access. The check prevents this issue.
>
> Coverity Issue
> CID 1574309: (#1 of 1): Out-of-bounds read (OVERRUN)
> overrun-local: Overrunning array veml6075_it_ms of 5 4-byte
> elements at element index 7 (byte offset 31) using
> index int_index (which evaluates to 7)
>

Hi Karan,

You have dropped the Fixes: tag, please add it again.

> Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>
> ---
>  drivers/iio/light/veml6075.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/light/veml6075.c b/drivers/iio/light/veml6075.c
> index 05d4c0e9015d..21de193ca09d 100644
> --- a/drivers/iio/light/veml6075.c
> +++ b/drivers/iio/light/veml6075.c
> @@ -210,8 +210,8 @@ static int veml6075_read_int_time_ms(struct veml6075_data *data, int *val)
>
>  	guard(mutex)(&data->lock);
>  	int_index = veml6075_read_int_time_index(data);
> -	if (int_index < 0)
> -		return int_index;
> +	if (int_index < 0 || int_index >= ARRAY_SIZE(veml6075_it_ms))
> +		return -EINVAL;

This approach modifies the original error codes that regmap_read() could
return, which are not always -EINVAL. You could check the index within
veml6075_read_int_time_index(), return -EINVAL, and this section would
stay as it is. There is another call to the function in the driver, but
the same approach would work there as well without modifications.

>
>  	*val = veml6075_it_ms[int_index];
>

I would also like to remind you that giving some reasonable time between
versions might let others review your patch. I am the maintainer of this
driver, but no matter how many iterations we go through, the final
decision is made by more qualified users i.e. subsystem maintainers. It
would be nice if we could save them from a bunch of versions in a short
period of time.

Apart from what I pointed out, the patch looks better now. Thanks!

Best regards,
Javier Carrasco