Re: [PATCH] iio: core: Prevent invalid memory access when there is no parent

[Andy Shevchenko]

On Tue, Jul 18, 2023 at 02:07:00PM +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

Read this
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
and amend the commit message accordingly.

> 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.

...

>  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)

>  	/* 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)
>  		fwnode = dev_fwnode(indio_dev->dev.parent);

This part of the change is okay with dropped " != NULL".

> -	device_set_node(&indio_dev->dev, fwnode);
> -
> -	fwnode_property_read_string(fwnode, "label", &indio_dev->label);
> +	if (fwnode != NULL) {
> +		device_set_node(&indio_dev->dev, fwnode);
> +		fwnode_property_read_string(fwnode, "label", &indio_dev->label);
> +	}

This part of the change is not needed.

-- 
With Best Regards,
Andy Shevchenko