linux-kernel - Re: [PATCH] netlink: fix memory leak

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:   Sun, 22 Jul 2018 07:00:47 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     cscnull@...il.com
Cc:     Pablo Neira Ayuso <pablo@...filter.org>, kadlec@...ckhole.kfki.hu,
        Florian Westphal <fw@...len.de>,
        David Miller <davem@...emloft.net>,
        "Berg, Johannes" <johannes.berg@...el.com>,
        Philippe Ombredanne <pombredanne@...b.com>,
        kstewart@...uxfoundation.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        dsahern@...il.com, lucien.xin@...il.com, ktkhai@...tuozzo.com,
        Cong Wang <xiyou.wangcong@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        netfilter-devel@...r.kernel.org, coreteam@...filter.org,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] netlink: fix memory leak

On Sun, Jul 22, 2018 at 4:51 AM Shaochun Chen <cscnull@...il.com> wrote:
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 393573a99a5a..7b85176cf9bb 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2275,6 +2275,7 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
>         struct netlink_callback *cb;
>         struct sock *sk;
>         struct netlink_sock *nlk;
> +       bool cb_running = false;
>         int ret;
>
>         refcount_inc(&skb->users);
> @@ -2317,6 +2318,7 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
>
>         nlk->cb_running = true;
>         nlk->dump_done_errno = INT_MAX;
> +       cb_running = true;
>
>         mutex_unlock(nlk->cb_mutex);
>
> @@ -2339,6 +2341,8 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
>         mutex_unlock(nlk->cb_mutex);
>  error_free:
>         kfree_skb(skb);
> +       if (cb_running)
> +               netlink_dump_start_fail(control);

cb_running is never true here, since nothing jumps to error_free after
you set it to be true. Pasting more code for context:


       nlk->cb_running = true;
       nlk->dump_done_errno = INT_MAX;
1) ----> cb_runnning = true;

       mutex_unlock(nlk->cb_mutex);

       ret = netlink_dump(sk);

       sock_put(sk);

       if (ret)
A)               return ret;

       /* We successfully started a dump, by returning -EINTR we
        * signal not to send ACK even if it was requested.
        */
B)       return -EINTR;

error_put:
       module_put(control->module);
error_unlock:
       sock_put(sk);
       mutex_unlock(nlk->cb_mutex);
error_free:
       kfree_skb(skb);
2) ----> if (cb_running) netlink_dump_start_fail(control);
       return ret;


After (1) is set, the function exits via (A) or (B), and so (2) is never hit.


But even if you moved it somehow to the if(ret), I'm still not sure
it'd be correct; start cbs should either succeed, or they should error
out and cleanup entirely after themselves.


>         return ret;
>  }
>  EXPORT_SYMBOL(__netlink_dump_start);
> --
> 2.17.1
>