iio: chemical: scd30: Prevent potential divide-by-zero error

[PATCH] iio: chemical: scd30: Prevent potential divide-by-zero error

Posted by Maxwell Doose 2 weeks, 6 days ago

In scd30_read_raw, the current value of tmp in the
IIO_CHAN_INFO_SAMP_FREQ case is unchecked. Add checking to see if the
value we got was 0 to prevent a divide-by-zero error.

Fixes: 64b3d8b1b0f5 ("iio: chemical: scd30: add core driver")
Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
 drivers/iio/chemical/scd30_core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
index be8c055be184..3e2bb6f4e42c 100644
--- a/drivers/iio/chemical/scd30_core.c
+++ b/drivers/iio/chemical/scd30_core.c
@@ -237,6 +237,13 @@ static int scd30_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const
 		if (ret)
 			return ret;
 
+		/* Likely only taken if something really strange happens */
+		if (!tmp) {
+			dev_err(&indio_dev->dev,
+				"Invalid measurement interval 0 received\n");
+			return -EIO;
+		}
+
 		*val = 0;
 		*val2 = 1000000000 / tmp;
 		return IIO_VAL_INT_PLUS_NANO;
-- 
2.54.0

Re: [PATCH] iio: chemical: scd30: Prevent potential divide-by-zero error

Posted by David Lechner 2 weeks, 6 days ago

On 5/9/26 4:19 PM, Maxwell Doose wrote:
> In scd30_read_raw, the current value of tmp in the
> IIO_CHAN_INFO_SAMP_FREQ case is unchecked. Add checking to see if the
> value we got was 0 to prevent a divide-by-zero error.
> 
> Fixes: 64b3d8b1b0f5 ("iio: chemical: scd30: add core driver")
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> ---
>  drivers/iio/chemical/scd30_core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
> index be8c055be184..3e2bb6f4e42c 100644
> --- a/drivers/iio/chemical/scd30_core.c
> +++ b/drivers/iio/chemical/scd30_core.c
> @@ -237,6 +237,13 @@ static int scd30_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const
>  		if (ret)
>  			return ret;
>  
> +		/* Likely only taken if something really strange happens */

Instead of saying "strange" I would mention a possible hardware failure
to justify the EIO return. I'm assuming this is reading the value over
SPI or I2C?

> +		if (!tmp) {
> +			dev_err(&indio_dev->dev,
> +				"Invalid measurement interval 0 received\n");
> +			return -EIO;
> +		}
> +
>  		*val = 0;
>  		*val2 = 1000000000 / tmp;
>  		return IIO_VAL_INT_PLUS_NANO;

Re: [PATCH] iio: chemical: scd30: Prevent potential divide-by-zero error

Posted by Maxwell Doose 2 weeks, 6 days ago

On Sat, May 9, 2026 at 4:25 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On 5/9/26 4:19 PM, Maxwell Doose wrote:
> > In scd30_read_raw, the current value of tmp in the
> > IIO_CHAN_INFO_SAMP_FREQ case is unchecked. Add checking to see if the
> > value we got was 0 to prevent a divide-by-zero error.
> >
> > Fixes: 64b3d8b1b0f5 ("iio: chemical: scd30: add core driver")
> > Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> > ---
> >  drivers/iio/chemical/scd30_core.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
> > index be8c055be184..3e2bb6f4e42c 100644
> > --- a/drivers/iio/chemical/scd30_core.c
> > +++ b/drivers/iio/chemical/scd30_core.c
> > @@ -237,6 +237,13 @@ static int scd30_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const
> >               if (ret)
> >                       return ret;
> >
> > +             /* Likely only taken if something really strange happens */
>
> Instead of saying "strange" I would mention a possible hardware failure
> to justify the EIO return. I'm assuming this is reading the value over
> SPI or I2C?
>

Yes, we're reading over either SPI or I2C, the sensor supports both.
Hardware failure is going to be the most likely cause of this path
being taken, but I could also see some sort of noise on the bus being
the cause. Perhaps we change it to:

/* Likely taken if hardware is failing or noise on bus */

best regards,
max

Re: [PATCH] iio: chemical: scd30: Prevent potential divide-by-zero error

Posted by David Lechner 2 weeks, 6 days ago

