iio: gyro: bmg160: bail out when bandwidth/filter is not in table

[PATCH] iio: gyro: bmg160: bail out when bandwidth/filter is not in table

Posted by Stepan Ionichev 2 weeks, 6 days ago

bmg160_get_filter() walks bmg160_samp_freq_table[] looking for the
entry matching the bw_bits value read from the chip:

	for (i = 0; i < ARRAY_SIZE(bmg160_samp_freq_table); ++i) {
		if (bmg160_samp_freq_table[i].bw_bits == bw_bits)
			break;
	}
	*val = bmg160_samp_freq_table[i].filter;

If no entry matches, i ends up equal to the array size and the next
line reads one slot past the end. bmg160_set_filter() has the same
shape, driven by 'val' instead of bw_bits.

smatch flags both:

  drivers/iio/gyro/bmg160_core.c:204 bmg160_get_filter() error:
  buffer overflow 'bmg160_samp_freq_table' 7 <= 7
  drivers/iio/gyro/bmg160_core.c:222 bmg160_set_filter() error:
  buffer overflow 'bmg160_samp_freq_table' 7 <= 7

Return -EINVAL when no entry matches.

Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
---
 drivers/iio/gyro/bmg160_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
index 38394b5f3..58963f3ea 100644
--- a/drivers/iio/gyro/bmg160_core.c
+++ b/drivers/iio/gyro/bmg160_core.c
@@ -201,6 +201,9 @@ static int bmg160_get_filter(struct bmg160_data *data, int *val)
 			break;
 	}
 
+	if (i == ARRAY_SIZE(bmg160_samp_freq_table))
+		return -EINVAL;
+
 	*val = bmg160_samp_freq_table[i].filter;
 
 	return ret ? ret : IIO_VAL_INT;
@@ -218,6 +221,9 @@ static int bmg160_set_filter(struct bmg160_data *data, int val)
 			break;
 	}
 
