Message-ID: <db8450b0-35bb-485d-af33-3ed32a9e0845@blackwall.org>
Date: Wed, 5 Nov 2025 11:25:59 +0200
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Ido Schimmel <idosch@...dia.com>
Cc: netdev@...r.kernel.org, tobias@...dekranz.com, kuba@...nel.org,
 davem@...emloft.net, bridge@...ts.linux.dev, pabeni@...hat.com,
 edumazet@...gle.com, horms@...nel.org,
 syzbot+dd280197f0f7ab3917be@...kaller.appspotmail.com
Subject: Re: [PATCH net 1/2] net: bridge: fix use-after-free due to MST port
 state bypass

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@...kaller.appspotmail.com
>> Closes: https://lore.kernel.org/netdev/69088ffa.050a0220.29fc44.003d.GAE@google.com/
>> Signed-off-by: Nikolay Aleksandrov <razor@...ckwall.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
>>

Powered by blists - more mailing lists