netdev - Re: [net-next][PATCH] net/ipv4: fix a net 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]

Message-ID: <87lg6l7tq2.fsf@miraculix.mork.no>
Date:   Thu, 25 Oct 2018 20:43:17 +0200
From:   Bjørn Mork <bjorn@...k.no>
To:     David Ahern <dsahern@...il.com>
Cc:     Li RongQing <lirongqing@...du.com>, netdev@...r.kernel.org,
        David Miller <davem@...emloft.net>
Subject: Re: [net-next][PATCH] net/ipv4: fix a net leak

David Ahern <dsahern@...il.com> writes:
> On 10/24/18 9:02 AM, David Ahern wrote:
>> On 10/24/18 3:36 AM, Li RongQing wrote:
>>> put net when input a invalid ifindex, otherwise it will be leaked
>>>
>>> Fixes: 5fcd266a9f64("net/ipv4: Add support for dumping addresses for a specific device")
>>> Cc: David Ahern <dsahern@...il.com>
>>> Signed-off-by: Zhang Yu <zhangyu31@...du.com>
>>> Signed-off-by: Li RongQing <lirongqing@...du.com>
>>> ---
>>>  net/ipv4/devinet.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
>>> index 63d5b58fbfdb..fd0c5a47e742 100644
>>> --- a/net/ipv4/devinet.c
>>> +++ b/net/ipv4/devinet.c
>>> @@ -1775,8 +1775,10 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
>>>  
>>>  		if (fillargs.ifindex) {
>>>  			dev = __dev_get_by_index(tgt_net, fillargs.ifindex);
>>> -			if (!dev)
>>> +			if (!dev) {
>>> +				put_net(tgt_net);
>>>  				return -ENODEV;
>>> +			}
>>>  
>>>  			in_dev = __in_dev_get_rtnl(dev);
>>>  			if (in_dev) {
>>>
>> 
>> Good catch. IPv6 has the same problem. Will fix that one.
>> 
> Actually remove that 'Reviewed-by'. You should only call put_net if
> (fillargs.netnsid >= 0)
>
> DaveM: just want to call this out since I mistakenly added the
> Reviewed-by. This patch should be dropped.

Hmm, I see that you implemented that.  But I believe it's still buggy if
called with an invalid netnsid.

inet_valid_dump_ifaddr_req() will bail out with an error, but only
*after* setting fillargs->netnsid:

                if (i == IFA_TARGET_NETNSID) {
                        struct net *net;

                        fillargs->netnsid = nla_get_s32(tb[i]);

                        net = rtnl_get_net_ns_capable(sk, fillargs->netnsid);
                        if (IS_ERR(net)) {
                                NL_SET_ERR_MSG(extack, "ipv4: Invalid target network namespace id");
                                return PTR_ERR(net);
                        }
                        *tgt_net = net;
                } else {



So inet_dump_ifaddr() ends up doing put_net(tgt_net):


                err = inet_valid_dump_ifaddr_req(nlh, &fillargs, &tgt_net,
                                                 skb->sk, cb);
                if (err < 0)
                        goto put_tgt_net;
..
put_tgt_net:
        if (fillargs.netnsid >= 0)
                put_net(tgt_net);



I believe you should set fillargs->netnsid back to -1 in the
inet_valid_dump_ifaddr_req() error path, or use a temp variable to avoid
changing it unless get_net is successful.



Bjørn