[v1] iio: adc: stm32-adc: fix possible division by zero in processed channel

[PATCH] iio: adc: stm32-adc: fix possible division by zero in processed channel

Posted by Fabrice Gasnier 1 week, 2 days ago

In case the conversion has failed or returned zero, processing *val
can lead to a division by zero.
Need to check for errors, or converted value is zero, before processing
the data.
In case converted value is zero, e.g. the Vrefint channel, this should
be considered as invalid in all case.

Fixes: 0e346b2cfa85 ("iio: adc: stm32-adc: add vrefint calibration support")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/all/20260911161555.244F31F000FF@smtp.kernel.org/
Cc: stable@vger.kernel.org
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/iio/adc/stm32-adc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 5c6c06b269be..d290c5a8f86d 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -1609,8 +1609,12 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
 		else
 			ret = -EINVAL;
 
-		if (mask == IIO_CHAN_INFO_PROCESSED)
-			*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / *val;
+		if (mask == IIO_CHAN_INFO_PROCESSED) {
+			if (ret >= 0 && *val)
+				*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / *val;
+			else
+				ret = ret < 0 ? ret : -EINVAL;
+		}
 
 		iio_device_release_direct(indio_dev);
 		return ret;

---
base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
change-id: 20260915-adc-fix-div0-ae38365abfeb

Best regards,
--  
Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Re: [PATCH] iio: adc: stm32-adc: fix possible division by zero in processed channel

Posted by Andy Shevchenko 1 week, 1 day ago

On Tue, Sep 15, 2026 at 06:10:40PM +0200, Fabrice Gasnier wrote:
> In case the conversion has failed or returned zero, processing *val
> can lead to a division by zero.
> Need to check for errors, or converted value is zero, before processing
> the data.
> In case converted value is zero, e.g. the Vrefint channel, this should
> be considered as invalid in all case.

Something went very wrong with the indentation of the above.

...

> -		if (mask == IIO_CHAN_INFO_PROCESSED)
> -			*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / *val;
> +		if (mask == IIO_CHAN_INFO_PROCESSED) {
> +			if (ret >= 0 && *val)
> +				*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / *val;
> +			else
> +				ret = ret < 0 ? ret : -EINVAL;
> +		}

Reuse of the *val makes code harder to follow. Add a temporary variable for
this and do something like this (also note other simplifications)

		tmp_choose_good_name = *val;
		...
		if (mask == IIO_CHAN_INFO_PROCESSED) {
			if (ret < 0)
				return ret;
			if (tmp == 0)
				return -EINVAL;

			*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / tmp;
		}

>  		iio_device_release_direct(indio_dev);
>  		return ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: stm32-adc: fix possible division by zero in processed channel

Posted by Fabrice Gasnier 1 week, 1 day ago

On 9/16/26 09:35, Andy Shevchenko wrote:
> On Tue, Sep 15, 2026 at 06:10:40PM +0200, Fabrice Gasnier wrote:
>> In case the conversion has failed or returned zero, processing *val
>> can lead to a division by zero.
>> Need to check for errors, or converted value is zero, before processing
>> the data.
>> In case converted value is zero, e.g. the Vrefint channel, this should
>> be considered as invalid in all case.
> 
> Something went very wrong with the indentation of the above.

Hi Andy,

Euh, sorry but I don't understand what's wrong with indentation in the
commit message ? Could you clarify ?

> 
> ...
> 
>> -		if (mask == IIO_CHAN_INFO_PROCESSED)
>> -			*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / *val;
>> +		if (mask == IIO_CHAN_INFO_PROCESSED) {
>> +			if (ret >= 0 && *val)
>> +				*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / *val;
>> +			else
>> +				ret = ret < 0 ? ret : -EINVAL;
>> +		}
> 
> Reuse of the *val makes code harder to follow. Add a temporary variable for
> this and do something like this (also note other simplifications)
> 
> 		tmp_choose_good_name = *val;

In case ret is an error; this lead to evaluate (e.g. read) *val from the
(maybe uninitialized) stack. Probably not an issue? But would prefer to
keep reading it only when ret >= 0, as done currently with the if
condition above ? (e.g. evaluate ret, before *val)

> 		...
> 		if (mask == IIO_CHAN_INFO_PROCESSED) {
> 			if (ret < 0)
> 				return ret;
> 			if (tmp == 0)
> 				return -EINVAL;

I've started with something similar before, but we can't return directly
here. Must call iio_device_release_direct() first. This would add more
lines.

So I chose to implement above ternary ret = ret < 0 ? ret : -EINVAL, to
fall-through.

Please advise on the preferred way,

Best Regards,
Fabrice

> 
> 			*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / tmp;
> 		}
> 
>>  		iio_device_release_direct(indio_dev);
>>  		return ret;
>

Re: [PATCH] iio: adc: stm32-adc: fix possible division by zero in processed channel

Posted by Andy Shevchenko 1 week, 1 day ago

On Wed, Sep 16, 2026 at 12:23:38PM +0200, Fabrice Gasnier wrote:
> 
> On 9/16/26 09:35, Andy Shevchenko wrote:
> > On Tue, Sep 15, 2026 at 06:10:40PM +0200, Fabrice Gasnier wrote:
> >> In case the conversion has failed or returned zero, processing *val
> >> can lead to a division by zero.
> >> Need to check for errors, or converted value is zero, before processing
> >> the data.
> >> In case converted value is zero, e.g. the Vrefint channel, this should
> >> be considered as invalid in all case.
> > 
> > Something went very wrong with the indentation of the above.
> 
> Hi Andy,
> 
> Euh, sorry but I don't understand what's wrong with indentation in the
> commit message ? Could you clarify ?

Each sentence seems to be a separate paragraph. Paragraphs are delimited
by a blank lines. But are they (sentences) really so independent?

Compare to:

  In case the conversion has failed or returned zero, processing *val
  can lead to a division by zero. Need to check for errors, or converted
  value is zero, before processing the data. In case converted value
  is zero, e.g. the Vrefint channel, this should be considered as invalid
  in all cases.

(also I fixed the typo, should be plural for 'cases' at the end).

...

> >> -		if (mask == IIO_CHAN_INFO_PROCESSED)
> >> -			*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / *val;
> >> +		if (mask == IIO_CHAN_INFO_PROCESSED) {
> >> +			if (ret >= 0 && *val)
> >> +				*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / *val;
> >> +			else
> >> +				ret = ret < 0 ? ret : -EINVAL;
> >> +		}
> > 
> > Reuse of the *val makes code harder to follow. Add a temporary variable for
> > this and do something like this (also note other simplifications)
> > 
> > 		tmp_choose_good_name = *val;
> 
> In case ret is an error; this lead to evaluate (e.g. read) *val from the
> (maybe uninitialized) stack. Probably not an issue? But would prefer to
> keep reading it only when ret >= 0, as done currently with the if
> condition above ? (e.g. evaluate ret, before *val)

> > 		...
> > 		if (mask == IIO_CHAN_INFO_PROCESSED) {
> > 			if (ret < 0)
> > 				return ret;

Then simply move the assignment here

			tmp_choose_good_name = *val;

> > 			if (tmp == 0)
> > 				return -EINVAL;
> 
> I've started with something similar before, but we can't return directly
> here. Must call iio_device_release_direct() first. This would add more
> lines.
> 
> So I chose to implement above ternary ret = ret < 0 ? ret : -EINVAL, to
> fall-through.
> 
> Please advise on the preferred way,

As per above.

> > 			*val = STM32_ADC_VREFINT_VOLTAGE * adc->vrefint.vrefint_cal / tmp;
> > 		}
> > 
> >>  		iio_device_release_direct(indio_dev);
> >>  		return ret;

-- 
With Best Regards,
Andy Shevchenko