| Message ID | 20221017080331.16878-1-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1ca695207ed2271ecbf8ee6c641970f621c157cc |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ip6mr: fix UAF issue in ip6mr_sk_done() when addrconf_init_net() failed | expand |
On Mon, Oct 17, 2022 at 12:55 AM Zhengchao Shao <shaozhengchao@huawei.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; > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.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 >
On 2022/10/17 23:58, Eric Dumazet wrote: > On Mon, Oct 17, 2022 at 12:55 AM Zhengchao Shao > <shaozhengchao@huawei.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@huawei.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 >>
On Mon, Oct 17, 2022 at 6:48 PM shaozhengchao <shaozhengchao@huawei.com> wrote: > > > > On 2022/10/17 23:58, Eric Dumazet wrote: > > On Mon, Oct 17, 2022 at 12:55 AM Zhengchao Shao > > <shaozhengchao@huawei.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@huawei.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 > >>
On Mon, Oct 17, 2022 at 7:33 PM Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Oct 17, 2022 at 6:48 PM shaozhengchao <shaozhengchao@huawei.com> wrote: > > 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. > Hmm... I read again the stack traces, and while addrconf_exit_net() is not called, igmp6_net_exit() _is_ called, so I guess your patch is fine, thanks ! Reviewed-by: Eric Dumazet <edumazet@google.com>
Hello: This patch was applied to netdev/net.git (master) by Paolo Abeni <pabeni@redhat.com>: On Mon, 17 Oct 2022 16:03:31 +0800 you 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)) > > [...] Here is the summary with links: - [net] ip6mr: fix UAF issue in ip6mr_sk_done() when addrconf_init_net() failed https://git.kernel.org/netdev/net/c/1ca695207ed2 You are awesome, thank you!
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:
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)) The following is the Call Trace information: BUG: KASAN: use-after-free in ip6mr_sk_done+0x112/0x3a0 Read of size 4 at addr ffff888075508e88 by task ip/14554 Call Trace: <TASK> dump_stack_lvl+0x8e/0xd1 print_report+0x155/0x454 kasan_report+0xba/0x1f0 kasan_check_range+0x35/0x1b0 ip6mr_sk_done+0x112/0x3a0 rawv6_close+0x48/0x70 inet_release+0x109/0x230 inet6_release+0x4c/0x70 sock_release+0x87/0x1b0 igmp6_net_exit+0x6b/0x170 ops_exit_list+0xb0/0x170 setup_net+0x7ac/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 RIP: 0033:0x7f7963322547 </TASK> Allocated by task 14554: kasan_save_stack+0x1e/0x40 kasan_set_track+0x21/0x30 __kasan_kmalloc+0xa1/0xb0 __kmalloc_node_track_caller+0x4a/0xb0 kmemdup+0x28/0x60 addrconf_init_net+0x1be/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 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 Fixes: 7d9b1b578d67 ("ip6mr: fix use-after-free in ip6mr_sk_done()") Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- net/ipv6/addrconf.c | 2 ++ 1 file changed, 2 insertions(+)