[PATCH net] net: netfilter: Fix use-after-free in get_info()

[PATCH net] net: netfilter: Fix use-after-free in get_info()

[Dong Chenchen]

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@huawei.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);
  out:
 	mutex_unlock(&xt[af].mutex);
-	return ERR_PTR(-ENOENT);
+	return ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(xt_find_table_lock);
 
-- 
2.25.1

Re: [PATCH net] net: netfilter: Fix use-after-free in get_info()

[Yue Haibing]

On 2024/10/22 16:57, 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().")

This should be
Fixes: fdacd57c79b7 ("netfilter: x_tables: never register tables by default")

> Signed-off-by: Dong Chenchen <dongchenchen2@huawei.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);
>   out:
>  	mutex_unlock(&xt[af].mutex);
> -	return ERR_PTR(-ENOENT);
> +	return ERR_PTR(err);
>  }
>  EXPORT_SYMBOL_GPL(xt_find_table_lock);
>  

Re: [PATCH net] net: netfilter: Fix use-after-free in get_info()

[Florian Westphal]

Dong Chenchen <dongchenchen2@huawei.com> wrote:
>  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)

Proabably also:

-  		if (strcmp(t->name, name) == 0)
+               if (strcmp(t->name, name) == 0 && owner == t->me)

Re: [PATCH net] net: netfilter: Fix use-after-free in get_info()

[Simon Horman]

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@huawei.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. 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.

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

Re: [PATCH net] net: netfilter: Fix use-after-free in get_info()

[dongchenchen (A)]

> Dong Chenchen <dongchenchen2@huawei.com> wrote:
> >  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)
> 
> Proabably also:
> 
> -  		if (strcmp(t->name, name) == 0)
> +               if (strcmp(t->name, name) == 0 && owner == t->me)
> 
Thank you very much for your suggestions!
V2 will be sent.

Re: [PATCH net] net: netfilter: Fix use-after-free in get_info()

[dongchenchen (A)]

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@huawei.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
>>
>>

Re: [PATCH net] net: netfilter: Fix use-after-free in get_info()

[dongchenchen (A)]

On 2024/10/23 14:32, dongchenchen (A) wrote:
>
> 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@huawei.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:
>
>
sorry, there is a problem with the email format. resend:

module_put(owner) 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
>>>
>>>

Re: [PATCH net] net: netfilter: Fix use-after-free in get_info()

[dongchenchen (A)]

On 2024/10/22 17:48, Florian Westphal wrote:
> Dong Chenchen <dongchenchen2@huawei.com> wrote:
>>   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)
> Proabably also:
>
> -  		if (strcmp(t->name, name) == 0)
> +               if (strcmp(t->name, name) == 0 && owner == t->me)

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

If err <0 , xt_table does not exist in the xt_net list or the tmpl list.

And  list operations are performed in the mutex lock.

Therefore, it may not be necessary to traverse the xt_net list again.

Re: [PATCH net] net: netfilter: Fix use-after-free in get_info()

[Dan Carpenter]

