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

[sashiko-bot@kernel.org]

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