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

[sashiko-bot@kernel.org]

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

New issues:
- [High] A double free occurs in the error path of `i3c_master_set_info()` because `master->this` is left as a dangling pointer and is subsequently freed again in `i3c_masterdev_release()`.

Pre-existing issues:
- [High] Devres lifetime mismatch allows use-after-free of the master controller struct in `i3c_masterdev_release()`.
--

--- Patch [4]: [PATCH V3 04/14] i3c: master: Fix use-after-free of master->this ---
commit e74d33e25ba87fdc800277aa5702a984d20076ca
Author: Adrian Hunter <adrian.hunter@intel.com>
i3c: master: Fix use-after-free of master->this

This patch delays freeing master->this until i3c_masterdev_release(). By
doing this, it keeps master->this alive until all users of the master
device have gone away and the sysfs state is torn down.

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index abb582645a2ed..4839c1c186eb0 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);
>  	struct i3c_bus *bus = dev_to_i3cbus(dev);

[Severity: High]
This is a pre-existing issue, but does this release function dereference
fields in the master struct after the driver's devres cleanup has already
freed the surrounding memory?

Master controller structures are typically allocated using devm_kzalloc().
When a driver is unbound, devres cleanup frees the master controller
structure. However, if userspace holds an open file descriptor to a sysfs
attribute (like /sys/class/i3c/i3c-0/bcr), the kobject refcount doesn't drop
to zero.

When the sysfs file is later closed, the final put_device() calls this
i3c_masterdev_release() function, which then dereferences fields like
master->wq, bus->devs, and now master->this, leading to a use-after-free.

>  	if (master->wq)
>  		destroy_workqueue(master->wq);
>  
>  	WARN_ON(!list_empty(&bus->devs.i2c) || !list_empty(&bus->devs.i3c));
>  	i3c_bus_cleanup(bus);
>  
>  	fwnode_handle_put(dev->fwnode);
> +
> +	i3c_master_free_i3c_dev(master->this);

[Severity: High]
Does unconditionally freeing master->this here cause a double free on the
error path of i3c_master_set_info()?

If i3c_master_attach_i3c_dev() fails during initialization in
i3c_master_set_info(), the allocated i3cdev is freed via
i3c_master_free_i3c_dev().

However, master->this and master->bus.cur_master were already assigned to
point to i3cdev and are not set back to NULL:

drivers/i3c/master.c:i3c_master_set_info() {
	...
	master->this = i3cdev;
	master->bus.cur_master = master->this;

	ret = i3c_master_attach_i3c_dev(master, i3cdev);
	if (ret)
		goto err_free_dev;
	...
err_free_dev:
	i3c_master_free_i3c_dev(i3cdev);
	return ret;
}

The failure propagates to i3c_master_register(), which calls
put_device(&master->dev) to abort driver registration. This calls
i3c_masterdev_release(), which now unconditionally frees master->this,
leading to a double free of the dangling pointer.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804133810.184905-1-adrian.hunter@intel.com?part=4