| Message ID | 20241022085753.2069639-1-dongchenchen2@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: netfilter: Fix use-after-free in get_info() | expand |
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); >
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)
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 > >
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 >> >>
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 >>> >>>
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.
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 }
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 } >
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);
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(-)