Re: [PATCH v3] iio: magnetometer: rm3100: add boundary check for the value read from RM3100_REG_TMRC

[Zhouyi Zhou]

On Wed, Jan 3, 2024 at 3:42 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Tue,  2 Jan 2024 09:07:11 +0800
> zhouzhouyi@xxxxxxxxx wrote:
>
> > From: "zhili.liu" <zhili.liu@xxxxxxxxxxx>
> >
> > Recently, we encounter kernel crash in function rm3100_common_probe
> > caused by out of bound access of array rm3100_samp_rates (because of
> > underlying hardware failures). Add boundary check to prevent out of
> > bound access.
> >
> > Fixes: 121354b2eceb ("iio: magnetometer: Add driver support for PNI RM3100")
> Fixes is a formal tag so needs to be part of the main tags block
> (i.e. No blank line here!)
>
> I'll fix that whilst applying.
Thank you for helping us modify the patch, I learned a lot during this
process, thanks
>
> >
> > Suggested-by: Zhouyi Zhou <zhouzhouyi@xxxxxxxxx>
> > Signed-off-by: zhili.liu <zhili.liu@xxxxxxxxxxx>
> > ---
> >  drivers/iio/magnetometer/rm3100-core.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
> > index 69938204456f..12c2e3b0aeb6 100644
> > --- a/drivers/iio/magnetometer/rm3100-core.c
> > +++ b/drivers/iio/magnetometer/rm3100-core.c
> > @@ -530,6 +530,7 @@ int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq)
> >       struct rm3100_data *data;
> >       unsigned int tmp;
> >       int ret;
> > +     int samp_rate_index;
> >
> >       indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> >       if (!indio_dev)
> > @@ -586,8 +587,14 @@ int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq)
> >       ret = regmap_read(regmap, RM3100_REG_TMRC, &tmp);
> >       if (ret < 0)
> >               return ret;
> > +
> > +     samp_rate_index = tmp - RM3100_TMRC_OFFSET;
> > +     if (samp_rate_index < 0 || samp_rate_index >=  RM3100_SAMP_NUM) {
> > +             dev_err(dev, "The value read from RM3100_REG_TMRC is invalid!\n");
> > +             return -EINVAL;
> > +     }
> >       /* Initializing max wait time, which is double conversion time. */
> > -     data->conversion_time = rm3100_samp_rates[tmp - RM3100_TMRC_OFFSET][2]
> > +     data->conversion_time = rm3100_samp_rates[samp_rate_index][2]
> >                               * 2;
> I've rewrapped this to be on one line whilst applying. Make sure to check for
> side effects like this when updating code.  If we don't tidy it up at the time
> we end up with gradually worse formatting over a long period!
Thank you for fixing that for us.
>
> Applied to the fixes-togreg branch of iio.git and marked for stable.
I saw the commit on the fixes-togreg branch of iio.git, exciting!
I learned a lot from it ;-)

Thanks again
Thanks,
Zhouyi
>
> Thanks,
>
> Jonathan
>
> >
> >       /* Cycle count values may not be what we want. */
>