[PATCH -next] bonding: Fix a use-after-free problem when bond_sysfs_slave_add() failed

[PATCH -next] bonding: Fix a use-after-free problem when bond_sysfs_slave_add() failed

[Huang Guobin]

When I do fuzz test for bonding device interface, I got the following
use-after-free Calltrace:

==================================================================
BUG: KASAN: use-after-free in bond_enslave+0x1521/0x24f0
Read of size 8 at addr ffff88825bc11c00 by task ifenslave/7365

CPU: 5 PID: 7365 Comm: ifenslave Tainted: G            E     5.15.0-rc1+ #13
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1 04/01/2014
Call Trace:
 dump_stack_lvl+0x6c/0x8b
 print_address_description.constprop.0+0x48/0x70
 kasan_report.cold+0x82/0xdb
 __asan_load8+0x69/0x90
 bond_enslave+0x1521/0x24f0
 bond_do_ioctl+0x3e0/0x450
 dev_ifsioc+0x2ba/0x970
 dev_ioctl+0x112/0x710
 sock_do_ioctl+0x118/0x1b0
 sock_ioctl+0x2e0/0x490
 __x64_sys_ioctl+0x118/0x150
 do_syscall_64+0x35/0xb0
 entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f19159cf577
Code: b3 66 90 48 8b 05 11 89 2c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 78
RSP: 002b:00007ffeb3083c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007ffeb3084bca RCX: 00007f19159cf577
RDX: 00007ffeb3083ce0 RSI: 0000000000008990 RDI: 0000000000000003
RBP: 00007ffeb3084bc4 R08: 0000000000000040 R09: 0000000000000000
R10: 00007ffeb3084bc0 R11: 0000000000000246 R12: 00007ffeb3083ce0
R13: 0000000000000000 R14: 0000000000000000 R15: 00007ffeb3083cb0

Allocated by task 7365:
 kasan_save_stack+0x23/0x50
 __kasan_kmalloc+0x83/0xa0
 kmem_cache_alloc_trace+0x22e/0x470
 bond_enslave+0x2e1/0x24f0
 bond_do_ioctl+0x3e0/0x450
 dev_ifsioc+0x2ba/0x970
 dev_ioctl+0x112/0x710
 sock_do_ioctl+0x118/0x1b0
 sock_ioctl+0x2e0/0x490
 __x64_sys_ioctl+0x118/0x150
 do_syscall_64+0x35/0xb0
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Freed by task 7365:
 kasan_save_stack+0x23/0x50
 kasan_set_track+0x20/0x30
 kasan_set_free_info+0x24/0x40
 __kasan_slab_free+0xf2/0x130
 kfree+0xd1/0x5c0
 slave_kobj_release+0x61/0x90
 kobject_put+0x102/0x180
 bond_sysfs_slave_add+0x7a/0xa0
 bond_enslave+0x11b6/0x24f0
 bond_do_ioctl+0x3e0/0x450
 dev_ifsioc+0x2ba/0x970
 dev_ioctl+0x112/0x710
 sock_do_ioctl+0x118/0x1b0
 sock_ioctl+0x2e0/0x490
 __x64_sys_ioctl+0x118/0x150
 do_syscall_64+0x35/0xb0
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Last potentially related work creation:
 kasan_save_stack+0x23/0x50
 kasan_record_aux_stack+0xb7/0xd0
 insert_work+0x43/0x190
 __queue_work+0x2e3/0x970
 delayed_work_timer_fn+0x3e/0x50
 call_timer_fn+0x148/0x470
 run_timer_softirq+0x8a8/0xc50
 __do_softirq+0x107/0x55f

Second to last potentially related work creation:
 kasan_save_stack+0x23/0x50
 kasan_record_aux_stack+0xb7/0xd0
 insert_work+0x43/0x190
 __queue_work+0x2e3/0x970
 __queue_delayed_work+0x130/0x180
 queue_delayed_work_on+0xa7/0xb0
 bond_enslave+0xe25/0x24f0
 bond_do_ioctl+0x3e0/0x450
 dev_ifsioc+0x2ba/0x970
 dev_ioctl+0x112/0x710
 sock_do_ioctl+0x118/0x1b0
 sock_ioctl+0x2e0/0x490
 __x64_sys_ioctl+0x118/0x150
 do_syscall_64+0x35/0xb0
 entry_SYSCALL_64_after_hwframe+0x44/0xae

