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

Commit Message

Fabrizio Lamarque March 27, 2023, 8:02 p.m. UTC

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.

Additional details

Kernel panic log
[    5.763067] Unable to handle kernel NULL pointer dereference at
virtual address 00000208
[...]
[    6.265076] [<c063b94c>] (driver_register) from [<c070e59c>]
(__spi_register_driver+0xd8/0xe4)
[    6.273757]  r5:c0b88c7c r4:00000000
[    6.277351] [<c070e4c4>] (__spi_register_driver) from [<c0e4c288>]
(ad7192_driver_init+0x20/0x28)
[    6.286305]  r9:c107fe00 r8:c107fe00 r7:00000000 r6:c0e56854
r5:c0e4c268 r4:c40fa000
[    6.294070] [<c0e4c268>] (ad7192_driver_init) from [<c01023d0>]
(do_one_initcall+0x58/0x2ac)
[    6.302569] [<c0102378>] (do_one_initcall) from [<c0e01594>]
(kernel_init_freeable+0x1c4/0x254)
[...]
[    6.387349] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x0000000b
[    6.395049] ---[ end Kernel panic - not syncing: Attempted to kill
init! exitcode=0x0000000b ]

The patch is against the current tree, but it applies without
modifications to 5.x (the driver has not changed much since then).
Reproduced in kernel version 5.15.x. Newer driver versions are
affected by the same issue.

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.

Signed-off-by: Fabrizio Lamarque <[e-mail]>

Comments

Nuno Sá March 29, 2023, 3:01 p.m. UTC | #1

On Mon, 2023-03-27 at 22:02 +0200, Fabrizio Lamarque 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.
> 
> Additional details
> 
> Kernel panic log
> [    5.763067] Unable to handle kernel NULL pointer dereference at
> virtual address 00000208
> [...]
> [    6.265076] [<c063b94c>] (driver_register) from [<c070e59c>]
> (__spi_register_driver+0xd8/0xe4)
> [    6.273757]  r5:c0b88c7c r4:00000000
> [    6.277351] [<c070e4c4>] (__spi_register_driver) from [<c0e4c288>]
> (ad7192_driver_init+0x20/0x28)
> [    6.286305]  r9:c107fe00 r8:c107fe00 r7:00000000 r6:c0e56854
> r5:c0e4c268 r4:c40fa000
> [    6.294070] [<c0e4c268>] (ad7192_driver_init) from [<c01023d0>]
> (do_one_initcall+0x58/0x2ac)
> [    6.302569] [<c0102378>] (do_one_initcall) from [<c0e01594>]
> (kernel_init_freeable+0x1c4/0x254)
> [...]
> [    6.387349] Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x0000000b
> [    6.395049] ---[ end Kernel panic - not syncing: Attempted to kill
> init! exitcode=0x0000000b ]
> 
> The patch is against the current tree, but it applies without
> modifications to 5.x (the driver has not changed much since then).
> Reproduced in kernel version 5.15.x. Newer driver versions are
> affected by the same issue.
> 
> 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.
> 
> Signed-off-by: Fabrizio Lamarque <[e-mail]>
> --- linux/drivers/iio/adc/ad7192.c  2023-03-13 19:32:42.646239506

Thanks for the fix. This is missing a fixes tag though... With that:

Reviewed-by: [e-mail]

Nuno Sá March 31, 2023, 6:27 a.m. UTC | #2

On Wed, 2023-03-29 at 17:01 +0200, Nuno Sá wrote:
> On Mon, 2023-03-27 at 22:02 +0200, Fabrizio Lamarque 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.
> > 
> > Additional details
> > 
> > Kernel panic log
> > [    5.763067] Unable to handle kernel NULL pointer dereference at
> > virtual address 00000208
> > [...]
> > [    6.265076] [<c063b94c>] (driver_register) from [<c070e59c>]
> > (__spi_register_driver+0xd8/0xe4)
> > [    6.273757]  r5:c0b88c7c r4:00000000
> > [    6.277351] [<c070e4c4>] (__spi_register_driver) from
> > [<c0e4c288>]
> > (ad7192_driver_init+0x20/0x28)
> > [    6.286305]  r9:c107fe00 r8:c107fe00 r7:00000000 r6:c0e56854
> > r5:c0e4c268 r4:c40fa000
> > [    6.294070] [<c0e4c268>] (ad7192_driver_init) from [<c01023d0>]
> > (do_one_initcall+0x58/0x2ac)
> > [    6.302569] [<c0102378>] (do_one_initcall) from [<c0e01594>]
> > (kernel_init_freeable+0x1c4/0x254)
> > [...]
> > [    6.387349] Kernel panic - not syncing: Attempted to kill init!
> > exitcode=0x0000000b
> > [    6.395049] ---[ end Kernel panic - not syncing: Attempted to
> > kill
> > init! exitcode=0x0000000b ]
> > 
> > The patch is against the current tree, but it applies without
> > modifications to 5.x (the driver has not changed much since then).
> > Reproduced in kernel version 5.15.x. Newer driver versions are
> > affected by the same issue.
> > 
> > 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.
> > 
> > Signed-off-by: Fabrizio Lamarque <[e-mail]>
> > --- linux/drivers/iio/adc/ad7192.c  2023-03-13 19:32:42.646239506
> 
> Thanks for the fix. This is missing a fixes tag though... With that:
> 
> Reviewed-by: [e-mail]
> 
> 

BTW, proper tag being:

Reviewed-by: Nuno Sa <[e-mail]>

Jonathan Cameron April 1, 2023, 2:28 p.m. UTC | #3

On Mon, 27 Mar 2023 22:02:48 +0200
Fabrizio Lamarque <[e-mail]> 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.
> 
> Additional details
> 
> Kernel panic log
> [    5.763067] Unable to handle kernel NULL pointer dereference at
> virtual address 00000208
> [...]
> [    6.265076] [<c063b94c>] (driver_register) from [<c070e59c>]
> (__spi_register_driver+0xd8/0xe4)
> [    6.273757]  r5:c0b88c7c r4:00000000
> [    6.277351] [<c070e4c4>] (__spi_register_driver) from [<c0e4c288>]
> (ad7192_driver_init+0x20/0x28)
> [    6.286305]  r9:c107fe00 r8:c107fe00 r7:00000000 r6:c0e56854
> r5:c0e4c268 r4:c40fa000
> [    6.294070] [<c0e4c268>] (ad7192_driver_init) from [<c01023d0>]
> (do_one_initcall+0x58/0x2ac)
> [    6.302569] [<c0102378>] (do_one_initcall) from [<c0e01594>]
> (kernel_init_freeable+0x1c4/0x254)
> [...]
> [    6.387349] Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x0000000b
> [    6.395049] ---[ end Kernel panic - not syncing: Attempted to kill
> init! exitcode=0x0000000b ]
> 
> The patch is against the current tree, but it applies without
> modifications to 5.x (the driver has not changed much since then).
> Reproduced in kernel version 5.15.x. Newer driver versions are
> affected by the same issue.
> 
> 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


> 
> Signed-off-by: Fabrizio Lamarque <[e-mail]>
> --- linux/drivers/iio/adc/ad7192.c  2023-03-13 19:32:42.646239506 +0100
> +++ linux/drivers/iio/adc/ad7192.c  2023-03-13 19:33:41.654803797 +0100
> @@ -997,7 +997,7 @@ static int ad7192_probe(struct spi_devic
>             return -ENOMEM;
> 
>       st = iio_priv(indio_dev);
> -
> +     spi_set_drvdata(spi, indio_dev);
>       mutex_init(&st->lock);
> 
>       st->avdd = devm_regulator_get(&spi->dev, "avdd");

Fabrizio Lamarque April 4, 2023, 6:57 a.m. UTC | #4

On Sat, Apr 1, 2023 at 4:13 PM Jonathan Cameron <[e-mail]> wrote:
>
> On Mon, 27 Mar 2023 22:02:48 +0200
> Fabrizio Lamarque <[e-mail]> 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 <[e-mail]>
---
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;

Jonathan Cameron April 8, 2023, 10:36 a.m. UTC | #5

On Tue, 4 Apr 2023 08:57:07 +0200
Fabrizio Lamarque <[e-mail]> wrote:

> On Sat, Apr 1, 2023 at 4:13 PM Jonathan Cameron <[e-mail]> wrote:
> >
> > On Mon, 27 Mar 2023 22:02:48 +0200
> > Fabrizio Lamarque <[e-mail]> 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 <[e-mail]>
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;
>

Patch

--- linux/drivers/iio/adc/ad7192.c  2023-03-13 19:32:42.646239506 +0100
+++ linux/drivers/iio/adc/ad7192.c  2023-03-13 19:33:41.654803797 +0100
@@ -997,7 +997,7 @@  static int ad7192_probe(struct spi_devic
            return -ENOMEM;

      st = iio_priv(indio_dev);
-
+     spi_set_drvdata(spi, indio_dev);
      mutex_init(&st->lock);

      st->avdd = devm_regulator_get(&spi->dev, "avdd");