[PATCH v2] iio: adc: ad4030: fix invalid oversampling_ratio validation

[PATCH v2] iio: adc: ad4030: fix invalid oversampling_ratio validation

[Salah Triki]

ad4030_set_avg_frame_len() computes avg_log2 = ilog2(avg_val) before
validating avg_val, and the subsequent range check only rejects
negative values or values above the maximum supported OSR. It does
not reject avg_val == 0, nor values that are not exact powers of 2.

- avg_val == 0 passes the check (0 is not < 0 and not > max), so
  ilog2(0) is called with an undefined/garbage result.

- Non-power-of-2 values (e.g. avg_val == 3) also pass the check and
  silently get rounded down by ilog2() to the nearest lower power of
  2, so userspace can write a value to the oversampling_ratio sysfs
  attribute that does not match what actually gets programmed into
  hardware, without any error being reported.

Only powers of 2 in [1, 65536] are valid OSR values, as listed in
ad4030_average_modes[]. Validate avg_val fully before computing its
log2, using is_power_of_2() and requiring avg_val > 0.

This issue was identified with assistance from Claude AI and manually
verified against the code.

Fixes: 949abd1ca5a4 ("iio: adc: ad4030: add averaging support")
Signed-off-by: Salah Triki <salah.triki@gmail.com>
---
Changes since v1:
   - Added note stating the issue was identified with assistance from
   Claude AI and verified manually.
   - Removed initialization of avg_log2 at declaration.

 drivers/iio/adc/ad4030.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
