[PATCH net v2 1/2] rtnetlink: restore handling of dumpit return value in rtnl_dump_all()

[PATCH net v2 1/2] rtnetlink: restore handling of dumpit return value in rtnl_dump_all()

[Alexey Kodanev]

For non-zero return from dumpit() we should break the loop
in rtnl_dump_all() and return the result. Otherwise, e.g.,
we could get the memory leak in inet6_dump_fib() [1]. The
pointer to the allocated struct fib6_walker there (saved
in cb->args) can be lost, reset on the next iteration.

Fix it by partially restoring the previous behavior before
commit c63586dc9b3e ("net: rtnl_dump_all needs to propagate
error from dumpit function"). The returned error from
dumpit() is still passed further.

[1]:
unreferenced object 0xffff88001322a200 (size 96):
  comm "sshd", pid 1484, jiffies 4296032768 (age 1432.542s)
  hex dump (first 32 bytes):
    00 01 00 00 00 00 ad de 00 02 00 00 00 00 ad de  ................
    18 09 41 36 00 88 ff ff 18 09 41 36 00 88 ff ff  ..A6......A6....
  backtrace:
    [<0000000095846b39>] kmem_cache_alloc_trace+0x151/0x220
    [<000000007d12709f>] inet6_dump_fib+0x68d/0x940
    [<000000002775a316>] rtnl_dump_all+0x1d9/0x2d0
    [<00000000d7cd302b>] netlink_dump+0x945/0x11a0
    [<000000002f43485f>] __netlink_dump_start+0x55d/0x800
    [<00000000f76bbeec>] rtnetlink_rcv_msg+0x4fa/0xa00
    [<000000009b5761f3>] netlink_rcv_skb+0x29c/0x420
    [<0000000087a1dae1>] rtnetlink_rcv+0x15/0x20
    [<00000000691b703b>] netlink_unicast+0x4e3/0x6c0
    [<00000000b5be0204>] netlink_sendmsg+0x7f2/0xba0
    [<0000000096d2aa60>] sock_sendmsg+0xba/0xf0
    [<000000008c1b786f>] __sys_sendto+0x1e4/0x330
    [<0000000019587b3f>] __x64_sys_sendto+0xe1/0x1a0
    [<00000000071f4d56>] do_syscall_64+0x9f/0x300
    [<000000002737577f>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
    [<0000000057587684>] 0xffffffffffffffff

Fixes: c63586dc9b3e ("net: rtnl_dump_all needs to propagate error from dumpit function")
Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---

v2: * fix it by restoring the previous behavior as suggested by David

    * adjust subject and commit description

 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index e01274b..33d9227 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3367,7 +3367,7 @@ static int rtnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb)
 			cb->seq = 0;
 		}
 		ret = dumpit(skb, cb);
-		if (ret < 0)
+		if (ret)
 			break;
 	}
 	cb->family = idx;
-- 
1.8.3.1

[PATCH net v2 2/2] ipv6: properly check return value in inet6_dump_all()

[Alexey Kodanev]

Make sure we call fib6_dump_end() if it happens that skb->len
is zero. rtnl_dump_all() can reset cb->args on the next loop
iteration there.

Fixes: 08e814c9e8eb ("net/ipv6: Bail early if user only wants cloned entries")
Fixes: ae677bbb4441 ("net: Don't return invalid table id error when dumping all families")
Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---

v2: a new patch in v2

 net/ipv6/ip6_fib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 1b8bc00..ae37861 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -591,7 +591,7 @@ static int inet6_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
 
 	/* fib entries are never clones */
 	if (arg.filter.flags & RTM_F_CLONED)
-		return skb->len;
+		goto out;
 
 	w = (void *)cb->args[2];
 	if (!w) {
@@ -621,7 +621,7 @@ static int inet6_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
 		tb = fib6_get_table(net, arg.filter.table_id);
 		if (!tb) {
 			if (arg.filter.dump_all_families)
-				return skb->len;
+				goto out;
 
 			NL_SET_ERR_MSG_MOD(cb->extack, "FIB table does not exist");
 			return -ENOENT;
-- 
1.8.3.1

Re: [PATCH net v2 1/2] rtnetlink: restore handling of dumpit return value in rtnl_dump_all()

[David Miller]

From: Alexey Kodanev <alexey.kodanev@oracle.com>
Date: Fri,  2 Nov 2018 19:11:04 +0300

> For non-zero return from dumpit() we should break the loop
> in rtnl_dump_all() and return the result. Otherwise, e.g.,
> we could get the memory leak in inet6_dump_fib() [1]. The
> pointer to the allocated struct fib6_walker there (saved
> in cb->args) can be lost, reset on the next iteration.
> 
> Fix it by partially restoring the previous behavior before
> commit c63586dc9b3e ("net: rtnl_dump_all needs to propagate
> error from dumpit function"). The returned error from
> dumpit() is still passed further.
 ...
> Fixes: c63586dc9b3e ("net: rtnl_dump_all needs to propagate error from dumpit function")
> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>

Applied.

Re: [PATCH net v2 2/2] ipv6: properly check return value in inet6_dump_all()

[David Miller]

From: Alexey Kodanev <alexey.kodanev@oracle.com>
Date: Fri,  2 Nov 2018 19:11:05 +0300

> Make sure we call fib6_dump_end() if it happens that skb->len
> is zero. rtnl_dump_all() can reset cb->args on the next loop
> iteration there.
> 
> Fixes: 08e814c9e8eb ("net/ipv6: Bail early if user only wants cloned entries")
> Fixes: ae677bbb4441 ("net: Don't return invalid table id error when dumping all families")
> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>

Applied.

Re: [PATCH net v2 1/2] rtnetlink: restore handling of dumpit return value in rtnl_dump_all()

[David Ahern]

On 11/5/18 6:06 PM, David Miller wrote:
> From: Alexey Kodanev <alexey.kodanev@oracle.com>
> Date: Fri,  2 Nov 2018 19:11:04 +0300
> 
>> For non-zero return from dumpit() we should break the loop
>> in rtnl_dump_all() and return the result. Otherwise, e.g.,
>> we could get the memory leak in inet6_dump_fib() [1]. The
>> pointer to the allocated struct fib6_walker there (saved
>> in cb->args) can be lost, reset on the next iteration.
>>
>> Fix it by partially restoring the previous behavior before
>> commit c63586dc9b3e ("net: rtnl_dump_all needs to propagate
>> error from dumpit function"). The returned error from
>> dumpit() is still passed further.
>  ...
>> Fixes: c63586dc9b3e ("net: rtnl_dump_all needs to propagate error from dumpit function")
>> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
> 
> Applied.
> 

Lost track of these in the noise of the past few days.

Thanks for the fixes Alexey.