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: <7492d260-0e17-4f60-81c2-861efe317ece@huawei.com>
Date: Wed, 23 Oct 2024 14:52:00 +0800
From: "dongchenchen (A)" <dongchenchen2@...wei.com>
To: <dongchenchen2@...wei.com>
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>, Simon Horman
	<horms@...nel.org>
Subject: Re: [PATCH net] net: netfilter: Fix use-after-free in get_info()


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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