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

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

[Vasileios Almpanis]

i2c_del_adapter() frees the adapter's debugfs directory before it
unregisters the adapter device, but the new_device sysfs attribute stays
writable until device_del(). A write racing with removal still reaches
i2c_device_probe(), which passes the freed adap->debugfs to
debugfs_create_dir() as the new client's parent:

  BUG: KASAN: slab-use-after-free in lookup_noperm_common+0x407/0x430
  Read of size 4 at addr ffff88803ef87810 by task syz.0.61/6090
   lookup_noperm_common+0x407/0x430
   simple_start_creating+0x9c/0x110
   debugfs_start_creating+0xdb/0x1a0
   debugfs_create_dir+0x24/0x350
   i2c_device_probe+0x814/0xbf0

Remove the directory only once the adapter device has been released.
Its refcount reaching zero means no client is left to reference it.

Fixes: 73febd775bdb ("i2c: create debugfs entry per adapter")
Reported-by: syzbot+23ad911c819b923238b7@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=23ad911c819b923238b7
Signed-off-by: Vasileios Almpanis <vasilisalmpanis@gmail.com>
---
 drivers/i2c/i2c-core-base.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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);
+
 	/* free bus id */
 	mutex_lock(&core_lock);
 	idr_remove(&i2c_adapter_idr, adap->nr);
-- 
2.47.3

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

[Andi Shyti]

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

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

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

[Vasileios Almpanis]

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