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]

Message-ID: <CANn89iJdZx=e2QN_AXPiZQDh4u4EY5dOrzgdsqgWTCpvLhJVcQ@mail.gmail.com>
Date:   Mon, 17 Oct 2022 08:58:17 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Zhengchao Shao <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 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;



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