iio: core: Fix double free.

PATCH RESEND In review Martin Fuzzey 2 participants 3 messages Feb 19, 2015 – Mar 14, 2015 IIO

Share & tools

Martin Fuzzey

author

2015-02-19 14:17 UTC #0 lore raw

PATCH RESEND 1/1 iio: core: Fix double free.

When an error occurred during event registration memory was freed twice resulting in kernel memory corruption and a crash in unrelated code.

The problem was caused by
iio_device_unregister_eventset()
iio_device_unregister_sysfs()

being called twice, once on the error path and then again via iio_dev_release().

Fix this by making these two functions idempotent so they may be called multiple times.

The problem was observed before applying
78b33216 iio:core: Handle error when mask type is not separate

Signed-off-by Martin Fuzzey <[email protected]>

---
 drivers/iio/industrialio-core.c  |    5 +++--
 drivers/iio/industrialio-event.c |    1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

drivers/iio/industrialio-core.c +3 -2

index af3e76d..f009d05 100644
@@ -832,8 +832,7 @@ static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev,
  * @attr_list: List of IIO device attributes
  *
  * This function frees the memory allocated for each of the IIO device
- * attributes in the list. Note: if you want to reuse the list after calling
- * this function you have to reinitialize it using INIT_LIST_HEAD().
+ * attributes in the list.
  */
 void iio_free_chan_devattr_list(struct list_head *attr_list)
 {

@@ -841,6 +840,7 @@ void iio_free_chan_devattr_list(struct list_head *attr_list)
 
 	list_for_each_entry_safe(p, n, attr_list, l) {
 		kfree(p->dev_attr.attr.name);
+		list_del(&p->l);
 		kfree(p);
 	}
 }

@@ -921,6 +921,7 @@ static void iio_device_unregister_sysfs(struct iio_dev *indio_dev)
 
 	iio_free_chan_devattr_list(&indio_dev->channel_attr_list);
 	kfree(indio_dev->chan_attr_group.attrs);
+	indio_dev->chan_attr_group.attrs = NULL;
 }
 
 static void iio_dev_release(struct device *device)

drivers/iio/industrialio-event.c +1 -0

index 0c1e37e..35c02ae 100644
@@ -493,6 +493,7 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
 error_free_setup_event_lines:
 	iio_free_chan_devattr_list(&indio_dev->event_interface->dev_attr_list);
 	kfree(indio_dev->event_interface);
+	indio_dev->event_interface = NULL;
 	return ret;
 }

Jonathan Cameron

maintainer

2015-02-21 18:53 UTC #1 lore raw

Quoted 10 lines · Martin Fuzzey

When an error occurred during event registration memory was freed twice resulting in kernel memory corruption and a crash in unrelated code.
The problem was caused by
iio_device_unregister_eventset()
iio_device_unregister_sysfs()
being called twice, once on the error path and then again via iio_dev_release().

Quoted 7 lines

Fix this by making these two functions idempotent so they may be called multiple times.
The problem was observed before applying
78b33216 iio:core: Handle error when mask type is not separate
Signed-off-by Martin Fuzzey <[email protected]>

Good find. I'm trying to get my head around why those functions are in the release rather than the unregister. Need to think on this a little more to work out if this is the right fix or is papering over a nastier issue.

Jonathan

Quoted 48 lines

---
 drivers/iio/industrialio-core.c  |    5 +++--
 drivers/iio/industrialio-event.c |    1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

drivers/iio/industrialio-core.c +3 -2

index af3e76d..f009d05 100644
@@ -832,8 +832,7 @@ static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev,
  * @attr_list: List of IIO device attributes
  *
  * This function frees the memory allocated for each of the IIO device
- * attributes in the list. Note: if you want to reuse the list after calling
- * this function you have to reinitialize it using INIT_LIST_HEAD().
+ * attributes in the list.
  */
 void iio_free_chan_devattr_list(struct list_head *attr_list)
 {

@@ -841,6 +840,7 @@ void iio_free_chan_devattr_list(struct list_head *attr_list)
 
 	list_for_each_entry_safe(p, n, attr_list, l) {
 		kfree(p->dev_attr.attr.name);
+		list_del(&p->l);
 		kfree(p);
 	}
 }

@@ -921,6 +921,7 @@ static void iio_device_unregister_sysfs(struct iio_dev *indio_dev)
 
 	iio_free_chan_devattr_list(&indio_dev->channel_attr_list);
 	kfree(indio_dev->chan_attr_group.attrs);
+	indio_dev->chan_attr_group.attrs = NULL;
 }
 
 static void iio_dev_release(struct device *device)

drivers/iio/industrialio-event.c +1 -0

index 0c1e37e..35c02ae 100644
@@ -493,6 +493,7 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
 error_free_setup_event_lines:
 	iio_free_chan_devattr_list(&indio_dev->event_interface->dev_attr_list);
 	kfree(indio_dev->event_interface);
+	indio_dev->event_interface = NULL;
 	return ret;
 }

Jonathan Cameron

maintainer

2015-03-14 18:54 UTC #2 lore raw

In reply to Martin Fuzzey

Quoted 10 lines · Martin Fuzzey

When an error occurred during event registration memory was freed twice resulting in kernel memory corruption and a crash in unrelated code.
The problem was caused by
iio_device_unregister_eventset()
iio_device_unregister_sysfs()
being called twice, once on the error path and then again via iio_dev_release().

Quoted 7 lines

Fix this by making these two functions idempotent so they may be called multiple times.
The problem was observed before applying
78b33216 iio:core: Handle error when mask type is not separate
Signed-off-by Martin Fuzzey <[email protected]>

I was hoping to get some time to look at better ways of fixing this, but what you have here is a definite improvement so we'll take this for now and kick working the best fix out into the long grass.

The whole balance of what is in the release function here vs isn't is wrong here.

Jonathan

Quoted 48 lines

---
 drivers/iio/industrialio-core.c  |    5 +++--
 drivers/iio/industrialio-event.c |    1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

drivers/iio/industrialio-core.c +3 -2

index af3e76d..f009d05 100644
@@ -832,8 +832,7 @@ static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev,
  * @attr_list: List of IIO device attributes
  *
  * This function frees the memory allocated for each of the IIO device
- * attributes in the list. Note: if you want to reuse the list after calling
- * this function you have to reinitialize it using INIT_LIST_HEAD().
+ * attributes in the list.
  */
 void iio_free_chan_devattr_list(struct list_head *attr_list)
 {

@@ -841,6 +840,7 @@ void iio_free_chan_devattr_list(struct list_head *attr_list)
 
 	list_for_each_entry_safe(p, n, attr_list, l) {
 		kfree(p->dev_attr.attr.name);
+		list_del(&p->l);
 		kfree(p);
 	}
 }

@@ -921,6 +921,7 @@ static void iio_device_unregister_sysfs(struct iio_dev *indio_dev)
 
 	iio_free_chan_devattr_list(&indio_dev->channel_attr_list);
 	kfree(indio_dev->chan_attr_group.attrs);
+	indio_dev->chan_attr_group.attrs = NULL;
 }
 
 static void iio_dev_release(struct device *device)

drivers/iio/industrialio-event.c +1 -0

index 0c1e37e..35c02ae 100644
@@ -493,6 +493,7 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
 error_free_setup_event_lines:
 	iio_free_chan_devattr_list(&indio_dev->event_interface->dev_attr_list);
 	kfree(indio_dev->event_interface);
+	indio_dev->event_interface = NULL;
 	return ret;
 }