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:   Mon, 17 Oct 2022 19:33:46 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     shaozhengchao <shaozhengchao@...wei.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 Mon, Oct 17, 2022 at 6:48 PM shaozhengchao <shaozhengchao@...wei.com> wrote:
>
>
>
> 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?

Do you have a repro ?

You could first use scripts/decode_stack.pl to get symbols from your
traces, to confirm the issue.

Freed by task 14554:
kasan_save_stack+0x1e/0x40
kasan_set_track+0x21/0x30
kasan_save_free_info+0x2a/0x40
____kasan_slab_free+0x155/0x1b0
slab_free_freelist_hook+0x11b/0x220
__kmem_cache_free+0xa4/0x360
addrconf_init_net+0x623/0x840
ops_init+0xa5/0x410
setup_net+0x5aa/0xbd0
copy_net_ns+0x2e6/0x6b0
create_new_namespaces+0x382/0xa50
unshare_nsproxy_namespaces+0xa6/0x1c0
ksys_unshare+0x3a4/0x7e0
__x64_sys_unshare+0x2d/0x40
do_syscall_64+0x35/0x80
entry_SYSCALL_64_after_hwframe+0x46/0xb0

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