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

On 04/08/2026 17:10, [email protected] wrote:
> 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()`.

That is valid.  i3c_master_set_info() needs to be amended either to set
master->this = NULL on the error path, or just don't call
i3c_master_free_i3c_dev() since it will get cleaned up in
i3c_masterdev_release() anyway.

> 
> 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 <[email protected]>
> 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.
> 
>>  }
> 


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