Hi Dong,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Dong-Chenchen/net-netfilter-Fix-use-after-free-in-get_info/20241022-165936
base:   net/main
patch link:    https://lore.kernel.org/r/20241022085753.2069639-1-dongchenchen2%40huawei.com
patch subject: [PATCH net] net: netfilter: Fix use-after-free in get_info()
config: x86_64-randconfig-161-20241023 (https://download.01.org/0day-ci/archive/20241024/202410240020.Cqi2d68p-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202410240020.Cqi2d68p-lkp@intel.com/

smatch warnings:
net/netfilter/x_tables.c:1280 xt_find_table_lock() warn: passing zero to 'ERR_PTR'

vim +/ERR_PTR +1280 net/netfilter/x_tables.c

03d13b6868a261 Florian Westphal  2017-12-08  1234  /* Find table by name, grabs mutex & ref.  Returns ERR_PTR on error. */
76108cea065cda Jan Engelhardt    2008-10-08  1235  struct xt_table *xt_find_table_lock(struct net *net, u_int8_t af,
76108cea065cda Jan Engelhardt    2008-10-08  1236  				    const char *name)
2e4e6a17af35be Harald Welte      2006-01-12  1237  {
1d610d4d31a8ed Florian Westphal  2021-04-01  1238  	struct xt_pernet *xt_net = net_generic(net, xt_pernet_id);
fdacd57c79b79a Florian Westphal  2021-08-03  1239  	struct module *owner = NULL;
fdacd57c79b79a Florian Westphal  2021-08-03  1240  	struct xt_template *tmpl;
fdacd57c79b79a Florian Westphal  2021-08-03  1241  	struct xt_table *t;
f4f502d5a8ea29 Dong Chenchen     2024-10-22  1242  	int err = -ENOENT;
2e4e6a17af35be Harald Welte      2006-01-12  1243  
7926dbfa4bc14e Pablo Neira Ayuso 2014-07-31  1244  	mutex_lock(&xt[af].mutex);
1d610d4d31a8ed Florian Westphal  2021-04-01  1245  	list_for_each_entry(t, &xt_net->tables[af], list)
2e4e6a17af35be Harald Welte      2006-01-12  1246  		if (strcmp(t->name, name) == 0 && try_module_get(t->me))
2e4e6a17af35be Harald Welte      2006-01-12  1247  			return t;
b9e69e12739718 Florian Westphal  2016-02-25  1248  
fdacd57c79b79a Florian Westphal  2021-08-03  1249  	/* Table doesn't exist in this netns, check larval list */
fdacd57c79b79a Florian Westphal  2021-08-03  1250  	list_for_each_entry(tmpl, &xt_templates[af], list) {
fdacd57c79b79a Florian Westphal  2021-08-03  1251  		if (strcmp(tmpl->name, name))
b9e69e12739718 Florian Westphal  2016-02-25  1252  			continue;
fdacd57c79b79a Florian Westphal  2021-08-03  1253  		if (!try_module_get(tmpl->me))
03d13b6868a261 Florian Westphal  2017-12-08  1254  			goto out;
fdacd57c79b79a Florian Westphal  2021-08-03  1255  
fdacd57c79b79a Florian Westphal  2021-08-03  1256  		owner = tmpl->me;
fdacd57c79b79a Florian Westphal  2021-08-03  1257  
b9e69e12739718 Florian Westphal  2016-02-25  1258  		mutex_unlock(&xt[af].mutex);
fdacd57c79b79a Florian Westphal  2021-08-03  1259  		err = tmpl->table_init(net);
03d13b6868a261 Florian Westphal  2017-12-08  1260  		if (err < 0) {
fdacd57c79b79a Florian Westphal  2021-08-03  1261  			module_put(owner);
03d13b6868a261 Florian Westphal  2017-12-08  1262  			return ERR_PTR(err);
b9e69e12739718 Florian Westphal  2016-02-25  1263  		}
b9e69e12739718 Florian Westphal  2016-02-25  1264  
b9e69e12739718 Florian Westphal  2016-02-25  1265  		mutex_lock(&xt[af].mutex);
b9e69e12739718 Florian Westphal  2016-02-25  1266  		break;
b9e69e12739718 Florian Westphal  2016-02-25  1267  	}
b9e69e12739718 Florian Westphal  2016-02-25  1268  
f4f502d5a8ea29 Dong Chenchen     2024-10-22  1269  	if (err < 0)
f4f502d5a8ea29 Dong Chenchen     2024-10-22  1270  		goto out;
f4f502d5a8ea29 Dong Chenchen     2024-10-22  1271  
b9e69e12739718 Florian Westphal  2016-02-25  1272  	/* and once again: */
1d610d4d31a8ed Florian Westphal  2021-04-01  1273  	list_for_each_entry(t, &xt_net->tables[af], list)
b9e69e12739718 Florian Westphal  2016-02-25  1274  		if (strcmp(t->name, name) == 0)
b9e69e12739718 Florian Westphal  2016-02-25  1275  			return t;

ret it zero here, but if we fail to find the name then we should set ret =
-ENOENT;

b9e69e12739718 Florian Westphal  2016-02-25  1276  
fdacd57c79b79a Florian Westphal  2021-08-03  1277  	module_put(owner);
b9e69e12739718 Florian Westphal  2016-02-25  1278   out:
9e19bb6d7a0959 Ingo Molnar       2006-03-25  1279  	mutex_unlock(&xt[af].mutex);
f4f502d5a8ea29 Dong Chenchen     2024-10-22 @1280  	return ERR_PTR(err);
2e4e6a17af35be Harald Welte      2006-01-12  1281  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net] net: netfilter: Fix use-after-free in get_info()

[dongchenchen (A)]

On 2024/10/24 1:16, Dan Carpenter wrote:
> Hi Dong,
>
> kernel test robot noticed the following build warnings:
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Dong-Chenchen/net-netfilter-Fix-use-after-free-in-get_info/20241022-165936
> base:   net/main
> patch link:    https://lore.kernel.org/r/20241022085753.2069639-1-dongchenchen2%40huawei.com
> patch subject: [PATCH net] net: netfilter: Fix use-after-free in get_info()
> config: x86_64-randconfig-161-20241023 (https://download.01.org/0day-ci/archive/20241024/202410240020.Cqi2d68p-lkp@intel.com/config)
> compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202410240020.Cqi2d68p-lkp@intel.com/
>
> smatch warnings:
> net/netfilter/x_tables.c:1280 xt_find_table_lock() warn: passing zero to 'ERR_PTR'
>
> vim +/ERR_PTR +1280 net/netfilter/x_tables.c
>
> 03d13b6868a261 Florian Westphal  2017-12-08  1234  /* Find table by name, grabs mutex & ref.  Returns ERR_PTR on error. */
> 76108cea065cda Jan Engelhardt    2008-10-08  1235  struct xt_table *xt_find_table_lock(struct net *net, u_int8_t af,
> 76108cea065cda Jan Engelhardt    2008-10-08  1236  				    const char *name)
> 2e4e6a17af35be Harald Welte      2006-01-12  1237  {
> 1d610d4d31a8ed Florian Westphal  2021-04-01  1238  	struct xt_pernet *xt_net = net_generic(net, xt_pernet_id);
> fdacd57c79b79a Florian Westphal  2021-08-03  1239  	struct module *owner = NULL;
> fdacd57c79b79a Florian Westphal  2021-08-03  1240  	struct xt_template *tmpl;
> fdacd57c79b79a Florian Westphal  2021-08-03  1241  	struct xt_table *t;
> f4f502d5a8ea29 Dong Chenchen     2024-10-22  1242  	int err = -ENOENT;
> 2e4e6a17af35be Harald Welte      2006-01-12  1243
> 7926dbfa4bc14e Pablo Neira Ayuso 2014-07-31  1244  	mutex_lock(&xt[af].mutex);
> 1d610d4d31a8ed Florian Westphal  2021-04-01  1245  	list_for_each_entry(t, &xt_net->tables[af], list)
> 2e4e6a17af35be Harald Welte      2006-01-12  1246  		if (strcmp(t->name, name) == 0 && try_module_get(t->me))
> 2e4e6a17af35be Harald Welte      2006-01-12  1247  			return t;
> b9e69e12739718 Florian Westphal  2016-02-25  1248
> fdacd57c79b79a Florian Westphal  2021-08-03  1249  	/* Table doesn't exist in this netns, check larval list */
> fdacd57c79b79a Florian Westphal  2021-08-03  1250  	list_for_each_entry(tmpl, &xt_templates[af], list) {
> fdacd57c79b79a Florian Westphal  2021-08-03  1251  		if (strcmp(tmpl->name, name))
> b9e69e12739718 Florian Westphal  2016-02-25  1252  			continue;
> fdacd57c79b79a Florian Westphal  2021-08-03  1253  		if (!try_module_get(tmpl->me))
> 03d13b6868a261 Florian Westphal  2017-12-08  1254  			goto out;
> fdacd57c79b79a Florian Westphal  2021-08-03  1255
> fdacd57c79b79a Florian Westphal  2021-08-03  1256  		owner = tmpl->me;
> fdacd57c79b79a Florian Westphal  2021-08-03  1257
> b9e69e12739718 Florian Westphal  2016-02-25  1258  		mutex_unlock(&xt[af].mutex);
> fdacd57c79b79a Florian Westphal  2021-08-03  1259  		err = tmpl->table_init(net);
> 03d13b6868a261 Florian Westphal  2017-12-08  1260  		if (err < 0) {
> fdacd57c79b79a Florian Westphal  2021-08-03  1261  			module_put(owner);
> 03d13b6868a261 Florian Westphal  2017-12-08  1262  			return ERR_PTR(err);
> b9e69e12739718 Florian Westphal  2016-02-25  1263  		}
> b9e69e12739718 Florian Westphal  2016-02-25  1264

If rmmod is executed concurrently here, xtable will be remove from 
xt_net list，

which may lead to ERR_PTR(0).

Thank you for your review. v2 has been sent

> b9e69e12739718 Florian Westphal  2016-02-25  1265  		mutex_lock(&xt[af].mutex);
> b9e69e12739718 Florian Westphal  2016-02-25  1266  		break;
> b9e69e12739718 Florian Westphal  2016-02-25  1267  	}
> b9e69e12739718 Florian Westphal  2016-02-25  1268
> f4f502d5a8ea29 Dong Chenchen     2024-10-22  1269  	if (err < 0)
> f4f502d5a8ea29 Dong Chenchen     2024-10-22  1270  		goto out;
> f4f502d5a8ea29 Dong Chenchen     2024-10-22  1271
> b9e69e12739718 Florian Westphal  2016-02-25  1272  	/* and once again: */
> 1d610d4d31a8ed Florian Westphal  2021-04-01  1273  	list_for_each_entry(t, &xt_net->tables[af], list)
> b9e69e12739718 Florian Westphal  2016-02-25  1274  		if (strcmp(t->name, name) == 0)
> b9e69e12739718 Florian Westphal  2016-02-25  1275  			return t;
>
> ret it zero here, but if we fail to find the name then we should set ret =
> -ENOENT;
>
> b9e69e12739718 Florian Westphal  2016-02-25  1276
> fdacd57c79b79a Florian Westphal  2021-08-03  1277  	module_put(owner);
> b9e69e12739718 Florian Westphal  2016-02-25  1278   out:
> 9e19bb6d7a0959 Ingo Molnar       2006-03-25  1279  	mutex_unlock(&xt[af].mutex);
> f4f502d5a8ea29 Dong Chenchen     2024-10-22 @1280  	return ERR_PTR(err);
> 2e4e6a17af35be Harald Welte      2006-01-12  1281  }
>