The buggy address belongs to the object at ffff88825bc11c00
 which belongs to the cache kmalloc-1k of size 1024
The buggy address is located 0 bytes inside of
 1024-byte region [ffff88825bc11c00, ffff88825bc12000)
The buggy address belongs to the page:
page:ffffea00096f0400 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x25bc10
head:ffffea00096f0400 order:3 compound_mapcount:0 compound_pincount:0
flags: 0x57ff00000010200(slab|head|node=1|zone=2|lastcpupid=0x7ff)
raw: 057ff00000010200 ffffea0009a71c08 ffff888240001968 ffff88810004dbc0
raw: 0000000000000000 00000000000a000a 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff88825bc11b00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88825bc11b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff88825bc11c00: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                   ^
 ffff88825bc11c80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff88825bc11d00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

Put new_slave in bond_sysfs_slave_add() will cause use-after-free problems
when new_slave is accessed in the subsequent error handling process. Since
new_slave will be put in the subsequent error handling process, remove the
unnecessary put to fix it.
In addition, when sysfs_create_file() fails, if some files have been crea-
ted successfully, we need to call sysfs_remove_file() to remove them.

Fixes: 7afcaec49696 (bonding: use kobject_put instead of _del after kobject_add)
Signed-off-by: Huang Guobin <huangguobin4@huawei.com>
---
 drivers/net/bonding/bond_sysfs_slave.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_sysfs_slave.c b/drivers/net/bonding/bond_sysfs_slave.c
