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