Re: [PATCH 1/2] ad7192 driver: fix null pointer dereference in probe when populating adc input ranges

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 4 Apr 2023 08:57:07 +0200
Fabrizio Lamarque <fl.scratchpad@gmail.com> wrote:

> On Sat, Apr 1, 2023 at 4:13 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 27 Mar 2023 22:02:48 +0200
> > Fabrizio Lamarque <fl.scratchpad@gmail.com> wrote:
> >  
> > > Fix ad7192.c NULL pointer dereference in ad7192_setup() when accessing
> > > indio_dev structure while populating input rages, causing a kernel
> > > panic.
> > > Fixed by calling spi_set_drvdata after indio_dev is allocated.
> > >
> > > Pointer to indio_dev structure is obtained via spi_get_drvdata() at
> > > the beginning of function ad7192_setup(), but the
> > > spi->dev->driver_data member is not initialized here, hence a NULL
> > > pointer is returned.
> > >
> > > By comparing every other iio adc driver, whenever there is a call to
> > > spi_get_drvdata() there is also one to spi_set_drvdata() within probe
> > > function.
> > > It should also be noted that the indio_dev structure is accessed just
> > > to get the number of bits for the converter, and no other driver calls
> > > spi_get_drvdata within probe.
> > > After the patch is applied the system boots correctly and the ADC is
> > > mapped within sysfs.  
> >
> > I'd prefer to fix this by changing the ad7192_setup() to take the
> > struct iio_dev (available at it's call site) and avoid the dance
> > that is currently going on entirely.
> > Drop the struct ad7192_state *st parameter and get that via
> > st = iio_priv(indio_dev);
> >
> > Thanks,
> >
> > Jonathan
> >  
> 
> Fix NULL pointer dereference in ad7192_setup() (ad7192.c) when accessing
> indio_dev structure while populating input rages, causing a kernel panic.
> 
> Changed ad7192_setup() signature to take pointer to struct
> iio_dev, and got ad7192_state pointer via st = iio_priv(indio_dev);
> 
> Fixes: bd5dcdeb3fd0 iio: adc: ad7192: convert to device-managed functions
> Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>
Looks good. If you haven't already (I'm behind with emails)
please send this out as a full patch etc so it gets correctly picked up
by patchwork / b4 etc.

Thanks

Jonathan

> ---
> V1 -> Revised after suggestions from Jonathan, removed Reviewed-by
>  since the entire patch changed its content.
> 
>  drivers/iio/adc/ad7192.c | 6 +++---
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -380,9 +380,9 @@ static int ad7192_of_clock_select(struct ad7192_state *st)
>      return clock_sel;
>  }
> 
> -static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
> +static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np)
>  {
> -    struct iio_dev *indio_dev = spi_get_drvdata(st->sd.spi);
> +    struct ad7192_state *st = iio_priv(indio_dev);
>      bool rej60_en, refin2_en;
>      bool buf_en, bipolar, burnout_curr_en;
>      unsigned long long scale_uv;
> @@ -1073,7 +1073,7 @@ static int ad7192_probe(struct spi_device *spi)
>          }
>      }
> 
> -    ret = ad7192_setup(st, spi->dev.of_node);
> +    ret = ad7192_setup(indio_dev, spi->dev.of_node);
>      if (ret)
>          return ret;
>