[PATCH] i2c: core: Fix double-free of fwnode in i2c_unregister_device()

[PATCH] i2c: core: Fix double-free of fwnode in i2c_unregister_device()

[Hans de Goede]

Before commit df6d7277e552 ("i2c: core: Do not dereference fwnode in struct
device"), i2c_unregister_device() only called fwnode_handle_put() on
of_node-s in the form of calling of_node_put(client->dev.of_node).

But after this commit the i2c_client's fwnode now unconditionally gets
fwnode_handle_put() on it.

When the i2c_client has no primary (ACPI / OF) fwnode but it does have
a software fwnode, the software-node will be the primary node and
fwnode_handle_put() will put() it.

But for the software fwnode device_remove_software_node() will also put()
it leading to a double free:

[   82.665598] ------------[ cut here ]------------
[   82.665609] refcount_t: underflow; use-after-free.
[   82.665808] WARNING: CPU: 3 PID: 1502 at lib/refcount.c:28 refcount_warn_saturate+0xba/0x11
...
[   82.666830] RIP: 0010:refcount_warn_saturate+0xba/0x110
...
[   82.666962]  <TASK>
[   82.666971]  i2c_unregister_device+0x60/0x90

Fix this by not calling fwnode_handle_put() when the primary fwnode is
a software-node.

Fixes: df6d7277e552 ("i2c: core: Do not dereference fwnode in struct device")
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/i2c/i2c-core-base.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 2ad2b1838f0f..0849aa44952d 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1066,7 +1066,13 @@ void i2c_unregister_device(struct i2c_client *client)
 		of_node_clear_flag(to_of_node(fwnode), OF_POPULATED);
 	else if (is_acpi_device_node(fwnode))
 		acpi_device_clear_enumerated(to_acpi_device_node(fwnode));
-	fwnode_handle_put(fwnode);
+
+	/*
+	 * If the primary fwnode is a software node it is free-ed by
+	 * device_remove_software_node() below, avoid double-free.
+	 */
+	if (!is_software_node(fwnode))
+		fwnode_handle_put(fwnode);
 
 	device_remove_software_node(&client->dev);
 	device_unregister(&client->dev);
-- 
2.49.0

Re: [PATCH] i2c: core: Fix double-free of fwnode in i2c_unregister_device()

[Andy Shevchenko]

On Sat, Jul 19, 2025 at 08:01:04PM +0200, Hans de Goede wrote:
> Before commit df6d7277e552 ("i2c: core: Do not dereference fwnode in struct
> device"), i2c_unregister_device() only called fwnode_handle_put() on
> of_node-s in the form of calling of_node_put(client->dev.of_node).
> 
> But after this commit the i2c_client's fwnode now unconditionally gets
> fwnode_handle_put() on it.
> 
> When the i2c_client has no primary (ACPI / OF) fwnode but it does have
> a software fwnode, the software-node will be the primary node and
> fwnode_handle_put() will put() it.
> 
> But for the software fwnode device_remove_software_node() will also put()
> it leading to a double free:
> 
> [   82.665598] ------------[ cut here ]------------
> [   82.665609] refcount_t: underflow; use-after-free.
> [   82.665808] WARNING: CPU: 3 PID: 1502 at lib/refcount.c:28 refcount_warn_saturate+0xba/0x11
> ...
> [   82.666830] RIP: 0010:refcount_warn_saturate+0xba/0x110
> ...
> [   82.666962]  <TASK>
> [   82.666971]  i2c_unregister_device+0x60/0x90
> 
> Fix this by not calling fwnode_handle_put() when the primary fwnode is
> a software-node.

Thanks for the fix!
It doesn't look elegant, but I think it's all due to the (current) design of
the fwnode linked list.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: core: Fix double-free of fwnode in i2c_unregister_device()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 729 bytes --]

On Sat, Jul 19, 2025 at 08:01:04PM +0200, Hans de Goede wrote:
> Before commit df6d7277e552 ("i2c: core: Do not dereference fwnode in struct
> device"), i2c_unregister_device() only called fwnode_handle_put() on
> of_node-s in the form of calling of_node_put(client->dev.of_node).
> 
> But after this commit the i2c_client's fwnode now unconditionally gets
> fwnode_handle_put() on it.
> 
> When the i2c_client has no primary (ACPI / OF) fwnode but it does have
> a software fwnode, the software-node will be the primary node and
> fwnode_handle_put() will put() it.
> 
> But for the software fwnode device_remove_software_node() will also put()
> it leading to a double free:
> 

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]