Re: [PATCH] i2c: core: fix debugfs UAF on adapter removal

On 7/31/26 11:35 PM, Andi Shyti wrote:
Hi Andi,
> Hi Vasileios,
>
> ...
>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 3ec04787a737..b894563f5a75 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -1826,8 +1826,6 @@ void i2c_del_adapter(struct i2c_adapter *adap)
>>   
>>   	i2c_host_notify_irq_teardown(adap);
>>   
>> -	debugfs_remove_recursive(adap->debugfs);
>> -
>>   	/* wait until all references to the device are gone
>>   	 *
>>   	 * FIXME: This is old code and should ideally be replaced by an
>> @@ -1839,6 +1837,9 @@ void i2c_del_adapter(struct i2c_adapter *adap)
>>   	device_unregister(&adap->dev);
>>   	wait_for_completion(&adap->dev_released);
>>   
>> +	/* clients use this directory as their debugfs parent */
>> +	debugfs_remove_recursive(adap->debugfs);
>> +
> Speaking of sysfs, this can't work if a new device is created
> through the new_device interface. Perhaps you can remove the
> attribute first with device_remove_file(), but we need to check
> whether that could lead to a double removal when the device
> attributes are cleaned up during device removal.
>
> Thanks,
> Andi
Thanks you for reviewing my patch. You're right. A write to new_device
that happens during removing clients leaks a client. It holds a reference
on the adapter device, so wait_for_completion() never returns.
I reproduced this locally by adding mdelay(5000) right after 
unregistering clients
and creating a client. The write succeeds and i2c_del_adapter() hangs.

I already wrote another patch that includes my hunk from v1 but also adds
device_remove_file(&adap->dev, &dev_attr_new_device) before deregistering
clients as you suggested. This no longer reproduces the same bug.
I also tested it with CONFIG_DEBUG_KOBJECT_RELEASE and KASAN as
i2c_del_adapter suggests and no warnings were emitted.

syzbot also tested the result against mainline and the reproducer
doesn't trigger anymore.

Is it okay with you if I keep the hunk of v1? Or you prefer its left as 
originally,
since after removing the new_device attr, we can't have new clients so
leaving there is safe.

Thanks,
Vasilis
>
>>   	/* free bus id */
>>   	mutex_lock(&core_lock);
>>   	idr_remove(&i2c_adapter_idr, adap->nr);
>> -- 
>> 2.47.3
>>