Re: [PATCH v2 03/13] i3c: master: Use unified device property interface

sashiko-bot
2026-04-09T11:53:05Z

Sashiko AI review found 3 potential issue(s):
- [High] I3C master registration unconditionally fails on ACPI systems if child nodes are present, breaking existing ACPI support.
- [High] Failure during device registration leaves a dangling pointer in `desc->dev`, leading to a Use-After-Free and Double Free during bus teardown.
- [High] Erroneous consumption of the `boardinfo` fwnode reference during device release causes a reference underflow and potential Use-After-Free.
--

commit 97e72c86c4ba51a8b484696ee0603781ab0f1651
Author: Akhil R <akhilrajeev@nvidia.com>
Subject: i3c: master: Use unified device property interface

This commit replaces OF-specific functions with unified device property
functions to prepare for supporting both ACPI and device tree.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -995,7 +997,7 @@ static void i3c_device_release(struct device *dev)
>
> WARN_ON(i3cdev->desc);
>
> - of_node_put(i3cdev->dev.of_node);
> + fwnode_handle_put(dev->fwnode);
> kfree(i3cdev);
> }

[ ... ]

> @@ -1783,7 +1785,7 @@ i3c_master_register_new_i3c_devs(struct i3c_master_controller *master)
> desc->info.pid);
>
> if (desc->boardinfo)
> - desc->dev->dev.of_node = desc->boardinfo->of_node;
> + device_set_node(&desc->dev->dev, desc->boardinfo->fwnode);
>
> ret = device_register(&desc->dev->dev);
> if (ret) {

Will this cause a reference underflow on the boardinfo fwnode?

When the device is registered, device_set_node() assigns the fwnode without
taking an extra reference. If the device is released (for instance, in the
device_register error path shown above), i3c_device_release() calls
fwnode_handle_put(). This consumes the reference that is actually owned by
boardinfo.

If the device is later rediscovered via hot-join, it could match the same
boardinfo and use the freed fwnode, resulting in a use-after-free.

Additionally, does this error path leave a dangling pointer in desc->dev?

While this is a pre-existing issue, if device_register() fails, it calls
put_device() which triggers i3c_device_release() and frees desc->dev via
kfree(). However, desc->dev is not set to NULL. Later during teardown,
i3c_master_unregister_i3c_devs() might skip the !i3cdev->dev check, write
to the freed memory, and call put_device() again, causing a double-free.

[ ... ]

> @@ -2402,8 +2404,8 @@ EXPORT_SYMBOL_GPL(i3c_master_add_i3c_dev_locked);
> #define OF_I3C_REG1_IS_I2C_DEV BIT(31)
>
> static int
> -of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
> - struct device_node *node, u32 *reg)
> +i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
> + struct fwnode_handle *fwnode, u32 *reg)
> {
> struct i2c_dev_boardinfo *boardinfo;
> struct device *dev = &master->dev;
> @@ -2413,9 +2415,13 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
> if (!boardinfo)
> return -ENOMEM;
>
> - ret = of_i2c_get_board_info(dev, node, &boardinfo->base);
> - if (ret)
> - return ret;
> + if (is_of_node(fwnode)) {
> + ret = of_i2c_get_board_info(dev, to_of_node(fwnode), &boardinfo->base);
> + if (ret)
> + return ret;
> + } else {
> + return -EINVAL;
> + }

[ ... ]

> -static int of_populate_i3c_bus(struct i3c_master_controller *master)
> +static int fwnode_populate_i3c_bus(struct i3c_master_controller *master)
> {
> struct device *dev = &master->dev;
> - struct device_node *i3cbus_np = dev->of_node;
> + struct fwnode_handle *fwnode = dev_fwnode(dev);
> int ret;
> u32 val;
>
> - if (!i3cbus_np)
> + if (!fwnode)
> return 0;
>
> - for_each_available_child_of_node_scoped(i3cbus_np, node) {
> - ret = of_i3c_master_add_dev(master, node);
> + fwnode_for_each_available_child_node_scoped(fwnode, child) {
> + ret = i3c_master_add_dev(master, child);
> if (ret)
> return ret;
> }

Does this change break I3C master registration on ACPI systems?

Previously, of_populate_i3c_bus() would return 0 on ACPI platforms because
dev->of_node was NULL. Now, fwnode_populate_i3c_bus() unconditionally
iterates over all ACPI child nodes.

Since i3c_master_add_dev() expects a device tree style reg property (which
ACPI nodes typically lack in favor of _CRS or _ADR), reading it may fail.
Additionally, if a child is identified as an I2C device, the new
i3c_master_add_i2c_boardinfo() explicitly returns -EINVAL for non-OF nodes.

These errors will propagate up and completely abort i3c_master_register().
Even if ACPI support is fixed in a subsequent commit, could this intermediate
patch break bisectability by causing the controller to fail probing?

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260409105747.48158-1-akhilrajeev@nvidia.com?part=3