[PATCH] i2c: core: move idr_remove() in i2c_del_adapter() before device_unregister()

[Tejas Mutalikdesai]

There is a race between i2c_del_adapter() and i2c_get_adapter() that
can trigger a "refcount_t: addition on 0; use-after-free" warning.

The sequence is:

  1. i2c_del_adapter() calls device_unregister(), which drops the
     device refcount to zero (the adapter's release callback fires
     and signals dev_released).

  2. The adapter is still in i2c_adapter_idr because idr_remove()
     hasn't been called yet.

  3. A concurrent i2c_get_adapter() calls idr_find() under core_lock
     and finds the adapter.  try_module_get() succeeds because the
     owning module is still MODULE_STATE_LIVE (this is a hot-remove
     path, not a module unload).

  4. get_device() is called on a kobject whose refcount is already
     zero, triggering refcount_warn_saturate() with REFCOUNT_ADD_UAF.

Fixing this by moving the idr_remove() call to before device_unregister().
Once the adapter is removed from the IDR, any concurrent
i2c_get_adapter() will get NULL from idr_find() and return -ENODEV.
Callers that already hold a device reference are unaffected:
wait_for_completion() correctly waits for them to release it via
i2c_put_adapter().

REPRODUCTION AND VALIDATION: Tested by inserting a msleep(500) after wait_for_completion() to widen
the race window, and using a kernel module that spawns two kthreads: one
calling i2c_del_adapter() and another calling i2c_get_adapter()
concurrently while the module remains MODULE_STATE_LIVE.  Without the
fix, the WARNING fires reliably.  With the fix, i2c_get_adapter()
returns NULL and no WARNING is observed.

Reported-by: syzbot+c0291c8c9aaa473c7721@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=c0291c8c9aaa473c7721
Fixes: 611e12ea0f12 ("i2c: core: manage i2c bus device refcount in i2c_[get|put]_adapter")
Signed-off-by: Tejas Mutalikdesai <tejasmutalikdesai@xxxxxxxxx>
---
 drivers/i2c/i2c-core-base.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index a2132d70fb36..780ce42fa950 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1801,6 +1801,16 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 
 	debugfs_remove_recursive(adap->debugfs);
 
+	/* Remove from IDR first so that a concurrent i2c_get_adapter() can
+	 * no longer find this adapter.  Without this ordering,
+	 * i2c_get_adapter() can find the adapter via idr_find() and call
+	 * get_device() after device_unregister() has already dropped the
+	 * refcount to zero, triggering a use-after-free warning.
+	 */
+	mutex_lock(&core_lock);
+	idr_remove(&i2c_adapter_idr, adap->nr);
+	mutex_unlock(&core_lock);
+
 	/* wait until all references to the device are gone
 	 *
 	 * FIXME: This is old code and should ideally be replaced by an
@@ -1812,11 +1822,6 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 	device_unregister(&adap->dev);
 	wait_for_completion(&adap->dev_released);
 
-	/* free bus id */
-	mutex_lock(&core_lock);
-	idr_remove(&i2c_adapter_idr, adap->nr);
-	mutex_unlock(&core_lock);
-
 	/* Clear the device structure in case this adapter is ever going to be
 	   added again */
 	memset(&adap->dev, 0, sizeof(adap->dev));
-- 
2.54.0