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

[Jonathan Cameron <jic23@kernel.org>]

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.
> 
> 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 <fl.scratchpad@gmail.com>
> --- 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");