index fd07561..d1a5b3f 100644
--- a/drivers/net/bonding/bond_sysfs_slave.c
+++ b/drivers/net/bonding/bond_sysfs_slave.c
@@ -137,18 +137,23 @@ static ssize_t slave_show(struct kobject *kobj,
 
 int bond_sysfs_slave_add(struct slave *slave)
 {
-	const struct slave_attribute **a;
+	const struct slave_attribute **a, **b;
 	int err;
 
 	for (a = slave_attrs; *a; ++a) {
 		err = sysfs_create_file(&slave->kobj, &((*a)->attr));
 		if (err) {
-			kobject_put(&slave->kobj);
-			return err;
+			goto err_remove_file;
 		}
 	}
 
 	return 0;
+
+err_remove_file:
+	for (b = slave_attrs; b < a; ++b)
+		sysfs_remove_file(&slave->kobj, &((*b)->attr));
+
+	return err;
 }
 
 void bond_sysfs_slave_del(struct slave *slave)
-- 
1.8.3.1

Re: [PATCH -next] bonding: Fix a use-after-free problem when bond_sysfs_slave_add() failed

[Julian Wiedmann]

On 01.11.21 15:34, Huang Guobin wrote:
> When I do fuzz test for bonding device interface, I got the following
> use-after-free Calltrace:
> 

[...]

> Fixes: 7afcaec49696 (bonding: use kobject_put instead of _del after kobject_add)
> Signed-off-by: Huang Guobin <huangguobin4@huawei.com>
> ---
>  drivers/net/bonding/bond_sysfs_slave.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_sysfs_slave.c b/drivers/net/bonding/bond_sysfs_slave.c
> index fd07561..d1a5b3f 100644
> --- a/drivers/net/bonding/bond_sysfs_slave.c
> +++ b/drivers/net/bonding/bond_sysfs_slave.c
> @@ -137,18 +137,23 @@ static ssize_t slave_show(struct kobject *kobj,
>  
>  int bond_sysfs_slave_add(struct slave *slave)
>  {
> -	const struct slave_attribute **a;
> +	const struct slave_attribute **a, **b;
>  	int err;
>  
>  	for (a = slave_attrs; *a; ++a) {
>  		err = sysfs_create_file(&slave->kobj, &((*a)->attr));
>  		if (err) {
> -			kobject_put(&slave->kobj);
> -			return err;
> +			goto err_remove_file;
>  		}
>  	}
>  
>  	return 0;
> +
> +err_remove_file:
> +	for (b = slave_attrs; b < a; ++b)
> +		sysfs_remove_file(&slave->kobj, &((*b)->attr));
> +
> +	return err;
>  }
>  

This looks like a candidate for sysfs_create_files(), no?

>  void bond_sysfs_slave_del(struct slave *slave)
> 

RE: [PATCH -next] bonding: Fix a use-after-free problem when bond_sysfs_slave_add() failed

[huangguobin]

I think bond_sysfs_slave_del should not be used in the error handling process, because bond_sysfs_slave_del will traverse all slave_attrs and release them. When sysfs_create_file fails, only some attributes may be created successfully.
-----Original Message-----
From: Julian Wiedmann [mailto:jwi@linux.ibm.com] 
Sent: Tuesday, November 2, 2021 3:31 AM
To: huangguobin <huangguobin4@huawei.com>; j.vosburgh@gmail.com; vfalico@gmail.com; andy@greyhouse.net; davem@davemloft.net; kuba@kernel.org
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH -next] bonding: Fix a use-after-free problem when bond_sysfs_slave_add() failed

On 01.11.21 15:34, Huang Guobin wrote:
> When I do fuzz test for bonding device interface, I got the following 
> use-after-free Calltrace:
> 

[...]

> Fixes: 7afcaec49696 (bonding: use kobject_put instead of _del after 
> kobject_add)
> Signed-off-by: Huang Guobin <huangguobin4@huawei.com>
> ---
>  drivers/net/bonding/bond_sysfs_slave.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_sysfs_slave.c 
> b/drivers/net/bonding/bond_sysfs_slave.c
> index fd07561..d1a5b3f 100644
> --- a/drivers/net/bonding/bond_sysfs_slave.c
> +++ b/drivers/net/bonding/bond_sysfs_slave.c
> @@ -137,18 +137,23 @@ static ssize_t slave_show(struct kobject *kobj,
>  
>  int bond_sysfs_slave_add(struct slave *slave)  {
> -	const struct slave_attribute **a;
> +	const struct slave_attribute **a, **b;
>  	int err;
>  
>  	for (a = slave_attrs; *a; ++a) {
>  		err = sysfs_create_file(&slave->kobj, &((*a)->attr));
>  		if (err) {
> -			kobject_put(&slave->kobj);
> -			return err;
> +			goto err_remove_file;
>  		}
>  	}
>  
>  	return 0;
> +
> +err_remove_file:
> +	for (b = slave_attrs; b < a; ++b)
> +		sysfs_remove_file(&slave->kobj, &((*b)->attr));
> +
> +	return err;
>  }
>  

This looks like a candidate for sysfs_create_files(), no?

>  void bond_sysfs_slave_del(struct slave *slave)
> 

Re: [PATCH -next] bonding: Fix a use-after-free problem when bond_sysfs_slave_add() failed

[Julian Wiedmann]

On 02.11.21 03:55, huangguobin wrote:
> I think bond_sysfs_slave_del should not be used in the error handling process, because bond_sysfs_slave_del will traverse all slave_attrs and release them. When sysfs_create_file fails, only some attributes may be created successfully.

[please don't top-post]

The suggestion was to use sysfs_create_files(), which would handle such rollback
internally. There was no mention of using bond_sysfs_slave_del().

ie. something like the following (untested):

diff --git a/drivers/net/bonding/bond_sysfs_slave.c b/drivers/net/bonding/bond_sysfs_slave.c
index fd07561da034..a1fd4bc0b0d2 100644
--- a/drivers/net/bonding/bond_sysfs_slave.c
+++ b/drivers/net/bonding/bond_sysfs_slave.c
@@ -108,15 +108,15 @@ static ssize_t ad_partner_oper_port_state_show(struct slave *slave, char *buf)
 }
 static SLAVE_ATTR_RO(ad_partner_oper_port_state);
 
-static const struct slave_attribute *slave_attrs[] = {
-       &slave_attr_state,
-       &slave_attr_mii_status,
-       &slave_attr_link_failure_count,
-       &slave_attr_perm_hwaddr,
-       &slave_attr_queue_id,
-       &slave_attr_ad_aggregator_id,
-       &slave_attr_ad_actor_oper_port_state,
-       &slave_attr_ad_partner_oper_port_state,
+static const struct attribute *slave_attrs[] = {
+       &slave_attr_state.attr,
+       &slave_attr_mii_status.attr,
+       &slave_attr_link_failure_count.attr,
+       &slave_attr_perm_hwaddr.attr,
+       &slave_attr_queue_id.attr,
+       &slave_attr_ad_aggregator_id.attr,
+       &slave_attr_ad_actor_oper_port_state.attr,
+       &slave_attr_ad_partner_oper_port_state.attr,
        NULL
 };
 
@@ -137,24 +137,16 @@ const struct sysfs_ops slave_sysfs_ops = {
 
 int bond_sysfs_slave_add(struct slave *slave)
 {
-       const struct slave_attribute **a;
        int err;
 
-       for (a = slave_attrs; *a; ++a) {
-               err = sysfs_create_file(&slave->kobj, &((*a)->attr));
-               if (err) {
-                       kobject_put(&slave->kobj);
-                       return err;
-               }
-       }
+       err = sysfs_create_files(&slave->kobj, slave_attrs);
+       if (err)
+               kobject_put(&slave->kobj);
 
-       return 0;
+       return err;
 }
 
 void bond_sysfs_slave_del(struct slave *slave)
 {
-       const struct slave_attribute **a;
-
-       for (a = slave_attrs; *a; ++a)
-               sysfs_remove_file(&slave->kobj, &((*a)->attr));
+       sysfs_remove_files(&slave->kobj, slave_attrs);
 }


> -----Original Message-----
> From: Julian Wiedmann [mailto:jwi@linux.ibm.com] 
> Sent: Tuesday, November 2, 2021 3:31 AM
> To: huangguobin <huangguobin4@huawei.com>; j.vosburgh@gmail.com; vfalico@gmail.com; andy@greyhouse.net; davem@davemloft.net; kuba@kernel.org
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH -next] bonding: Fix a use-after-free problem when bond_sysfs_slave_add() failed
> 
> On 01.11.21 15:34, Huang Guobin wrote:
>> When I do fuzz test for bonding device interface, I got the following 
>> use-after-free Calltrace:
>>
> 
> [...]
> 
>> Fixes: 7afcaec49696 (bonding: use kobject_put instead of _del after 
>> kobject_add)
>> Signed-off-by: Huang Guobin <huangguobin4@huawei.com>
>> ---
>>  drivers/net/bonding/bond_sysfs_slave.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_sysfs_slave.c 
>> b/drivers/net/bonding/bond_sysfs_slave.c
>> index fd07561..d1a5b3f 100644
>> --- a/drivers/net/bonding/bond_sysfs_slave.c
>> +++ b/drivers/net/bonding/bond_sysfs_slave.c
>> @@ -137,18 +137,23 @@ static ssize_t slave_show(struct kobject *kobj,
>>  
>>  int bond_sysfs_slave_add(struct slave *slave)  {
>> -	const struct slave_attribute **a;
>> +	const struct slave_attribute **a, **b;
>>  	int err;
>>  
>>  	for (a = slave_attrs; *a; ++a) {
>>  		err = sysfs_create_file(&slave->kobj, &((*a)->attr));
>>  		if (err) {
>> -			kobject_put(&slave->kobj);
>> -			return err;
>> +			goto err_remove_file;
>>  		}
>>  	}
>>  
>>  	return 0;
>> +
>> +err_remove_file:
>> +	for (b = slave_attrs; b < a; ++b)
>> +		sysfs_remove_file(&slave->kobj, &((*b)->attr));
>> +
>> +	return err;
>>  }
>>  
> 
> This looks like a candidate for sysfs_create_files(), no?
> 
>>  void bond_sysfs_slave_del(struct slave *slave)
>>
>