* [PATCH net] ip6mr: fix UAF issue in ip6mr_sk_done() when addrconf_init_net() failed
@ 2022-10-17 8:03 Zhengchao Shao
2022-10-17 15:58 ` Eric Dumazet
2022-10-18 9:20 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 6+ messages in thread
From: Zhengchao Shao @ 2022-10-17 8:03 UTC (permalink / raw)
To: netdev, davem, yoshfuji, dsahern, edumazet, kuba, pabeni
Cc: weiyongjun1, yuehaibing, shaozhengchao
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(+)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] ip6mr: fix UAF issue in ip6mr_sk_done() when addrconf_init_net() failed
2022-10-17 8:03 [PATCH net] ip6mr: fix UAF issue in ip6mr_sk_done() when addrconf_init_net() failed Zhengchao Shao
@ 2022-10-17 15:58 ` Eric Dumazet
2022-10-18 1:48 ` shaozhengchao
2022-10-18 9:20 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2022-10-17 15:58 UTC (permalink / raw)
To: Zhengchao Shao
Cc: netdev, davem, yoshfuji, dsahern, kuba, pabeni, weiyongjun1, yuehaibing
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ip6mr: fix UAF issue in ip6mr_sk_done() when addrconf_init_net() failed
2022-10-17 15:58 ` Eric Dumazet
@ 2022-10-18 1:48 ` shaozhengchao
2022-10-18 2:33 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: shaozhengchao @ 2022-10-18 1:48 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, davem, yoshfuji, dsahern, kuba, pabeni, weiyongjun1, yuehaibing
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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ip6mr: fix UAF issue in ip6mr_sk_done() when addrconf_init_net() failed
2022-10-18 1:48 ` shaozhengchao
@ 2022-10-18 2:33 ` Eric Dumazet
2022-10-18 3:35 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2022-10-18 2:33 UTC (permalink / raw)
To: shaozhengchao
Cc: netdev, davem, yoshfuji, dsahern, kuba, pabeni, weiyongjun1, yuehaibing
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
> >>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ip6mr: fix UAF issue in ip6mr_sk_done() when addrconf_init_net() failed
2022-10-18 2:33 ` Eric Dumazet
@ 2022-10-18 3:35 ` Eric Dumazet
0 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2022-10-18 3:35 UTC (permalink / raw)
To: shaozhengchao
Cc: netdev, davem, yoshfuji, dsahern, kuba, pabeni, weiyongjun1, yuehaibing
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] ip6mr: fix UAF issue in ip6mr_sk_done() when addrconf_init_net() failed
2022-10-17 8:03 [PATCH net] ip6mr: fix UAF issue in ip6mr_sk_done() when addrconf_init_net() failed Zhengchao Shao
2022-10-17 15:58 ` Eric Dumazet
@ 2022-10-18 9:20 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-10-18 9:20 UTC (permalink / raw)
To: Zhengchao Shao
Cc: netdev, davem, yoshfuji, dsahern, edumazet, kuba, pabeni,
weiyongjun1, yuehaibing
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-18 9:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-17 8:03 [PATCH net] ip6mr: fix UAF issue in ip6mr_sk_done() when addrconf_init_net() failed Zhengchao Shao
2022-10-17 15:58 ` Eric Dumazet
2022-10-18 1:48 ` shaozhengchao
2022-10-18 2:33 ` Eric Dumazet
2022-10-18 3:35 ` Eric Dumazet
2022-10-18 9:20 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.