Re: [Devel] [PATCH vz7] ms/bridge: stp: Fix a potential use-after-free when deleting a bridge

[Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>]

On 9/11/26 18:10, Konstantin Khorenko wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> The three STP timers are not supposed to be armed while the bridge is
> administratively down. They are synchronously deactivated when the
> bridge is put administratively down and the various call sites check for
> 'IFF_UP' before arming them.
> 
> This check is missing from br_topology_change_detection() and it is
> possible to engineer a situation in which the topology change timer is
> armed while the bridge is administratively down, resulting in a
> use-after-free [1] when the bridge is deleted.
> 
> Fix by adding the missing check and for good measures synchronously
> shutdown the three timers when the bridge is deleted.
> 
> [1]
> ODEBUG: free active (active state 0) object: ffff88811662b9b0 object type: timer_list hint: br_topology_change_timer_expired (net/bridge/br_stp_timer.c:120)
> WARNING: lib/debugobjects.c:629 at debug_print_object+0x1bc/0x450, CPU#9: ip/359
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Noam Rathaus <noamr@ssd-disclosure.com>
> Reported-by: Neil Young <contact@ssd-disclosure.com>
> Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> Reviewed-by: Breno Leitao <leitao@debian.org>
> Link: https://patch.msgid.link/20260629072117.497959-1-idosch@nvidia.com
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> (cherry picked from commit 2a00517db8de4be7df3d483b215c5544fb30a191)
> Changes:
>   - s/timer_shutdown_sync/del_timer_sync/
> 
> timer_shutdown_sync() came with ms commit f571faf6e443 ("timers: Provide
> timer_shutdown[_sync]()") in v6.2, so it does not exist here. There is no
> stable backport to copy from either: 5.15.y and 6.1.y took this patch
> unchanged only because that timer API had been backported into them first,
> and 5.10.y got neither.
> 
> del_timer_sync() is enough at this call site. Both functions dequeue the
> timer and wait for a handler already running on another CPU, so on return
> the timer is neither queued nor running and the following free_netdev() no
> longer frees a live timer_list. The one thing timer_shutdown_sync() adds
> is clearing timer->function, which turns a later re-arm into a no-op, and
> nothing can re-arm these timers by the time br_dev_delete() reaches them:
> 
>   - del_nbp() has already unregistered the rx handler of every port, so
>     no BPDU reaches the bridge;
>   - the remaining arming sites, br_become_root_bridge(),
>     br_received_config_bpdu(), br_topology_change_detection(),
>     br_stp_enable_bridge() and br_stp_start(), run either from that rx
>     path or from ndo_open/ioctl under RTNL, which br_dev_delete() holds;
>   - the handlers themselves are stopped by del_timer_sync().
> 
> br_stp_disable_bridge() already uses del_timer_sync() on the same three
> timers, on a path where the bridge survives and they can legitimately be
> armed again.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-145092
> 
> Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>

Reviewed-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

> ---
>   net/bridge/br_if.c  | 3 +++
>   net/bridge/br_stp.c | 3 ++-
>   2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 494293c999c20..8c9d2f0dd055e 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -311,6 +311,9 @@ void br_dev_delete(struct net_device *dev, struct list_head *head)
>   
>   	br_fdb_delete_by_port(br, NULL, 0, 1);
>   
> +	del_timer_sync(&br->hello_timer);
> +	del_timer_sync(&br->topology_change_timer);
> +	del_timer_sync(&br->tcn_timer);
>   	cancel_delayed_work_sync(&br->gc_work);
>   
>   	br_sysfs_delbr(br->dev);
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index 8f56c2d1f1a70..65b10d3c968b6 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -337,7 +337,8 @@ void br_topology_change_detection(struct net_bridge *br)
>   {
>   	int isroot = br_is_root_bridge(br);
>   
> -	if (br->stp_enabled != BR_KERNEL_STP)
> +	if (br->stp_enabled != BR_KERNEL_STP ||
> +	    !(br->dev->flags & IFF_UP))
>   		return;
>   
>   	br_info(br, "topology change detected, %s\n",

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel