linux-kernel - Re: [PATCH v2] iio: light: Add check for array bounds in veml6075_read_int_time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <D7J4BHL127FC.NVE7SPPOFKOU@gmail.com>
Date: Mon, 03 Feb 2025 22:11:22 +0100
From: "Javier Carrasco" <javier.carrasco.cruz@...il.com>
To: "Karan Sanghavi" <karansanghvi98@...il.com>, "Jonathan Cameron"
 <jic23@...nel.org>, "Lars-Peter Clausen" <lars@...afoo.de>
Cc: <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Shuah
 Khan" <skhan@...uxfoundation.org>
Subject: Re: [PATCH v2] iio: light: Add check for array bounds in
 veml6075_read_int_time_ms

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@...il.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