| Message ID | 20230718120700.132579-1-mzamazal@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: core: Prevent invalid memory access when there is no parent | expand |
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@redhat.com> > --- LGTM (just one minor question below...) Reviewed-by: Nuno Sa <nuno.sa@analog.com> > 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á
Nuno Sá <noname.nuno@gmail.com> writes: > 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@redhat.com> >> --- > > LGTM (just one minor question below...) > > Reviewed-by: Nuno Sa <nuno.sa@analog.com> > >> 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? Good idea, also to not forget about iio-dummy generally. I'll add it. > 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á
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.
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) fwnode = dev_fwnode(indio_dev->dev.parent); - 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); + } ret = iio_check_unique_scan_index(indio_dev); if (ret < 0)
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@redhat.com> --- drivers/iio/industrialio-core.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)