+	if (i == ARRAY_SIZE(bmg160_samp_freq_table))
+		return -EINVAL;
+
 	ret = regmap_write(data->regmap, BMG160_REG_PMU_BW,
 			   bmg160_samp_freq_table[i].bw_bits);
 	if (ret < 0) {
-- 
2.43.0

Re: [PATCH] iio: gyro: bmg160: bail out when bandwidth/filter is not in table

Posted by Stepan Ionichev 2 weeks, 2 days ago

Fixes: 22b46c45fb9b ("iio:gyro:bmg160 Gyro Sensor driver")

The set_filter() path is reachable from userspace via the
sysfs in_anglvel_filter_low_pass_3db_frequency interface, so
userspace can trivially trigger the out-of-bounds read with a
value that is not in bmg160_samp_freq_table[].filter.

Stepan

Re: [PATCH] iio: gyro: bmg160: bail out when bandwidth/filter is not in table

Posted by Jonathan Cameron 1 week, 6 days ago

On Wed, 13 May 2026 13:12:51 +0500
Stepan Ionichev <sozdayvek@gmail.com> wrote:

> Fixes: 22b46c45fb9b ("iio:gyro:bmg160 Gyro Sensor driver")
> 
> The set_filter() path is reachable from userspace via the
> sysfs in_anglvel_filter_low_pass_3db_frequency interface, so
> userspace can trivially trigger the out-of-bounds read with a
> value that is not in bmg160_samp_freq_table[].filter.

Thanks. Applied with both the fixes tag and that extra bit of analysis
added.  Also marked for stable inclusion.

Jonathan

> 
> Stepan

Re: [PATCH] iio: gyro: bmg160: bail out when bandwidth/filter is not in table

Posted by Jonathan Cameron 2 weeks, 4 days ago

On Sun, 10 May 2026 07:35:00 +0500
Stepan Ionichev <sozdayvek@gmail.com> wrote:

> bmg160_get_filter() walks bmg160_samp_freq_table[] looking for the
> entry matching the bw_bits value read from the chip:
> 
> 	for (i = 0; i < ARRAY_SIZE(bmg160_samp_freq_table); ++i) {
> 		if (bmg160_samp_freq_table[i].bw_bits == bw_bits)
> 			break;
> 	}
> 	*val = bmg160_samp_freq_table[i].filter;
> 
> If no entry matches, i ends up equal to the array size and the next
> line reads one slot past the end. bmg160_set_filter() has the same
> shape, driven by 'val' instead of bw_bits.
> 
> smatch flags both:
> 
>   drivers/iio/gyro/bmg160_core.c:204 bmg160_get_filter() error:
>   buffer overflow 'bmg160_samp_freq_table' 7 <= 7
>   drivers/iio/gyro/bmg160_core.c:222 bmg160_set_filter() error:
>   buffer overflow 'bmg160_samp_freq_table' 7 <= 7
> 
> Return -EINVAL when no entry matches.
> 
> Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
For the get case it would be a hardware bug to return wrong value.
That's good to harden against but not something I'd consider an urgent bug.

The set case is a different matter. Looks to me like userspace can trivally cause
an overflow.  As such this needs a fixes tag.  Please can you send one in reply
to this email so I can pick it up when taking the patch.

Thanks and good fix!

Jonathan

> ---
>  drivers/iio/gyro/bmg160_core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
> index 38394b5f3..58963f3ea 100644
> --- a/drivers/iio/gyro/bmg160_core.c
> +++ b/drivers/iio/gyro/bmg160_core.c
> @@ -201,6 +201,9 @@ static int bmg160_get_filter(struct bmg160_data *data, int *val)
>  			break;
>  	}
>  
> +	if (i == ARRAY_SIZE(bmg160_samp_freq_table))
> +		return -EINVAL;
> +
>  	*val = bmg160_samp_freq_table[i].filter;
>  
>  	return ret ? ret : IIO_VAL_INT;
> @@ -218,6 +221,9 @@ static int bmg160_set_filter(struct bmg160_data *data, int val)
>  			break;
>  	}
>  
> +	if (i == ARRAY_SIZE(bmg160_samp_freq_table))
> +		return -EINVAL;
> +
>  	ret = regmap_write(data->regmap, BMG160_REG_PMU_BW,
>  			   bmg160_samp_freq_table[i].bw_bits);
>  	if (ret < 0) {

Re: [PATCH] iio: gyro: bmg160: bail out when bandwidth/filter is not in table

Posted by Andy Shevchenko 2 weeks, 5 days ago

On Sun, May 10, 2026 at 07:35:00AM +0500, Stepan Ionichev wrote:
> bmg160_get_filter() walks bmg160_samp_freq_table[] looking for the
> entry matching the bw_bits value read from the chip:
> 
> 	for (i = 0; i < ARRAY_SIZE(bmg160_samp_freq_table); ++i) {
> 		if (bmg160_samp_freq_table[i].bw_bits == bw_bits)
> 			break;
> 	}
> 	*val = bmg160_samp_freq_table[i].filter;
> 
> If no entry matches, i ends up equal to the array size and the next
> line reads one slot past the end. bmg160_set_filter() has the same
> shape, driven by 'val' instead of bw_bits.
> 
> smatch flags both:
> 
>   drivers/iio/gyro/bmg160_core.c:204 bmg160_get_filter() error:
>   buffer overflow 'bmg160_samp_freq_table' 7 <= 7
>   drivers/iio/gyro/bmg160_core.c:222 bmg160_set_filter() error:
>   buffer overflow 'bmg160_samp_freq_table' 7 <= 7
> 
> Return -EINVAL when no entry matches.

Sounds legit and proper behaviour in this case.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: gyro: bmg160: bail out when bandwidth/filter is not in table

Posted by Stepan Ionichev 2 weeks, 6 days ago

I checked patchwork for bmg160 -- no prior post for this fix. If you
remember the thread, point me to it.

Same pattern (loop with 'i' used after no-break) appears in other iio
drivers too. Maybe that is what you saw.

Stepan

Re: [PATCH] iio: gyro: bmg160: bail out when bandwidth/filter is not in table

Posted by Andy Shevchenko 2 weeks, 5 days ago

On Sun, May 10, 2026 at 08:06:47AM +0500, Stepan Ionichev wrote:
> I checked patchwork for bmg160 -- no prior post for this fix. If you
> remember the thread, point me to it.

Okay, thanks for confirming!

> Same pattern (loop with 'i' used after no-break) appears in other iio
> drivers too. Maybe that is what you saw.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: gyro: bmg160: bail out when bandwidth/filter is not in table

Posted by Andy Shevchenko 2 weeks, 5 days ago

On Sun, May 10, 2026 at 07:35:00AM +0500, Stepan Ionichev wrote:
> bmg160_get_filter() walks bmg160_samp_freq_table[] looking for the
> entry matching the bw_bits value read from the chip:
> 
> 	for (i = 0; i < ARRAY_SIZE(bmg160_samp_freq_table); ++i) {
> 		if (bmg160_samp_freq_table[i].bw_bits == bw_bits)
> 			break;
> 	}
> 	*val = bmg160_samp_freq_table[i].filter;
> 
> If no entry matches, i ends up equal to the array size and the next
> line reads one slot past the end. bmg160_set_filter() has the same
> shape, driven by 'val' instead of bw_bits.
> 
> smatch flags both:
> 
>   drivers/iio/gyro/bmg160_core.c:204 bmg160_get_filter() error:
>   buffer overflow 'bmg160_samp_freq_table' 7 <= 7
>   drivers/iio/gyro/bmg160_core.c:222 bmg160_set_filter() error:
>   buffer overflow 'bmg160_samp_freq_table' 7 <= 7
> 
> Return -EINVAL when no entry matches.

Have you checked the mailing list archive? I have a weak memory of seeing this
or something similar in the (recent) past...

-- 
With Best Regards,
Andy Shevchenko