Re: [PATCH 1/5] i3c: master: Release the fwnode of i2c boardinfo

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

Pre-existing issues:
- [High] The patch fixes a fwnode reference leak in `i3c_master_add_i2c_boardinfo()` but completely misses the identical leak in `i3c_master_add_i3c_boardinfo()`. Furthermore, there is a structural lifecycle flaw: `i3c_master_register_new_i3c_devs()` assigns the boardinfo's fwnode to the new I3C device without taking a reference, and `i3c_device_release()` unconditionally drops it. This means if one merely blindly copies the devm fix to `i3c_master_add_i3c_boardinfo()`, it would trigger a double-put UAF for instantiated devices. To fix the subsystem fully, both the devm action must be added and a `fwnode_handle_get()` must be taken in `i3c_master_register_new_i3c_devs()`.
--

--- Patch [1]: [PATCH 1/5] i3c: master: Release the fwnode of i2c boardinfo ---
commit 9b7e7e5dd15b80d230fc0e52c4d7b8ef9dea16c9
Author: Akhil R <[email protected]>

i3c: master: Release the fwnode of i2c boardinfo

This commit fixes a fwnode reference leak in i3c_master_add_i2c_boardinfo().
It adds a devm action to automatically drop the firmware node reference taken
for every I2C child device when the controller device is unbound.

Closes: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index f485b98805cf6..08dc10f172aab 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2913,7 +2918,8 @@ i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
>  	list_add_tail(&boardinfo->node, &master->boardinfo.i2c);
>  	fwnode_handle_get(fwnode);
>  
> -	return 0;
> +	return devm_add_action_or_reset(dev, i3c_master_put_boardinfo_fwnode,
> +					fwnode);
>  }

[Severity: High]
This is a pre-existing issue, but while fixing the leak for I2C devices, it 
looks like the identical leak was left behind in the I3C counterpart, 
i3c_master_add_i3c_boardinfo().

However, would applying the same devm_add_action_or_reset() fix there
trigger a double-put use-after-free for instantiated I3C devices?

Looking at how the I3C devices are created, i3c_master_register_new_i3c_devs()
assigns the boardinfo fwnode to the new device without taking an additional
reference:

drivers/i3c/master.c:i3c_master_register_new_i3c_devs() {
    ...
    device_set_node(&desc->dev->dev, desc->boardinfo->fwnode);
    ...
}

And when the device is unregistered, i3c_device_release() unconditionally
drops it:

drivers/i3c/master.c:i3c_device_release() {
    ...
    fwnode_handle_put(dev->fwnode);
    ...
}

If a devm action is added to i3c_master_add_i3c_boardinfo() to drop the
reference on teardown, wouldn't we also need to add a fwnode_handle_get()
in i3c_master_register_new_i3c_devs() to ensure i3c_device_release()
doesn't consume the reference the devm action expects to drop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c