Re: [PATCH V4 04/14] i3c: master: Fix use-after-free of master->this

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

Pre-existing issues:
- [High] Delayed device release triggers Use-After-Free of devm-allocated master structure.
- [High] If `idr_alloc()` fails in `i3c_bus_init()`, the error path will erroneously remove ID 0 from `i3c_bus_idr`, corrupting the state of another successfully registered I3C master.
--

--- Patch [4]: [PATCH V4 04/14] i3c: master: Fix use-after-free of master->this ---
commit 327e5dd17c108564121fb788d79fe1ed7a32a2b4
Author: Adrian Hunter <[email protected]>

i3c: master: Fix use-after-free of master->this

This commit aims to fix a use-after-free by keeping master->this alive until
i3c_masterdev_release() is called. It moves the freeing of master->this out
of the normal device detach path.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index abb582645a2ed..2357874bb9d6a 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -842,6 +842,11 @@ static struct attribute *i3c_masterdev_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(i3c_masterdev);
>  
> +static void i3c_master_free_i3c_dev(struct i3c_dev_desc *dev)
> +{
> +	kfree(dev);
> +}
> +
>  static void i3c_masterdev_release(struct device *dev)
>  {
>  	struct i3c_master_controller *master = dev_to_i3cmaster(dev);
> @@ -854,6 +859,8 @@ static void i3c_masterdev_release(struct device *dev)
>  	i3c_bus_cleanup(bus);

[Severity: High]
This is a pre-existing issue, but can this unconditional call to
i3c_bus_cleanup() corrupt the ID pool if idr_alloc() fails during probe?

If idr_alloc() fails in i3c_bus_init(), the error path in
i3c_master_register() will call put_device(&master->dev), which leads to this
release function.

Because the bus ID was not assigned on failure, i3cbus->id remains at its
default value of 0. Calling i3c_bus_cleanup() will then unconditionally call
idr_remove(&i3c_bus_idr, 0), removing ID 0 which might currently be in use
by another successfully registered I3C master.

>  	fwnode_handle_put(dev->fwnode);
> +
> +	i3c_master_free_i3c_dev(master->this);

[Severity: High]
This isn't a bug introduced by this patch, but does accessing master->this
here risk a use-after-free if the master structure was allocated with
devres?

Many I3C master drivers allocate their controller structures using
devm_kzalloc() and tie the memory lifetime to the platform device. If a
userspace process holds a sysfs attribute open for the master device, the
kobject refcount remains elevated.

When the master device is unbound (for example, if the module is unloaded),
devres will free the master structure before the final kobject reference is
dropped. When the sysfs file is eventually closed, this release callback
will execute, but the master structure it dereferences (for master->this and
master->wq) will have already been freed.

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