netdev - Re: [PATCH net] net: netfilter: Fix use-after-free in get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1e9083bf-2297-443c-a703-4c54b6062886@huawei.com>
Date: Wed, 23 Oct 2024 14:32:09 +0800
From: "dongchenchen (A)" <dongchenchen2@...wei.com>
To: Simon Horman <horms@...nel.org>
CC: <pablo@...filter.org>, <kadlec@...filter.org>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<fw@...len.de>, <kuniyu@...zon.com>, <netfilter-devel@...r.kernel.org>,
	<netdev@...r.kernel.org>, <yuehaibing@...wei.com>
Subject: Re: [PATCH net] net: netfilter: Fix use-after-free in get_info()


On 2024/10/22 23:33, Simon Horman wrote:
> On Tue, Oct 22, 2024 at 04:57:53PM +0800, Dong Chenchen wrote:
>> ip6table_nat module unload has refcnt warning for UAF. call trace is:
>>
>> WARNING: CPU: 1 PID: 379 at kernel/module/main.c:853 module_put+0x6f/0x80
>> Modules linked in: ip6table_nat(-)
>> CPU: 1 UID: 0 PID: 379 Comm: ip6tables Not tainted 6.12.0-rc4-00047-gc2ee9f594da8-dirty #205
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>> BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
>> RIP: 0010:module_put+0x6f/0x80
>> Call Trace:
>>   <TASK>
>>   get_info+0x128/0x180
>>   do_ip6t_get_ctl+0x6a/0x430
>>   nf_getsockopt+0x46/0x80
>>   ipv6_getsockopt+0xb9/0x100
>>   rawv6_getsockopt+0x42/0x190
>>   do_sock_getsockopt+0xaa/0x180
>>   __sys_getsockopt+0x70/0xc0
>>   __x64_sys_getsockopt+0x20/0x30
>>   do_syscall_64+0xa2/0x1a0
>>   entry_SYSCALL_64_after_hwframe+0x77/0x7f
>>
>> Concurrent execution of module unload and get_info() trigered the warning.
>> The root cause is as follows:
>>
>> cpu0				      cpu1
>> module_exit
>> //mod->state = MODULE_STATE_GOING
>>    ip6table_nat_exit
>>      xt_unregister_template
>>      //remove table from templ list
>> 				      getinfo()
>> 					  t = xt_find_table_lock
>> 						list_for_each_entry(tmpl, &xt_templates[af]...)
>> 							if (strcmp(tmpl->name, name))
>> 								continue;  //table not found
>> 							try_module_get
>> 						list_for_each_entry(t, &xt_net->tables[af]...)
>> 							return t;  //not get refcnt
>> 					  module_put(t->me) //uaf
>>      unregister_pernet_subsys
>>      //remove table from xt_net list
>>
>> While xt_table module was going away and has been removed from
>> xt_templates list, we couldnt get refcnt of xt_table->me. Skip
>> the re-traversal of xt_net->tables list to fix it.
>>
>> Fixes: c22921df777d ("netfilter: iptables: Fix potential null-ptr-deref in ip6table_nat_table_init().")
>> Signed-off-by: Dong Chenchen <dongchenchen2@...wei.com>
>> ---
>>   net/netfilter/x_tables.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
>> index da5d929c7c85..359c880ecb07 100644
>> --- a/net/netfilter/x_tables.c
>> +++ b/net/netfilter/x_tables.c
>> @@ -1239,6 +1239,7 @@ struct xt_table *xt_find_table_lock(struct net *net, u_int8_t af,
>>   	struct module *owner = NULL;
>>   	struct xt_template *tmpl;
>>   	struct xt_table *t;
>> +	int err = -ENOENT;
>>   
>>   	mutex_lock(&xt[af].mutex);
>>   	list_for_each_entry(t, &xt_net->tables[af], list)
>> @@ -1247,8 +1248,6 @@ struct xt_table *xt_find_table_lock(struct net *net, u_int8_t af,
>>   
>>   	/* Table doesn't exist in this netns, check larval list */
>>   	list_for_each_entry(tmpl, &xt_templates[af], list) {
>> -		int err;
>> -
>>   		if (strcmp(tmpl->name, name))
>>   			continue;
>>   		if (!try_module_get(tmpl->me))
>> @@ -1267,6 +1266,9 @@ struct xt_table *xt_find_table_lock(struct net *net, u_int8_t af,
>>   		break;
>>   	}
>>   
>> +	if (err < 0)
>> +		goto out;
>> +
>>   	/* and once again: */
>>   	list_for_each_entry(t, &xt_net->tables[af], list)
>>   		if (strcmp(t->name, name) == 0)
>> @@ -1275,7 +1277,7 @@ struct xt_table *xt_find_table_lock(struct net *net, u_int8_t af,
>>   	module_put(owner);
> Hi Dong Chenchen,
>
> I'm unsure if this can happen in practice, although I guess so else the
> module_put() call above is never reached.

Hi, Simon. Thank you very much for your suggestions!

module_put(owner) will be never reached indeed. which wiil be executed in:

1. xt_table not found in tmpl list and xt_net list:

Owner == NULL, no need to put

2. xt_table found in tmpl list;  table_init() fail to add table to 
xt_net list but return 0

this situation may be mutually exclusive

So I thought it may not need to call module_puy(owner) here

xt_find_table_lock

list_for_each_entry(tmpl, &xt_templates[af], list)

if (strcmp(tmpl->name, name))

continue;

err = tmpl->table_init(net); //add xtable to xt_net list

if (err < 0) {

module_put(owner);

return ERR_PTR(err);

}

list_for_each_entry(t, &xt_net->tables[af], list)

if (strcmp(t->name, name) == 0)

return t;//err = 0, will return here

module_put(owner); // put effectively while (err == 0) && (xtable found 
in tmpl list) and add table xt_net list failed in table_init()

out:

mutex_unlock(&xt[af].mutex);

return ERR_PTR(-ENOENT);

> In any case, previously if we got
> to this line then the function would return ERR_PTR(-ENOENT).  But now it
> will return ERR_PTR(0). Which although valid often indicates a bug.
>
> Flagged by Smatch.

As described above, err = 0 will be return in xt_net table list re- 
traversal.

>>    out:
>>   	mutex_unlock(&xt[af].mutex);
>> -	return ERR_PTR(-ENOENT);
>> +	return ERR_PTR(err);
>>   }
>>   EXPORT_SYMBOL_GPL(xt_find_table_lock);
>>   
>> -- 
>> 2.25.1
>>
>>