| Message ID | Z7dnrEpKQdRZ2qFU@Emma (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] iio: light: Add check for array bounds in veml6075_read_int_time_ms | expand |
On Thu Feb 20, 2025 at 6:34 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) > > Fixes: 3b82f43238ae ("iio: light: add VEML6075 UVA and UVB light sensor driver") > Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com> > --- > drivers/iio/light/veml6075.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/light/veml6075.c b/drivers/iio/light/veml6075.c > index 05d4c0e9015d..5dd951f6e989 100644 > --- a/drivers/iio/light/veml6075.c > +++ b/drivers/iio/light/veml6075.c > @@ -201,7 +201,12 @@ static int veml6075_read_int_time_index(struct veml6075_data *data) > if (ret < 0) > return ret; > > - return FIELD_GET(VEML6075_CONF_IT, conf); Please declare the variable at the beginning of the function (there are some integers there already) and rename it to follow the driver convention, it for integration time: it_index > + int int_index = FIELD_GET(VEML6075_CONF_IT, conf); > + > + if (int_index >= ARRAY_SIZE(veml6075_it_ms)) > + return -EINVAL; > + > + return int_index; > } > > static int veml6075_read_int_time_ms(struct veml6075_data *data, int *val) With that applied: Reviewed-by: Javier Carrasco <javier.carrasco@gmail.com>
On Thu Feb 20, 2025 at 9:00 PM CET, Javier Carrasco wrote: > On Thu Feb 20, 2025 at 6:34 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) > > > > Fixes: 3b82f43238ae ("iio: light: add VEML6075 UVA and UVB light sensor driver") > > Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com> > > --- > > drivers/iio/light/veml6075.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/light/veml6075.c b/drivers/iio/light/veml6075.c > > index 05d4c0e9015d..5dd951f6e989 100644 > > --- a/drivers/iio/light/veml6075.c > > +++ b/drivers/iio/light/veml6075.c > > @@ -201,7 +201,12 @@ static int veml6075_read_int_time_index(struct veml6075_data *data) > > if (ret < 0) > > return ret; > > > > - return FIELD_GET(VEML6075_CONF_IT, conf); > > Please declare the variable at the beginning of the function (there are > some integers there already) and rename it to follow the driver > convention, it for integration time: it_index > > > + int int_index = FIELD_GET(VEML6075_CONF_IT, conf); > > + > > + if (int_index >= ARRAY_SIZE(veml6075_it_ms)) > > + return -EINVAL; > > + > > + return int_index; > > } > > > > static int veml6075_read_int_time_ms(struct veml6075_data *data, int *val) > > With that applied: > > Reviewed-by: Javier Carrasco <javier.carrasco@gmail.com> Fix: Reviewed-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
On Thu, 20 Feb 2025 17:34:36 +0000 Karan Sanghavi <karansanghvi98@gmail.com> 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) > > Fixes: 3b82f43238ae ("iio: light: add VEML6075 UVA and UVB light sensor driver") > Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com> > --- Superficially this looks hardening against malicious or broken hardware. That is fine to add, but not worth backporting or (in my opinion) adding a fixes tag. > drivers/iio/light/veml6075.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/light/veml6075.c b/drivers/iio/light/veml6075.c > index 05d4c0e9015d..5dd951f6e989 100644 > --- a/drivers/iio/light/veml6075.c > +++ b/drivers/iio/light/veml6075.c > @@ -201,7 +201,12 @@ static int veml6075_read_int_time_index(struct veml6075_data *data) > if (ret < 0) > return ret; > > - return FIELD_GET(VEML6075_CONF_IT, conf); > + int int_index = FIELD_GET(VEML6075_CONF_IT, conf); > + > + if (int_index >= ARRAY_SIZE(veml6075_it_ms)) > + return -EINVAL; > + > + return int_index; > } > > static int veml6075_read_int_time_ms(struct veml6075_data *data, int *val)
On Sat, 22 Feb 2025 16:43:37 +0000 Jonathan Cameron <jic23@kernel.org> wrote: > On Thu, 20 Feb 2025 17:34:36 +0000 > Karan Sanghavi <karansanghvi98@gmail.com> 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) > > > > Fixes: 3b82f43238ae ("iio: light: add VEML6075 UVA and UVB light sensor driver") > > Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com> > > --- > Superficially this looks hardening against malicious or broken hardware. > That is fine to add, but not worth backporting or (in my opinion) adding a fixes > tag. Ah I see Javier asked for the fixes tag. Ok. Maybe just add a note that the hardware is not expected to return such an out of bounds value. That will help me to remember we don't need to rush this one upstream! Jonathan > > > drivers/iio/light/veml6075.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/light/veml6075.c b/drivers/iio/light/veml6075.c > > index 05d4c0e9015d..5dd951f6e989 100644 > > --- a/drivers/iio/light/veml6075.c > > +++ b/drivers/iio/light/veml6075.c > > @@ -201,7 +201,12 @@ static int veml6075_read_int_time_index(struct veml6075_data *data) > > if (ret < 0) > > return ret; > > > > - return FIELD_GET(VEML6075_CONF_IT, conf); > > + int int_index = FIELD_GET(VEML6075_CONF_IT, conf); > > + > > + if (int_index >= ARRAY_SIZE(veml6075_it_ms)) > > + return -EINVAL; > > + > > + return int_index; > > } > > > > static int veml6075_read_int_time_ms(struct veml6075_data *data, int *val) >
diff --git a/drivers/iio/light/veml6075.c b/drivers/iio/light/veml6075.c index 05d4c0e9015d..5dd951f6e989 100644 --- a/drivers/iio/light/veml6075.c +++ b/drivers/iio/light/veml6075.c @@ -201,7 +201,12 @@ static int veml6075_read_int_time_index(struct veml6075_data *data) if (ret < 0) return ret; - return FIELD_GET(VEML6075_CONF_IT, conf); + int int_index = FIELD_GET(VEML6075_CONF_IT, conf); + + if (int_index >= ARRAY_SIZE(veml6075_it_ms)) + return -EINVAL; + + return int_index; } static int veml6075_read_int_time_ms(struct veml6075_data *data, int *val)
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) Fixes: 3b82f43238ae ("iio: light: add VEML6075 UVA and UVB light sensor driver") Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com> --- drivers/iio/light/veml6075.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)