2023-07-19 08:44:33

by Milan Zamazal

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

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: ...
...
Call Trace:
__iio_device_register
iio_dummy_probe

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")
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Milan Zamazal <[email protected]>
---
Changes in v3:
- The call trace in the commit message reduced to the minimum lines
needed to explain the problem.

Changes in v2:
- Added a source comment about the dummy IIO device.
- Adjusted the backtrace cited in the commit message a bit.
- Replaced `... != NULL' condition with `...'.
- Dropped the unnecessary `fwnode != NULL' check (the involved calls
do the right thing when the argument is NULL).
---
drivers/iio/industrialio-core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index c117f50d0cf3..adcba832e6fa 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,7 +1899,8 @@ 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
+ /* The default dummy IIO device has no parent */
+ else if (indio_dev->dev.parent)
fwnode = dev_fwnode(indio_dev->dev.parent);
device_set_node(&indio_dev->dev, fwnode);

--
2.40.1

2023-07-19 15:58:18

by Andy Shevchenko

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

On Wed, Jul 19, 2023 at 10:32:08AM +0200, Milan Zamazal wrote:

> Changes in v3:
> - The call trace in the commit message reduced to the minimum lines
> needed to explain the problem.

Precisely, that deserves my tag which I already had given :-)
Thank you!

--
With Best Regards,
Andy Shevchenko

2023-07-22 17:16:31

by Jonathan Cameron

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

On Wed, 19 Jul 2023 18:27:45 +0300
Andy Shevchenko <[email protected]> wrote:

> On Wed, Jul 19, 2023 at 10:32:08AM +0200, Milan Zamazal wrote:
>
> > Changes in v3:
> > - The call trace in the commit message reduced to the minimum lines
> > needed to explain the problem.
>
> Precisely, that deserves my tag which I already had given :-)
> Thank you!
>
Applied to the fixes-togreg branch of iio.git.

In theory every device should have a parent, but this fix is more
minimal than adding one for the dummy device (which we should still do
- even if it's a virtual device of some type that we create just for this
purpose).

That reminds me, I was in process of fixing this for perf drivers and
forgot to finish it off :)

Jonathan