netdev - Re: [PATCH net] ip6mr: fix UAF issue in ip6mr_sk_done() when addrconf_init

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]

Date:   Tue, 18 Oct 2022 09:48:15 +0800
From:   shaozhengchao <shaozhengchao@...wei.com>
To:     Eric Dumazet <edumazet@...gle.com>
CC:     <netdev@...r.kernel.org>, <davem@...emloft.net>,
        <yoshfuji@...ux-ipv6.org>, <dsahern@...nel.org>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <weiyongjun1@...wei.com>,
        <yuehaibing@...wei.com>
Subject: Re: [PATCH net] ip6mr: fix UAF issue in ip6mr_sk_done() when
 addrconf_init_net() failed



On 2022/10/17 23:58, Eric Dumazet wrote:
> On Mon, Oct 17, 2022 at 12:55 AM Zhengchao Shao
> <shaozhengchao@...wei.com> wrote:
>>
>> If the initialization fails in calling addrconf_init_net(), devconf_all is
>> the pointer that has been released. Then ip6mr_sk_done() is called to
>> release the net, accessing devconf->mc_forwarding directly causes invalid
>> pointer access.
>>
>> The process is as follows:
>> setup_net()
>>          ops_init()
>>                  addrconf_init_net()
>>                  all = kmemdup(...)           ---> alloc "all"
>>                  ...
>>                  net->ipv6.devconf_all = all;
>>                  __addrconf_sysctl_register() ---> failed
>>                  ...
>>                  kfree(all);                  ---> ipv6.devconf_all invalid
>>                  ...
>>          ops_exit_list()
>>                  ...
>>                  ip6mr_sk_done()
>>                          devconf = net->ipv6.devconf_all;
>>                          //devconf is invalid pointer
>>                          if (!devconf || !atomic_read(&devconf->mc_forwarding))
>>
>>
>> Fixes: 7d9b1b578d67 ("ip6mr: fix use-after-free in ip6mr_sk_done()")
> 
> Hmmm. I wonder if you are not masking a more serious bug.
> 
> When I wrote this patch ( 7d9b1b578d67) I was following the standard
> rule of ops->exit() being called
> only if the corresponding ops->init() function had not failed.
> 
> net/core/net_namespace.c:setup_net() even has some comments about this rule:
> 
> out_undo:
>          /* Walk through the list backwards calling the exit functions
>           * for the pernet modules whose init functions did not fail.
>           */
>          list_add(&net->exit_list, &net_exit_list);
>          saved_ops = ops;
>          list_for_each_entry_continue_reverse(ops, &pernet_list, list)
>                  ops_pre_exit_list(ops, &net_exit_list);
> 
>          synchronize_rcu();
> 
>          ops = saved_ops;
>          list_for_each_entry_continue_reverse(ops, &pernet_list, list)
>                  ops_exit_list(ops, &net_exit_list);
> 
>          ops = saved_ops;
>          list_for_each_entry_continue_reverse(ops, &pernet_list, list)
>                  ops_free_list(ops, &net_exit_list);
> 
>          rcu_barrier();
>          goto out;
> 
> 
Hi Eric:
	Thank you for your reply. I wonder if fixing commit
e0da5a480caf ("[NETNS]: Create ipv6 devconf-s for namespaces")
would be more appropriate?

Zhengchao Shao

> 
>> Signed-off-by: Zhengchao Shao <shaozhengchao@...wei.com>
>> ---
>>   net/ipv6/addrconf.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index 417834b7169d..9c3f5202a97b 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -7214,9 +7214,11 @@ static int __net_init addrconf_init_net(struct net *net)
>>          __addrconf_sysctl_unregister(net, all, NETCONFA_IFINDEX_ALL);
>>   err_reg_all:
>>          kfree(dflt);
>> +       net->ipv6.devconf_dflt = NULL;
>>   #endif
>>   err_alloc_dflt:
>>          kfree(all);
>> +       net->ipv6.devconf_all = NULL;
>>   err_alloc_all:
>>          kfree(net->ipv6.inet6_addr_lst);
>>   err_alloc_addr:
>> --
>> 2.17.1
>>