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
| ||
|
Date: Mon, 06 Jan 2014 00:26:18 +0900 From: Toshiaki Makita <toshiaki.makita1@...il.com> To: vyasevic@...hat.com Cc: Toshiaki Makita <makita.toshiaki@....ntt.co.jp>, "David S . Miller" <davem@...emloft.net>, Stephen Hemminger <stephen@...workplumber.org>, netdev@...r.kernel.org Subject: Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote: > On 01/03/2014 02:28 PM, Vlad Yasevich wrote: > > On 12/17/2013 07:03 AM, Toshiaki Makita wrote: > >> br_fdb_changeaddr() assumes that there is at most one local entry per port > >> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow > >> creating/deleting fdb entries via netlink"), it has not been so. > >> Therefore, the function might fail to search a correct previous address > >> to be deleted and delete an arbitrary local entry if user has added local > >> entries manually. > >> > >> Example of problematic case: > >> ip link set eth0 address ee:ff:12:34:56:78 > >> brctl addif br0 eth0 > >> bridge fdb add 12:34:56:78:90:ab dev eth0 master > >> ip link set eth0 address aa:bb:cc:dd:ee:ff > >> Then, the address 12:34:56:78:90:ab might be deleted instead of > >> ee:ff:12:34:56:78, the original mac address of eth0. > >> > >> Address this issue by introducing a new flag, added_by_user, to struct > >> net_bridge_fdb_entry. > >> > >> Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case > >> like: > >> ip link set eth0 address 12:34:56:78:90:ab > >> ip link set eth1 address aa:bb:cc:dd:ee:ff > >> brctl addif br0 eth0 > >> bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master > >> brctl addif br0 eth1 > >> brctl delif br0 eth0 > >> In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff, > >> but it also should have been added by "brctl addif br0 eth1" originally, > >> so we don't delete it and treat it a new kernel-created entry. > >> > > > > I was looking over my patch series that adds something similar to this > > and noticed that you are not handing the NTF_USE case. That case was > > always troublesome for me as it allows for 2 different way to create > > the same FDB: one through br_fdb_update() and one through fdb_add_entry(). > > > > It is possible, though I haven't found any users yet, that NTF_USE > > may be used and in that case, bridge will create a dynamic fdb and > > disregard all NUD flags. In case case, add_by_user will not be set > > either. > > > > I think that the above is broken and plan to submit a fix shortly. > > Just looked again at my NTF_USE patch and while it seems ok, the whole > NTF_USE usage is racy to begin with and I am really starting to question > it's validity. > > Presently, br_fdb_update() will not update local fdb entries. Instead > it will log a misleading warning... It will only let you update > non-local entries. This is fine for user-created entries, but any > operation on dynamically created entries will only persist until > the next packet. It also races against the packet, so there is > absolutely no guarantee that the values of fdb->dst and fdb->updated > will be consistent.. > > It seems to me that the update capability of NTF_USE would actually be > of more value on local or user-created fdb entries. > > The fdb creation capability of NTF_USE should be disabled. > > Thoughts? I ignored NTF_USE in this patch because I regard it as emulating kernel creating entries after investigating git log. http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8 http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498 So I think NTF_USE shouldn't set added_by_user. And to emulate kernel creating entries, simply calling br_fdb_update() is the right way, isn't it? Thanks, Toshiaki Makita -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists