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

On Tue, Aug 04, 2026 at 10:33:05AM +0000, [email protected] wrote:
> 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?

Make sense, please make i2c/i3c handle fwnode consisent.

Frank

>
> --
> 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