[net-next][PATCH] net/ipv4: fix a net leak

[net-next][PATCH] net/ipv4: fix a net leak

[Li RongQing]

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@gmail.com>
Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
Signed-off-by: Li RongQing <lirongqing@baidu.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) {
-- 
2.16.2

Re: [net-next][PATCH] net/ipv4: fix a net leak

[David Ahern]

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@gmail.com>
> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.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.

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [net-next][PATCH] net/ipv4: fix a net leak

[David Ahern]

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@gmail.com>
>> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
>> Signed-off-by: Li RongQing <lirongqing@baidu.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.

Re: [net-next][PATCH] net/ipv4: fix a net leak

[Bjørn Mork]

David Ahern <dsahern@gmail.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@gmail.com>
>>> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
>>> Signed-off-by: Li RongQing <lirongqing@baidu.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

Re: [net-next][PATCH] net/ipv4: fix a net leak

[David Ahern]

On 10/25/18 12:43 PM, Bjørn Mork wrote:
> 
> 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.

good point. either use of an intermediate or resetting nsid on failure.
Will you send a patch to fix ipv4 and v6?

Thanks,