On 5/9/26 4:36 PM, Maxwell Doose wrote:
> On Sat, May 9, 2026 at 4:25 PM David Lechner <dlechner@baylibre.com> wrote:
>>
>> On 5/9/26 4:19 PM, Maxwell Doose wrote:
>>> In scd30_read_raw, the current value of tmp in the
>>> IIO_CHAN_INFO_SAMP_FREQ case is unchecked. Add checking to see if the
>>> value we got was 0 to prevent a divide-by-zero error.
>>>
>>> Fixes: 64b3d8b1b0f5 ("iio: chemical: scd30: add core driver")
>>> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
>>> ---
>>>  drivers/iio/chemical/scd30_core.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
>>> index be8c055be184..3e2bb6f4e42c 100644
>>> --- a/drivers/iio/chemical/scd30_core.c
>>> +++ b/drivers/iio/chemical/scd30_core.c
>>> @@ -237,6 +237,13 @@ static int scd30_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const
>>>               if (ret)
>>>                       return ret;
>>>
>>> +             /* Likely only taken if something really strange happens */
>>
>> Instead of saying "strange" I would mention a possible hardware failure
>> to justify the EIO return. I'm assuming this is reading the value over
>> SPI or I2C?
>>
> 
> Yes, we're reading over either SPI or I2C, the sensor supports both.
> Hardware failure is going to be the most likely cause of this path
> being taken, but I could also see some sort of noise on the bus being
> the cause. Perhaps we change it to:
> 
> /* Likely taken if hardware is failing or noise on bus */
> 
> best regards,
> max

Sounds OK. 

A more verbose alternative:

/*
 * Value of 0 is unexpected, but could happen, e.g. due to
 * hardware failure.
 */

Re: [PATCH] iio: chemical: scd30: Prevent potential divide-by-zero error

Posted by Maxwell Doose 2 weeks, 6 days ago

On Sat, May 9, 2026 at 4:47 PM David Lechner <dlechner@baylibre.com> wrote:
> On 5/9/26 4:36 PM, Maxwell Doose wrote:
> > Yes, we're reading over either SPI or I2C, the sensor supports both.
> > Hardware failure is going to be the most likely cause of this path
> > being taken, but I could also see some sort of noise on the bus being
> > the cause. Perhaps we change it to:
> >
> > /* Likely taken if hardware is failing or noise on bus */
> >
> > best regards,
> > max
>
> Sounds OK.
>
> A more verbose alternative:
>
> /*
>  * Value of 0 is unexpected, but could happen, e.g. due to
>  * hardware failure.
>  */

Also works for me, I'm impartial to either. Perhaps we can see if
Jonathan is willing to change it during merge or if I'll have to go
back and change it in a v2.

best regards,
max

Re: [PATCH] iio: chemical: scd30: Prevent potential divide-by-zero error

Posted by Maxwell Doose 2 weeks, 6 days ago

On Sat, May 9, 2026 at 4:53 PM Maxwell Doose <m32285159@gmail.com> wrote:
>
> On Sat, May 9, 2026 at 4:47 PM David Lechner <dlechner@baylibre.com> wrote:
> > On 5/9/26 4:36 PM, Maxwell Doose wrote:
> > > Yes, we're reading over either SPI or I2C, the sensor supports both.
> > > Hardware failure is going to be the most likely cause of this path
> > > being taken, but I could also see some sort of noise on the bus being
> > > the cause. Perhaps we change it to:
> > >
> > > /* Likely taken if hardware is failing or noise on bus */
> > >
> > > best regards,
> > > max
> >
> > Sounds OK.
> >
> > A more verbose alternative:
> >
> > /*
> >  * Value of 0 is unexpected, but could happen, e.g. due to
> >  * hardware failure.
> >  */
>
> Also works for me, I'm impartial to either. Perhaps we can see if
> Jonathan is willing to change it during merge or if I'll have to go
> back and change it in a v2.
>
> best regards,
> max

Also maybe we update to use dev_err_ratelimited(), sashiko has a good
point here:

https://sashiko.dev/#/patchset/20260509211921.84969-1-m32285159%40gmail.com

best regards,
max

Re: [PATCH] iio: chemical: scd30: Prevent potential divide-by-zero error

Posted by Jonathan Cameron 2 weeks, 4 days ago

On Sat, 9 May 2026 19:47:53 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Sat, May 9, 2026 at 4:53 PM Maxwell Doose <m32285159@gmail.com> wrote:
> >
> > On Sat, May 9, 2026 at 4:47 PM David Lechner <dlechner@baylibre.com> wrote:  
> > > On 5/9/26 4:36 PM, Maxwell Doose wrote:  
> > > > Yes, we're reading over either SPI or I2C, the sensor supports both.
> > > > Hardware failure is going to be the most likely cause of this path
> > > > being taken, but I could also see some sort of noise on the bus being
> > > > the cause. Perhaps we change it to:
> > > >
> > > > /* Likely taken if hardware is failing or noise on bus */
> > > >
> > > > best regards,
> > > > max  
> > >
> > > Sounds OK.
> > >
> > > A more verbose alternative:
> > >
> > > /*
> > >  * Value of 0 is unexpected, but could happen, e.g. due to
> > >  * hardware failure.
> > >  */  
> >
> > Also works for me, I'm impartial to either. Perhaps we can see if
> > Jonathan is willing to change it during merge or if I'll have to go
> > back and change it in a v2.
> >
> > best regards,
> > max  
> 
> Also maybe we update to use dev_err_ratelimited(), sashiko has a good
> point here:
> 
> https://sashiko.dev/#/patchset/20260509211921.84969-1-m32285159%40gmail.com
> 
Do we care for something that is going to be a very rare hardware failure?
That's one interesting attack vector. I suppose maybe they have another
exploit that lets them trip the scd30 into an odd state. 

Ah well, doesn't do much harm to rate limit it I guess.

Jonathan

> best regards,
> max

Re: [PATCH] iio: chemical: scd30: Prevent potential divide-by-zero error

Posted by Andy Shevchenko 2 weeks, 4 days ago

On Mon, May 11, 2026 at 01:03:10PM +0100, Jonathan Cameron wrote:
> On Sat, 9 May 2026 19:47:53 -0500
> Maxwell Doose <m32285159@gmail.com> wrote:
> > On Sat, May 9, 2026 at 4:53 PM Maxwell Doose <m32285159@gmail.com> wrote:

...

> > Also maybe we update to use dev_err_ratelimited(), sashiko has a good
> > point here:
> > 
> > https://sashiko.dev/#/patchset/20260509211921.84969-1-m32285159%40gmail.com
> > 
> Do we care for something that is going to be a very rare hardware failure?

If it happens rarely, ratelimit is no-op (no change from regular printing),
the only difference if (and I think this is actually what may happen IRL)
there is an intermittent failure of HW and it starts spamming logs like a
hell.

> That's one interesting attack vector. I suppose maybe they have another
> exploit that lets them trip the scd30 into an odd state. 
> 
> Ah well, doesn't do much harm to rate limit it I guess.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: chemical: scd30: Prevent potential divide-by-zero error

Posted by Maxwell Doose 2 weeks, 4 days ago

On Mon, May 11, 2026 at 8:31 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, May 11, 2026 at 01:03:10PM +0100, Jonathan Cameron wrote:
> > On Sat, 9 May 2026 19:47:53 -0500
> > Maxwell Doose <m32285159@gmail.com> wrote:
> > > On Sat, May 9, 2026 at 4:53 PM Maxwell Doose <m32285159@gmail.com> wrote:
>
> ...
>
> > > Also maybe we update to use dev_err_ratelimited(), sashiko has a good
> > > point here:
> > >
> > > https://sashiko.dev/#/patchset/20260509211921.84969-1-m32285159%40gmail.com
> > >
> > Do we care for something that is going to be a very rare hardware failure?
>
> If it happens rarely, ratelimit is no-op (no change from regular printing),
> the only difference if (and I think this is actually what may happen IRL)
> there is an intermittent failure of HW and it starts spamming logs like a
> hell.
>

Yeah, I believe that sashiko was pointing out that if we have a device
spamming a bunch of 0s then dmesg would get flooded with errors.

best regards,
max



> > That's one interesting attack vector. I suppose maybe they have another
> > exploit that lets them trip the scd30 into an odd state.
> >
> > Ah well, doesn't do much harm to rate limit it I guess.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>