netdev - Re: [PATCH net v2] net: bridge: fix early call to br_stp_change_bridge

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20171218.133107.1225923645891511984.davem@davemloft.net>
Date:   Mon, 18 Dec 2017 13:31:07 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     nikolay@...ulusnetworks.com
Cc:     netdev@...r.kernel.org, makita.toshiaki@....ntt.co.jp,
        roopa@...ulusnetworks.com, stephen@...workplumber.org,
        avagin@...il.com, bridge@...ts.linux-foundation.org,
        idosch@...lanox.com
Subject: Re: [PATCH net v2] net: bridge: fix early call to
 br_stp_change_bridge_id and plug newlink leaks

From: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
Date: Mon, 18 Dec 2017 17:35:09 +0200

> The early call to br_stp_change_bridge_id in bridge's newlink can cause
> a memory leak if an error occurs during the newlink because the fdb
> entries are not cleaned up if a different lladdr was specified, also
> another minor issue is that it generates fdb notifications with
> ifindex = 0. Another unrelated memory leak is the bridge sysfs entries
> which get added on NETDEV_REGISTER event, but are not cleaned up in the
> newlink error path. To remove this special case the call to
> br_stp_change_bridge_id is done after netdev register and we cleanup the
> bridge on changelink error via br_dev_delete to plug all leaks.
> 
> This patch makes netlink bridge destruction on newlink error the same as
> dellink and ioctl del which is necessary since at that point we have a
> fully initialized bridge device.
> 
> To reproduce the issue:
> $ ip l add br0 address 00:11:22:33:44:55 type bridge group_fwd_mask 1
> RTNETLINK answers: Invalid argument
> 
> $ rmmod bridge
> [ 1822.142525] =============================================================================
> [ 1822.143640] BUG bridge_fdb_cache (Tainted: G           O    ): Objects remaining in bridge_fdb_cache on __kmem_cache_shutdown()
> [ 1822.144821] -----------------------------------------------------------------------------
 ...
> Fixes: 30313a3d5794 ("bridge: Handle IFLA_ADDRESS correctly when creating bridge device")
> Fixes: 5b8d5429daa0 ("bridge: netlink: register netdevice before executing changelink")
> Signed-off-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
> ---
> v2: it's better to use br_dev_delete to clean up because the sysfs entries
> were leaked as well, basically we use it for the bridge fdb flush and sysfs
> entries delete
> 
> Consequently this also would fix the null ptr deref due to the rhashtable
> not being initialized in net-next when br_stp_change_bridge_id is called.

Applied, but doing state changes after register_netdev() is always dicey.

I hope things go well if the device is brought up immediately during
that register_netdev() call and before the stp bridge ID setting is
done.