i2c: dev: fix blocked adapter deregistration

The I2C subsystem allows controllers to be used by non-child devices that may remain registered after an adapter goes away.

To handle this, adapter deregistration blocks until the last reference to the adapter is released. Albeit unorthodox, this is mostly fine for the vast majority of controllers but can cause some trouble when controllers reside on hotpluggable buses.

Specifically, userspace can prevent an adapter from being deregistered indefinitely by holding an i2c-dev character device file open. And with USB attached controllers this prevents further hub events from being processed by the parent hub until the file is closed.

Fix the i2c-dev implementation by dropping the additional reference taken at open() and using an rwsem to make sure the adapter is only accessed while registered.

Note that before commit 611e12ea0f12 ("i2c: core: manage i2c bus device refcount in i2c_[get|put]_adapter") an adapter going away would instead have resulted in a use-after-free.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")

Cc [email protected]

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/i2c/i2c-dev.c | 95 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 76 insertions(+), 19 deletions(-)

drivers/i2c/i2c-dev.c +12 -0

index ccaac5e29f90..0940070c0317 100644
@@ -28,6 +28,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
+#include <linux/rwsem.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 

@@ -41,11 +42,17 @@
  */
 struct i2c_dev {
 	struct list_head list;
+	struct rw_semaphore rwsem;
 	struct i2c_adapter *adap;
 	struct device dev;
 	struct cdev cdev;
 };
 
+struct i2c_dev_data {
+	struct i2c_dev *i2c_dev;
+	struct i2c_client client;
+};
+
 #define I2C_MINORS	(MINORMASK + 1)
 static LIST_HEAD(i2c_dev_list);
 static DEFINE_SPINLOCK(i2c_dev_list_lock);

@@ -92,6 +99,11 @@ static void put_i2c_dev(struct i2c_dev *i2c_dev, bool del_cdev)
 	spin_unlock(&i2c_dev_list_lock);
 	if (del_cdev)
 		cdev_device_del(&i2c_dev->cdev, &i2c_dev->dev);
+
+	scoped_guard(rwsem_write, &i2c_dev->rwsem) {
+		i2c_dev->adap = NULL;
+	}
+
 	put_device(&i2c_dev->dev);
 }
 

@@ -134,10 +146,16 @@ ATTRIBUTE_GROUPS(i2c);
 static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,

Quoted 35 lines · Johan Hovold

The I2C subsystem allows controllers to be used by non-child devices that may remain registered after an adapter goes away.

To handle this, adapter deregistration blocks until the last reference to the adapter is released. Albeit unorthodox, this is mostly fine for the vast majority of controllers but can cause some trouble when controllers reside on hotpluggable buses.

Specifically, userspace can prevent an adapter from being deregistered indefinitely by holding an i2c-dev character device file open. And with USB attached controllers this prevents further hub events from being processed by the parent hub until the file is closed.

Fix the i2c-dev implementation by dropping the additional reference taken at open() and using an rwsem to make sure the adapter is only accessed while registered.

Note that before commit 611e12ea0f12 ("i2c: core: manage i2c bus device refcount in i2c_[get|put]_adapter") an adapter going away would instead have resulted in a use-after-free.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")

Cc [email protected]

Signed-off-by Johan Hovold <[email protected]>

---

static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,

