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

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

[Konstantin Khorenko]

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>
---
 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",
-- 
2.43.0

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

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

[Vladimir Riabchun]

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

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

[Vasileios Almpanis]

On 9/11/26 6:10 PM, 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.
>
> [...]

Reviewed-by: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>

-- 
Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>


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