syzbot reported[1] a use-after-free when deleting an expired fdb. It is due to a race condition between learning still happening and a port being deleted, after all its fdbs have been flushed. The port's state has been toggled to disabled so no learning should happen at that time, but if we have MST enabled, it will bypass the port's state, that together with VLAN filtering disabled can lead to fdb learning at a time when it shouldn't happen while the port is being deleted. VLAN filtering must be disabled because we flush the port VLANs when it's being deleted which will stop learning. This fix avoids adding more checks in the fast-path and instead toggles the MST static branch when changing VLAN filtering which effectively disables MST when VLAN filtering gets disabled, and re-enables it when VLAN filtering is enabled && MST is enabled as well. [1] https://syzkaller.appspot.com/bug?extid=dd280197f0f7ab3917be Fixes: ec7328b59176 ("net: bridge: mst: Multiple Spanning Tree (MST) mode") Reported-by: syzbot+dd280197f0f7ab3917be@syzkaller.appspotmail.com Closes: https://lore.kernel.org/netdev/69088ffa.050a0220.29fc44.003d.GAE@google.com/ Signed-off-by: Nikolay Aleksandrov --- Initially I did look into moving the rx handler registration/unregistration but that is much more difficult and error-prone. This solution seems like the cleanest approach that doesn't affect the fast-path. net/bridge/br_mst.c | 18 +++++++++++++----- net/bridge/br_private.h | 5 +++++ net/bridge/br_vlan.c | 1 + 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/net/bridge/br_mst.c b/net/bridge/br_mst.c index 3f24b4ee49c2..4abf8551290f 100644 --- a/net/bridge/br_mst.c +++ b/net/bridge/br_mst.c @@ -194,6 +194,18 @@ void br_mst_vlan_init_state(struct net_bridge_vlan *v) v->state = v->port->state; } +void br_mst_static_branch_toggle(struct net_bridge *br) +{ + /* enable the branch only if both VLAN filtering and MST are enabled + * otherwise port state bypass can lead to learning race conditions + */ + if (br_opt_get(br, BROPT_VLAN_ENABLED) && + br_opt_get(br, BROPT_MST_ENABLED)) + static_branch_enable(&br_mst_used); + else + static_branch_disable(&br_mst_used); +} + int br_mst_set_enabled(struct net_bridge *br, bool on, struct netlink_ext_ack *extack) { @@ -224,11 +236,7 @@ int br_mst_set_enabled(struct net_bridge *br, bool on, if (err && err != -EOPNOTSUPP) return err; - if (on) - static_branch_enable(&br_mst_used); - else - static_branch_disable(&br_mst_used); - + br_mst_static_branch_toggle(br); br_opt_toggle(br, BROPT_MST_ENABLED, on); return 0; } diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 16be5d250402..052bea4b3c06 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -1952,6 +1952,7 @@ int br_mst_fill_info(struct sk_buff *skb, const struct net_bridge_vlan_group *vg); int br_mst_process(struct net_bridge_port *p, const struct nlattr *mst_attr, struct netlink_ext_ack *extack); +void br_mst_static_branch_toggle(struct net_bridge *br); #else static inline bool br_mst_is_enabled(struct net_bridge *br) { @@ -1987,6 +1988,10 @@ static inline int br_mst_process(struct net_bridge_port *p, { return -EOPNOTSUPP; } + +static inline void br_mst_static_branch_toggle(struct net_bridge *br) +{ +} #endif struct nf_br_ops { diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index ce72b837ff8e..636d11f932eb 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -911,6 +911,7 @@ int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val, br_manage_promisc(br); recalculate_group_addr(br); br_recalculate_fwd_mask(br); + br_mst_static_branch_toggle(br); if (!val && br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED)) { br_info(br, "vlan filtering disabled, automatically disabling multicast vlan snooping\n"); br_multicast_toggle_vlan_snooping(br, false, NULL); -- 2.51.0 Add a test which checks that port state bypass cannot happen if we have VLAN filtering disabled and MST enabled. Such bypass could lead to race condition when deleting a port because learning may happen after its state has been toggled to disabled while it's being deleted, leading to a use after free. Signed-off-by: Nikolay Aleksandrov --- .../net/forwarding/bridge_vlan_unaware.sh | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/net/forwarding/bridge_vlan_unaware.sh b/tools/testing/selftests/net/forwarding/bridge_vlan_unaware.sh index 2b5700b61ffa..20769793310e 100755 --- a/tools/testing/selftests/net/forwarding/bridge_vlan_unaware.sh +++ b/tools/testing/selftests/net/forwarding/bridge_vlan_unaware.sh @@ -1,7 +1,7 @@ #!/bin/bash # SPDX-License-Identifier: GPL-2.0 -ALL_TESTS="ping_ipv4 ping_ipv6 learning flooding pvid_change" +ALL_TESTS="ping_ipv4 ping_ipv6 learning flooding pvid_change mst_state_no_bypass" NUM_NETIFS=4 source lib.sh @@ -114,6 +114,39 @@ pvid_change() ping_ipv6 " with bridge port $swp1 PVID deleted" } +mst_state_no_bypass() +{ + local mac=de:ad:be:ef:13:37 + + # Test that port state isn't bypassed when MST is enabled and VLAN + # filtering is disabled + RET=0 + + # MST can be enabled only when there are no VLANs + bridge vlan del vid 1 dev $swp1 + bridge vlan del vid 1 dev $swp2 + bridge vlan del vid 1 dev br0 self + + ip link set br0 type bridge mst_enabled 1 + check_err $? "Could not enable MST" + + bridge link set dev $swp1 state disabled + check_err $? "Could not set port state" + + $MZ $h1 -c 1 -p 64 -a $mac -t ip -q + + bridge fdb show brport $swp1 | grep -q de:ad:be:ef:13:37 + check_fail $? "FDB entry found when it shouldn't be" + + log_test "VLAN filtering disabled and MST enabled port state no bypass" + + ip link set br0 type bridge mst_enabled 0 + bridge link set dev $swp1 state forwarding + bridge vlan add vid 1 dev $swp1 pvid untagged + bridge vlan add vid 1 dev $swp2 pvid untagged + bridge vlan add vid 1 dev br0 self +} + trap cleanup EXIT setup_prepare -- 2.51.0