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

Subsystems: networking [general], networking [ipv4/ipv6], the rest

5 messages, 3 authors, 2018-10-26 · open the first message on its own page

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

From: Li RongQing <hidden>
Date: 2018-10-24 18:04:05

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 <redacted>
Signed-off-by: Zhang Yu <redacted>
Signed-off-by: Li RongQing <redacted>
---
 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

From: David Ahern <hidden>
Date: 2018-10-24 23:30:44

On 10/24/18 3:36 AM, Li RongQing wrote:

quoted hunk

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 <redacted>
Signed-off-by: Zhang Yu <redacted>
Signed-off-by: Li RongQing <redacted>
---
 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 <redacted>

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

From: David Ahern <hidden>
Date: 2018-10-24 23:49:35

On 10/24/18 9:02 AM, David Ahern wrote:

On 10/24/18 3:36 AM, Li RongQing wrote:

quoted

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 <redacted>
Signed-off-by: Zhang Yu <redacted>
Signed-off-by: Li RongQing <redacted>
---
 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

From: Bjørn Mork <bjorn@mork.no>
Date: 2018-10-26 03:17:22

David Ahern [off-list ref] writes:

On 10/24/18 9:02 AM, David Ahern wrote:

quoted

On 10/24/18 3:36 AM, Li RongQing wrote:

quoted

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 <redacted>
Signed-off-by: Zhang Yu <redacted>
Signed-off-by: Li RongQing <redacted>
---
 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

From: David Ahern <hidden>
Date: 2018-10-26 03:20:13

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,

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help