index 9c5f19321e3b..20911cc55873 100644
--- a/drivers/iio/adc/ad4030.c
+++ b/drivers/iio/adc/ad4030.c
@@ -746,14 +746,16 @@ static int ad4030_set_chan_calibbias(struct iio_dev *indio_dev,
 static int ad4030_set_avg_frame_len(struct iio_dev *dev, int avg_val)
 {
 	struct ad4030_state *st = iio_priv(dev);
-	unsigned int avg_log2 = ilog2(avg_val);
+	unsigned int avg_log2;
 	unsigned int last_avg_idx = ARRAY_SIZE(ad4030_average_modes) - 1;
 	int freq_hz;
 	int ret;
 
-	if (avg_val < 0 || avg_val > ad4030_average_modes[last_avg_idx])
+	if (avg_val <= 0 || avg_val > ad4030_average_modes[last_avg_idx] || !is_power_of_2(avg_val))
 		return -EINVAL;
 
+	avg_log2 = ilog2(avg_val);
+
 	if (st->offload_trigger) {
 		/*
 		 * The sample averaging and sampling frequency configurations
-- 
2.43.0

Re: [PATCH v2] iio: adc: ad4030: fix invalid oversampling_ratio validation

[David Lechner]

On 8/22/26 11:52 PM, Salah Triki wrote:
> ad4030_set_avg_frame_len() computes avg_log2 = ilog2(avg_val) before
> validating avg_val, and the subsequent range check only rejects
> negative values or values above the maximum supported OSR. It does
> not reject avg_val == 0, nor values that are not exact powers of 2.
> 
> - avg_val == 0 passes the check (0 is not < 0 and not > max), so
>   ilog2(0) is called with an undefined/garbage result.
> 
> - Non-power-of-2 values (e.g. avg_val == 3) also pass the check and
>   silently get rounded down by ilog2() to the nearest lower power of
>   2, so userspace can write a value to the oversampling_ratio sysfs
>   attribute that does not match what actually gets programmed into
>   hardware, without any error being reported.
> 
> Only powers of 2 in [1, 65536] are valid OSR values, as listed in
> ad4030_average_modes[]. Validate avg_val fully before computing its
> log2, using is_power_of_2() and requiring avg_val > 0.

There is no rule in IIO that says we can't round values when writing
attributes. Users are expected to read the attribute after writing to
see what actually took effect. So this patch doesn't seem justified.

> 
> This issue was identified with assistance from Claude AI and manually
> verified against the code.
> 
> Fixes: 949abd1ca5a4 ("iio: adc: ad4030: add averaging support")

Actually, this could break existing users that depend on the current
behavior. It certainly doesn't fix anything.

> Signed-off-by: Salah Triki <salah.triki@gmail.com>
> ---
> Changes since v1:
>    - Added note stating the issue was identified with assistance from
>    Claude AI and verified manually.
>    - Removed initialization of avg_log2 at declaration.
> 
>  drivers/iio/adc/ad4030.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

Re: [PATCH v2] iio: adc: ad4030: fix invalid oversampling_ratio validation

[Jonathan Cameron]

On Sun, 23 Aug 2026 13:29:32 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 8/22/26 11:52 PM, Salah Triki wrote:
> > ad4030_set_avg_frame_len() computes avg_log2 = ilog2(avg_val) before
> > validating avg_val, and the subsequent range check only rejects
> > negative values or values above the maximum supported OSR. It does
> > not reject avg_val == 0, nor values that are not exact powers of 2.
> > 
> > - avg_val == 0 passes the check (0 is not < 0 and not > max), so
> >   ilog2(0) is called with an undefined/garbage result.
> > 
> > - Non-power-of-2 values (e.g. avg_val == 3) also pass the check and
> >   silently get rounded down by ilog2() to the nearest lower power of
> >   2, so userspace can write a value to the oversampling_ratio sysfs
> >   attribute that does not match what actually gets programmed into
> >   hardware, without any error being reported.
> > 
> > Only powers of 2 in [1, 65536] are valid OSR values, as listed in
> > ad4030_average_modes[]. Validate avg_val fully before computing its
> > log2, using is_power_of_2() and requiring avg_val > 0.  
> 
> There is no rule in IIO that says we can't round values when writing
> attributes. Users are expected to read the attribute after writing to
> see what actually took effect. So this patch doesn't seem justified.
> 
> > 
> > This issue was identified with assistance from Claude AI and manually
> > verified against the code.
> > 
> > Fixes: 949abd1ca5a4 ("iio: adc: ad4030: add averaging support")  
> 
> Actually, this could break existing users that depend on the current
> behavior. It certainly doesn't fix anything.
The zero check may make sense even if the rest do not.

Jonathan

> 
> > Signed-off-by: Salah Triki <salah.triki@gmail.com>
> > ---
> > Changes since v1:
> >    - Added note stating the issue was identified with assistance from
> >    Claude AI and verified manually.
> >    - Removed initialization of avg_log2 at declaration.
> > 
> >  drivers/iio/adc/ad4030.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >   

Re: [PATCH v2] iio: adc: ad4030: fix invalid oversampling_ratio validation

[Andy Shevchenko]

On Sun, Aug 23, 2026 at 05:52:05AM +0100, Salah Triki wrote:
> ad4030_set_avg_frame_len() computes avg_log2 = ilog2(avg_val) before
> validating avg_val, and the subsequent range check only rejects
> negative values or values above the maximum supported OSR. It does
> not reject avg_val == 0, nor values that are not exact powers of 2.
> 
> - avg_val == 0 passes the check (0 is not < 0 and not > max), so
>   ilog2(0) is called with an undefined/garbage result.
> 
> - Non-power-of-2 values (e.g. avg_val == 3) also pass the check and
>   silently get rounded down by ilog2() to the nearest lower power of
>   2, so userspace can write a value to the oversampling_ratio sysfs
>   attribute that does not match what actually gets programmed into
>   hardware, without any error being reported.
> 
> Only powers of 2 in [1, 65536] are valid OSR values, as listed in
> ad4030_average_modes[]. Validate avg_val fully before computing its
> log2, using is_power_of_2() and requiring avg_val > 0.

No need to repeat in the commit message what we can see in the code.
Use plain English to write the problem statement, the solution approach
and what might happen if patch is not applied.

> This issue was identified with assistance from Claude AI and manually
> verified against the code.

Assisted-by?

> Fixes: 949abd1ca5a4 ("iio: adc: ad4030: add averaging support")
> Signed-off-by: Salah Triki <salah.triki@gmail.com>

...

>  	struct ad4030_state *st = iio_priv(dev);
> -	unsigned int avg_log2 = ilog2(avg_val);
> +	unsigned int avg_log2;
>  	unsigned int last_avg_idx = ARRAY_SIZE(ad4030_average_modes) - 1;
>  	int freq_hz;
>  	int ret;

Reorder (only the line you touched) to follow the reversed xmas tree ordering.

...

> -	if (avg_val < 0 || avg_val > ad4030_average_modes[last_avg_idx])
> +	if (avg_val <= 0 || avg_val > ad4030_average_modes[last_avg_idx] || !is_power_of_2(avg_val))
>  		return -EINVAL;

Split it, the

	if (avg_val == 0 || !is_power_of_2(avg_val))
		return -EINVAL;

is idiomatic as the 0-check required for is_power_of_2(). Also it puts the line
in the limits.

-- 
With Best Regards,
Andy Shevchenko