netdev - Re: [PATCH net] rtnetlink: invoke 'cb->done' destructor before 'cb->args' reset

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: <dad66984-2187-3db3-c94e-32d3383f4af4@oracle.com>
Date:   Wed, 31 Oct 2018 09:42:28 +0300
From:   Alexey Kodanev <alexey.kodanev@...cle.com>
To:     netdev@...r.kernel.org
Cc:     David Ahern <dsahern@...il.com>, David Miller <davem@...emloft.net>
Subject: Re: [PATCH net] rtnetlink: invoke 'cb->done' destructor before
 'cb->args' reset

On 31.10.2018 09:42, Alexey Kodanev wrote:
> cb->args[2] can store the pointer to the struct fib6_walker,
> allocated in inet6_dump_fib(). On the next loop iteration in
> rtnl_dump_all(), 'memset(&cb, 0, sizeof(cb->args))' can reset
> that pointer, leaking the memory [1].
>

On the second thought we could as well save the state of fib6_walker
in inet6_dump_fib() with cb->data. That should fix the leak too.
Is it sounds reasonable?

Thanks,
Alexey

 
> Fix it by calling cb->done, if it is set, before filling 'cb->args'
> with zeros.
> 
> Looks like the recent changes in rtnl_dump_all() contributed to
> the appearance of this kmemleak [1], commit c63586dc9b3e ("net:
> rtnl_dump_all needs to propagate error from dumpit function")
> breaks the loop only on an error now.
> 
> [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: 1b43af5480c3 ("[IPV6]: Increase number of possible routing tables to 2^32")
> Signed-off-by: Alexey Kodanev <alexey.kodanev@...cle.com>
> ---
>  net/core/rtnetlink.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index f679c7a..314c683 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3362,6 +3362,8 @@ static int rtnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb)
>  			continue;
>  
>  		if (idx > s_idx) {
> +			if (cb->done)
> +				cb->done(cb);
>  			memset(&cb->args[0], 0, sizeof(cb->args));
>  			cb->prev_seq = 0;
>  			cb->seq = 0;
>