drivers/i2c/i2c-dev.c +3 -0

 		loff_t *offset)
 {
+	struct i2c_dev_data *data = file->private_data;
+	struct i2c_client *client = &data->client;
+	struct i2c_dev *i2c_dev = data->i2c_dev;
 	char *tmp;
 	int ret;

Review

Quoted 2 lines

(quoted diff) +1 -1

-	struct i2c_client *client = file->private_data;
+	guard(rwsem_read)(&i2c_dev->rwsem);

I think sashiko is correct in its comment and you should limit the critical section to until right after i2c_master_recv(). Same elsewhere.

[1] https://sashiko.dev/#/patchset/20260827102043.673273-1-johan%40kernel.org?part=1

The I2C subsystem allows controllers to be used by non-child devices that may remain registered after an adapter goes away.

To handle this, adapter deregistration blocks until the last reference to the adapter is released. Albeit unorthodox, this is mostly fine for the vast majority of controllers but can cause some trouble when controllers reside on hotpluggable buses.

Specifically, userspace can prevent an adapter from being deregistered indefinitely by holding an i2c-dev character device file open. And with USB attached controllers this prevents further hub events from being processed by the parent hub until the file is closed.

Fix the i2c-dev implementation by dropping the additional reference taken at open() and using an rwsem to make sure the adapter is only accessed while registered.

Note that before commit 611e12ea0f12 ("i2c: core: manage i2c bus device refcount in i2c_[get|put]_adapter") an adapter going away would instead have resulted in a use-after-free.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")

Cc [email protected]

Signed-off-by Johan Hovold <[email protected]>

The sysfs lifetime rules guarantees that the i2c-dev class device is still valid while its attribute callbacks are executing so drop the unnecessary reverse lookup.

Signed-off-by Johan Hovold <[email protected]>

---

The sysfs lifetime rules guarantees that the i2c-dev class device is still valid while its attribute callbacks are executing so drop the unnecessary reverse lookup.

Signed-off-by Johan Hovold <[email protected]>

The sysfs lifetime rules guarantees that the i2c-dev class device is still valid while its attribute callbacks are executing so drop the unnecessary reverse lookup.

Signed-off-by Johan Hovold <[email protected]>

Drop the get_free_i2c_dev() and put_i2c_dev() helpers and do all setup and teardown directly in i2cdev_attach_adapter() and i2cdev_detach_adapter() for consistency and to make the logic clearer.

Note that the device list is only used at detach so the i2c-dev can be added after registering the class device.

Signed-off-by Johan Hovold <[email protected]>

---

Drop the get_free_i2c_dev() and put_i2c_dev() helpers and do all setup and teardown directly in i2cdev_attach_adapter() and i2cdev_detach_adapter() for consistency and to make the logic clearer.

Note that the device list is only used at detach so the i2c-dev can be added after registering the class device.

Signed-off-by Johan Hovold <[email protected]>

Quoted 6 lines · Johan Hovold

Drop the get_free_i2c_dev() and put_i2c_dev() helpers and do all setup and teardown directly in i2cdev_attach_adapter() and i2cdev_detach_adapter() for consistency and to make the logic clearer.

Note that the device list is only used at detach so the i2c-dev can be added after registering the class device.

Can we have the last sentence as a comment in the code, too, please?

Quoted 1 line

Signed-off-by Johan Hovold <[email protected]>

Other than that, I think this patch is an improvement. The logic is more explicit. I didn't like 'put_i2c_dev' having a bool parameter, for example.

So, only patch 1 needs a bigger update, it seems. Rest is looking good to me.

Quoted 9 lines · Wolfram Sang

Quoted 6 lines · Johan Hovold

Drop the get_free_i2c_dev() and put_i2c_dev() helpers and do all setup and teardown directly in i2cdev_attach_adapter() and i2cdev_detach_adapter() for consistency and to make the logic clearer.

Note that the device list is only used at detach so the i2c-dev can be added after registering the class device.

Can we have the last sentence as a comment in the code, too, please?

Sure.

Quoted 8 lines

Quoted 1 line

Signed-off-by Johan Hovold <[email protected]>

Other than that, I think this patch is an improvement. The logic is more explicit. I didn't like 'put_i2c_dev' having a bool parameter, for example.

So, only patch 1 needs a bigger update, it seems. Rest is looking good to me.

I had rejected that Sashiko finding as being too outlandish for us to have to worry about. As I just in a reply to Bartosz:

If a malicious actor has physical access to the machine, we have bigger problems than worrying about them going out of their way to cause a USB hub worker thread to hang. :)

Limiting the critical section can possibly be done as a follow-up, but it may not even be worth the effort doing that.

Quoted 10 lines · Johan Hovold

I had rejected that Sashiko finding as being too outlandish for us to have to worry about. As I just in a reply to Bartosz:

If a malicious actor has physical access to the machine, we have bigger problems than worrying about them going out of their way to cause a USB hub worker thread to hang. :)

Limiting the critical section can possibly be done as a follow-up, but it may not even be worth the effort doing that.

Why is limiting the scope of the semaphore such a big deal though? It's not a big change, literally just use scoped_guard() instead. Why would we commit incorrect code if we can correct it in a v3 with not much effort? You're fixing a use-case where user-space can hang a kernel thread, why not do it for corner-cases too?

If Wolfram insists, I can revisit this, but I don't think it's needed.

Drop the get_free_i2c_dev() and put_i2c_dev() helpers and do all setup and teardown directly in i2cdev_attach_adapter() and i2cdev_detach_adapter() for consistency and to make the logic clearer.

Note that the device list is only used at detach so the i2c-dev can be added after registering the class device.

Signed-off-by Johan Hovold <[email protected]>

As part of addressing I2C lifetime issues, this series fixes an issue which may actually matter to some people, at least if they are playing around with USB attached I2C controllers, and whose fix does not depend on any larger rework.

This could possibly go into 7.3, but can also wait for 7.4 as the impact of the issue (both in terms of number of affected users and symptoms) should be limited.

Quoted 8 lines · Johan Hovold

As part of addressing I2C lifetime issues, this series fixes an issue which may actually matter to some people, at least if they are playing around with USB attached I2C controllers, and whose fix does not depend on any larger rework.

This could possibly go into 7.3, but can also wait for 7.4 as the impact of the issue (both in terms of number of affected users and symptoms) should be limited.

Oh wow, I completely missed this for some reason :( Just discovered it while digging around in patchwork.

I will try to find some time soon, so we can have it at least in 7.5.

Adding Bart to CC, maybe he is interested.