* [PATCH net 0/2] net: bridge: fix use-after-free due to MST port state bypass
@ 2025-11-04 12:03 Nikolay Aleksandrov
2025-11-04 12:03 ` [PATCH net 1/2] " Nikolay Aleksandrov
2025-11-04 12:03 ` [PATCH net 2/2] selftests: forwarding: bridge: add a state bypass with disabled VLAN filtering test Nikolay Aleksandrov
0 siblings, 2 replies; 8+ messages in thread
From: Nikolay Aleksandrov @ 2025-11-04 12:03 UTC (permalink / raw)
To: netdev
Cc: tobias, idosch, kuba, davem, bridge, pabeni, edumazet, horms,
Nikolay Aleksandrov
Hi,
Patch 01 fixes a race condition that exists between expired fdb deletion
and port deletion when MST is enabled. Learning can happen after the
port's state has been changed to disabled which could lead to that
port's memory being used after it's been freed. The issue was reported
by syzbot, more information in patch 01. Patch 02 adds a selftest to
make sure port state bypass doesn't happen when we have VLAN filtering
disabled, regardless of MST state.
Thanks,
Nik
Nikolay Aleksandrov (2):
net: bridge: fix use-after-free due to MST port state bypass
selftests: forwarding: bridge: add a state bypass with disabled VLAN
filtering test
net/bridge/br_mst.c | 18 +++++++---
net/bridge/br_private.h | 5 +++
net/bridge/br_vlan.c | 1 +
.../net/forwarding/bridge_vlan_unaware.sh | 35 ++++++++++++++++++-
4 files changed, 53 insertions(+), 6 deletions(-)
--
2.51.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net 1/2] net: bridge: fix use-after-free due to MST port state bypass
2025-11-04 12:03 [PATCH net 0/2] net: bridge: fix use-after-free due to MST port state bypass Nikolay Aleksandrov
@ 2025-11-04 12:03 ` Nikolay Aleksandrov
2025-11-04 12:21 ` Nikolay Aleksandrov
2025-11-05 8:44 ` Ido Schimmel
2025-11-04 12:03 ` [PATCH net 2/2] selftests: forwarding: bridge: add a state bypass with disabled VLAN filtering test Nikolay Aleksandrov
1 sibling, 2 replies; 8+ messages in thread
From: Nikolay Aleksandrov @ 2025-11-04 12:03 UTC (permalink / raw)
To: netdev
Cc: tobias, idosch, kuba, davem, bridge, pabeni, edumazet, horms,
Nikolay Aleksandrov, syzbot+dd280197f0f7ab3917be
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 <razor@blackwall.org>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net 2/2] selftests: forwarding: bridge: add a state bypass with disabled VLAN filtering test
2025-11-04 12:03 [PATCH net 0/2] net: bridge: fix use-after-free due to MST port state bypass Nikolay Aleksandrov
2025-11-04 12:03 ` [PATCH net 1/2] " Nikolay Aleksandrov
@ 2025-11-04 12:03 ` Nikolay Aleksandrov
2025-11-04 17:15 ` Petr Machata
1 sibling, 1 reply; 8+ messages in thread
From: Nikolay Aleksandrov @ 2025-11-04 12:03 UTC (permalink / raw)
To: netdev
Cc: tobias, idosch, kuba, davem, bridge, pabeni, edumazet, horms,
Nikolay Aleksandrov
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 <razor@blackwall.org>
---
.../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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net 1/2] net: bridge: fix use-after-free due to MST port state bypass
2025-11-04 12:03 ` [PATCH net 1/2] " Nikolay Aleksandrov
@ 2025-11-04 12:21 ` Nikolay Aleksandrov
2025-11-05 8:44 ` Ido Schimmel
1 sibling, 0 replies; 8+ messages in thread
From: Nikolay Aleksandrov @ 2025-11-04 12:21 UTC (permalink / raw)
To: netdev
Cc: tobias, idosch, kuba, davem, bridge, pabeni, edumazet, horms,
syzbot+dd280197f0f7ab3917be
On 11/4/25 14:03, Nikolay Aleksandrov wrote:
> 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 <razor@blackwall.org>
> ---
> 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)
> +{
> +}
oh, just realized this is actually unnecessary since MST relies on bridge vlan filtering
to be enabled, I will wait 24h for feedback and post v2 without this
> #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);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net 2/2] selftests: forwarding: bridge: add a state bypass with disabled VLAN filtering test
2025-11-04 12:03 ` [PATCH net 2/2] selftests: forwarding: bridge: add a state bypass with disabled VLAN filtering test Nikolay Aleksandrov
@ 2025-11-04 17:15 ` Petr Machata
2025-11-04 19:10 ` Nikolay Aleksandrov
0 siblings, 1 reply; 8+ messages in thread
From: Petr Machata @ 2025-11-04 17:15 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: netdev, tobias, idosch, kuba, davem, bridge, pabeni, edumazet,
horms
Nikolay Aleksandrov <razor@blackwall.org> writes:
> 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 <razor@blackwall.org>
> ---
> .../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"
I think you'll need to adjust this test for v2, can you please change
the above line to the following while at it?
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
Pretty sure these naked references will explode in the CI's shellcheck.
I expect they'll have to be quoted as "$swp1".
> + 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
Here as well. And more cases are below.
I've got this in my bash history. Might come in handy.
cat files | while read file; do git show net-next/main:$file > file; shellcheck file > sc-old; git show HEAD:$file > file; shellcheck file > sc-new; echo $file; diff -u sc-old sc-new; done | less
> + 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net 2/2] selftests: forwarding: bridge: add a state bypass with disabled VLAN filtering test
2025-11-04 17:15 ` Petr Machata
@ 2025-11-04 19:10 ` Nikolay Aleksandrov
0 siblings, 0 replies; 8+ messages in thread
From: Nikolay Aleksandrov @ 2025-11-04 19:10 UTC (permalink / raw)
To: Petr Machata
Cc: netdev, tobias, idosch, kuba, davem, bridge, pabeni, edumazet,
horms
On 11/4/25 19:15, Petr Machata wrote:
>
> Nikolay Aleksandrov <razor@blackwall.org> writes:
>
>> 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 <razor@blackwall.org>
>> ---
>> .../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"
>
> I think you'll need to adjust this test for v2, can you please change
> the above line to the following while at it?
>
ack
> 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
>
> Pretty sure these naked references will explode in the CI's shellcheck.
> I expect they'll have to be quoted as "$swp1".
>
Oh haven't written selftests in awhile, I've missed the shellcheck stuff. :)
>> + 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
>
> Here as well. And more cases are below.
>
> I've got this in my bash history. Might come in handy.
>
> cat files | while read file; do git show net-next/main:$file > file; shellcheck file > sc-old; git show HEAD:$file > file; shellcheck file > sc-new; echo $file; diff -u sc-old sc-new; done | less
>
Thanks for the hint. I'll add this to my patch review process as well.
Cheers,
Nik
>> + 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net 1/2] net: bridge: fix use-after-free due to MST port state bypass
2025-11-04 12:03 ` [PATCH net 1/2] " Nikolay Aleksandrov
2025-11-04 12:21 ` Nikolay Aleksandrov
@ 2025-11-05 8:44 ` Ido Schimmel
2025-11-05 9:25 ` Nikolay Aleksandrov
1 sibling, 1 reply; 8+ messages in thread
From: Ido Schimmel @ 2025-11-05 8:44 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: netdev, tobias, kuba, davem, bridge, pabeni, edumazet, horms,
syzbot+dd280197f0f7ab3917be
On Tue, Nov 04, 2025 at 02:03:12PM +0200, Nikolay Aleksandrov wrote:
> 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
Nice analysis!
>
> 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 <razor@blackwall.org>
> ---
> 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);
I believe the current code is buggy and these
static_branch_{enable,disable}() should be converted to
static_branch_{inc,dec}(). The static key is global and MST can be
enabled / disabled on multiple bridges, which makes this fix
problematic.
Can we instead clear BR_LEARNING from a port that is being deleted?
> +}
> +
> 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net 1/2] net: bridge: fix use-after-free due to MST port state bypass
2025-11-05 8:44 ` Ido Schimmel
@ 2025-11-05 9:25 ` Nikolay Aleksandrov
0 siblings, 0 replies; 8+ messages in thread
From: Nikolay Aleksandrov @ 2025-11-05 9:25 UTC (permalink / raw)
To: Ido Schimmel
Cc: netdev, tobias, kuba, davem, bridge, pabeni, edumazet, horms,
syzbot+dd280197f0f7ab3917be
On 11/5/25 10:44, Ido Schimmel wrote:
> On Tue, Nov 04, 2025 at 02:03:12PM +0200, Nikolay Aleksandrov wrote:
>> 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
>
> Nice analysis!
>
>>
>> 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 <razor@blackwall.org>
>> ---
>> 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);
>
> I believe the current code is buggy and these
> static_branch_{enable,disable}() should be converted to
> static_branch_{inc,dec}(). The static key is global and MST can be
> enabled / disabled on multiple bridges, which makes this fix
> problematic.
>
Indeed, multiple bridges could re-enable it. That code is buggy..
> Can we instead clear BR_LEARNING from a port that is being deleted?
>
Yeah, if done before the vlan flush (i.e. before synchronize_rcu), it should
work for this case, but I'd like to avoid bypassing port state altogether
because it could lead to other bugs in the future. I am tempted to make
MST enabled dependent on VLAN filtering enabled (as it should have been),
running with vlan filtering 0 and mst enabled 1 is nonsense anyway.
I think we should add one check when MST is enabled for port's vlan group,
if it is NULL then don't bypass its state since that means it is getting
deleted, that would prevent state bypass altogether.
>> +}
>> +
>> 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
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-11-05 9:26 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-04 12:03 [PATCH net 0/2] net: bridge: fix use-after-free due to MST port state bypass Nikolay Aleksandrov
2025-11-04 12:03 ` [PATCH net 1/2] " Nikolay Aleksandrov
2025-11-04 12:21 ` Nikolay Aleksandrov
2025-11-05 8:44 ` Ido Schimmel
2025-11-05 9:25 ` Nikolay Aleksandrov
2025-11-04 12:03 ` [PATCH net 2/2] selftests: forwarding: bridge: add a state bypass with disabled VLAN filtering test Nikolay Aleksandrov
2025-11-04 17:15 ` Petr Machata
2025-11-04 19:10 ` Nikolay Aleksandrov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).