On Tue, 2023-07-18 at 14:07 +0200, Milan Zamazal wrote:
> Commit 813665564b3d ("iio: core: Convert to use firmware node handle
> instead of OF node") switched the kind of nodes to use for label
> retrieval in device registration. Probably an unwanted change in that
> commit was that if the device has no parent then NULL pointer is
> accessed. This is what happens in the stock IIO dummy driver when a
> new entry is created in configfs:
>
> # mkdir /sys/kernel/config/iio/devices/dummy/foo
> BUG: kernel NULL pointer dereference, address: 0000000000000278
> ...
> ? asm_exc_page_fault+0x22/0x30
> ? container_offline+0x20/0x20
> __iio_device_register+0x45/0xc10
> ? krealloc+0x73/0xa0
> ? iio_device_attach_buffer+0x31/0xc0
> ? iio_simple_dummy_configure_buffer+0x20/0x20
> ? iio_triggered_buffer_setup_ext+0xb4/0x100
> iio_dummy_probe+0x112/0x190
> iio_sw_device_create+0xa8/0xd0
> device_make_group+0xe/0x40
> configfs_mkdir+0x1a6/0x440
>
> Since there seems to be no reason to make a parent device of an IIO
> dummy device mandatory, let’s prevent the invalid memory access in
> __iio_device_register when the parent device is NULL. With this
> change, the IIO dummy driver works fine with configfs.
>
> Fixes: 813665564b3d ("iio: core: Convert to use firmware node handle instead
> of OF node")
> Signed-off-by: Milan Zamazal <mzamazal@xxxxxxxxxx>
> ---
LGTM (just one minor question below...)
Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> drivers/iio/industrialio-core.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index c117f50d0cf3..229527b3434a 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1888,7 +1888,7 @@ static const struct iio_buffer_setup_ops
> noop_ring_setup_ops;
> int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
> {
> struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> - struct fwnode_handle *fwnode;
> + struct fwnode_handle *fwnode = NULL;
> int ret;
>
> if (!indio_dev->info)
> @@ -1899,11 +1899,12 @@ int __iio_device_register(struct iio_dev *indio_dev,
> struct module *this_mod)
> /* If the calling driver did not initialize firmware node, do it here
> */
> if (dev_fwnode(&indio_dev->dev))
> fwnode = dev_fwnode(&indio_dev->dev);
> - else
> + else if (indio_dev->dev.parent != NULL)
Maybe this is odd enough that it could have an explicit comment referencing the
iio-dummy device? Not sure if there's any other place where this can actually
happen...
Apparently there are also some dev_err() on the parent device (even though
dev_err() handles it) but yeah, unrelated with this.
- Nuno Sá
