[PATCH] i3c: master: Release I2C boardinfo node reference

[PATCH] i3c: master: Release I2C boardinfo node reference

[Ruoyu Wang]

of_i3c_master_add_i2c_boardinfo() takes a reference to each Device Tree
child node so the fwnode stored in the persistent board info remains
valid. i2c_new_client_device() later takes its own reference for the
client, and client teardown only releases that reference. The original
board-info reference is therefore leaked for every DT-described I2C
device. Registration failures after bus population leak it as well.

Register the board-info node reference as a managed resource of the I3C
master. This retains it for the full board-info lifetime and releases it
on both registration failure and normal master teardown.

This issue was found by a static analysis checker and confirmed by
manual source review.

Fixes: 3a379bbcea0af ("i3c: Add core I3C infrastructure")
Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>
---
 drivers/i3c/master.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index f1be38a640ca1..8063e2642cbda 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2619,6 +2619,11 @@ EXPORT_SYMBOL_GPL(i3c_master_do_daa);
 
 #define OF_I3C_REG1_IS_I2C_DEV			BIT(31)
 
+static void of_i3c_master_put_node(void *data)
+{
+	of_node_put(data);
+}
+
 static int
 of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
 				struct device_node *node, u32 *reg)
@@ -2648,8 +2653,12 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
 	/* LVR is encoded in reg[2]. */
 	boardinfo->lvr = reg[2];
 
+	ret = devm_add_action_or_reset(dev, of_i3c_master_put_node,
+				       of_node_get(node));
+	if (ret)
+		return ret;
+
 	list_add_tail(&boardinfo->node, &master->boardinfo.i2c);
-	of_node_get(node);
 
 	return 0;
 }
-- 
2.51.0


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH] i3c: master: Release I2C boardinfo node reference

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free and Double-Free on I3C device registration failure.
--

--- Patch [1]: [PATCH] i3c: master: Release I2C boardinfo node reference ---
commit fb94e3e139711c2723cb1a74b85c8a9c79efcfd3
Author: Ruoyu Wang <ruoyuw560@gmail.com>

i3c: master: Release I2C boardinfo node reference

The commit registers the board-info node reference as a managed resource
of the I3C master. This ensures the reference is properly retained for the
full lifetime of the board-info and released on both registration failure
and normal master teardown, preventing memory leaks.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index f1be38a640ca1..8063e2642cbda 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but while reviewing this file, I noticed a
potential use-after-free and double-free regression when handling I3C
device registration failures.

If device_register() fails in i3c_master_register_new_i3c_devs(), put_device()
is called to free desc->dev:

drivers/i3c/master.c:i3c_master_register_new_i3c_devs() {
    ...
		ret = device_register(&desc->dev->dev);
		if (ret) {
			dev_err(&master->dev,
				"Failed to add I3C device (err = %d)\n", ret);
			put_device(&desc->dev->dev);
		}
    ...
}

However, desc->dev is not set to NULL after being freed, leaving a dangling
pointer on the master's bus list.

If the master is later unregistered, i3c_master_unregister_i3c_devs() iterates
over the bus list and accesses this dangling pointer:

drivers/i3c/master.c:i3c_master_unregister_i3c_devs() {
    ...
	i3c_bus_for_each_i3cdev(&master->bus, i3cdev) {
		if (!i3cdev->dev)
			continue;

		i3cdev->dev->desc = NULL;
		if (device_is_registered(&i3cdev->dev->dev))
			device_unregister(&i3cdev->dev->dev);
		else
			put_device(&i3cdev->dev->dev);
		i3cdev->dev = NULL;
	}
    ...
}

Could this sequence lead to writing to freed memory when setting
i3cdev->dev->desc to NULL, followed by a double-free when put_device() is
called again?

Would it be safer to clear the desc->dev pointer in the error path of
i3c_master_register_new_i3c_devs() after the initial put_device() call?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814134027.1386818-1-ruoyuw560@gmail.com?part=1

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c