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: <104f12e4-866b-b986-cb9d-28c40d5c5e84@gmail.com>
Date:   Wed, 31 Oct 2018 10:55:35 -0600
From:   David Ahern <dsahern@...il.com>
To:     Alexey Kodanev <alexey.kodanev@...cle.com>, netdev@...r.kernel.org
Cc:     David Miller <davem@...emloft.net>
Subject: Re: [PATCH net] rtnetlink: invoke 'cb->done' destructor before
 'cb->args' reset

On 10/31/18 12:42 AM, 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].
> 
> 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.
> 
...

It is more efficient to keep going.

I think the simplest fix for 4.20 is to break the loop if ret is non-0 -
restore the previous behavior. For net-next I think the done callback is
not needed for ipv6; I think there is a simpler way to do it.