[PATCH net] net: openvswitch: fix flow mask use-after-free on flow deletion

[PATCH net] net: openvswitch: fix flow mask use-after-free on flow deletion

[Ilya Maximets]

The commit in the Fixes tag below made so flow->mask free is scheduled
via RCU right after it is removed from the flow table.  The pointer
stays in the flow structure and it can be accessible while in the same
RCU critical section.  This is done to avoid requiring ovs_mutex for
the ovs_flow_free().

However, while removing the flow during processing of CMD_DEL, we do
not take RCU read lock before the removal, and ovs_flow_cmd_fill_info()
uses the flow->mask pointer afterwards.  The RCU read lock is taken,
but it's already late at that point.  The comment on that line
acknowledges that the lock is cosmetic and doesn't serve a real purpose.

This leads to use-after-free if the RCU grace period passes between
removal and the filling.  It is a short race window, but it is there
and can lead to a real crash in case memory allocation for the info
takes a bit longer:

 BUG: KASAN: slab-use-after-free in __ovs_nla_put_key
             net/openvswitch/flow_netlink.c:1996
 BUG: KASAN: slab-use-after-free in ovs_nla_put_key+0x2463/0x2e30
             net/openvswitch/flow_netlink.c:2250
 Read of size 4 at addr ffff88801ee89970 by task ovs_flow_del_ec/9487

 Call Trace:
  <TASK>
  __ovs_nla_put_key net/openvswitch/flow_netlink.c:1996
  ovs_nla_put_key+0x2463/0x2e30 net/openvswitch/flow_netlink.c:2250
  ovs_flow_cmd_fill_info+0x420/0x9c0 net/openvswitch/datapath.c:930
  ovs_flow_cmd_del+0x53a/0x970 net/openvswitch/datapath.c:1467
  ...
  netlink_rcv_skb+0x156/0x420 net/netlink/af_netlink.c:2556
  </TASK>

 Allocated by task 9487:
  mask_alloc net/openvswitch/flow_table.c:967
  flow_mask_insert net/openvswitch/flow_table.c:1012
  ovs_flow_tbl_insert+0xea2/0x1a90 net/openvswitch/flow_table.c:1084
  ovs_flow_cmd_new+0x7e3/0xd90 net/openvswitch/datapath.c:1086
  ...
  netlink_rcv_skb+0x156/0x420 net/netlink/af_netlink.c:2556

 Freed by task 9485:
  rcu_free_sheaf+0x1e/0x100 mm/slub.c:5978
  rcu_do_batch kernel/rcu/tree.c:2645
  rcu_core+0x59c/0x10c0 kernel/rcu/tree.c:2897
  handle_softirqs+0x1e4/0x9a0 kernel/softirq.c:622
  ...
  instr_sysvec_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1062

ovs_flow_tbl_remove() must be called after the ovs_flow_cmd_fill_info()
to avoid this race.  This also helps with cleaning up the forced cast
and the cosmetic RCU read lock.  Before the commit in the Fixes tag the
order did not matter as long as the flow object itself was not freed.

A wider RCU critical section could be another option, but we have a
GFP_KERNEL allocation in the way.

Reported by Trend Micro's Zero Day Initiative as ZDI-CAN-32042.

Fixes: 56c19868e115 ("openvswitch: Make flow mask removal symmetric.")
Cc: stable@vger.kernel.org
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 net/openvswitch/datapath.c | 45 +++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index ae69b2cabab9..ded46d993a4e 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1473,33 +1473,34 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
 		goto unlock;
 	}
 
-	ovs_flow_tbl_remove(&dp->table, flow);
-	ovs_unlock();
-
-	reply = ovs_flow_cmd_alloc_info((const struct sw_flow_actions __force *) flow->sf_acts,
+	reply = ovs_flow_cmd_alloc_info(ovsl_dereference(flow->sf_acts),
 					&flow->id, info, false, ufid_flags);
-	if (likely(reply)) {
-		if (!IS_ERR(reply)) {
-			rcu_read_lock();	/*To keep RCU checker happy. */
-			err = ovs_flow_cmd_fill_info(flow, ovs_header->dp_ifindex,
-						     reply, info->snd_portid,
-						     info->snd_seq, 0,
-						     OVS_FLOW_CMD_DEL,
-						     ufid_flags);
-			rcu_read_unlock();
-			if (WARN_ON_ONCE(err < 0)) {
-				kfree_skb(reply);
-				goto out_free;
-			}
+	if (IS_ERR(reply)) {
+		netlink_set_err(sock_net(skb->sk)->genl_sock, 0, 0,
+				PTR_ERR(reply));
+		reply = NULL;
+	}
 
-			ovs_notify(&dp_flow_genl_family, reply, info);
-		} else {
-			netlink_set_err(sock_net(skb->sk)->genl_sock, 0, 0,
-					PTR_ERR(reply));
+	if (likely(reply)) {
+		err = ovs_flow_cmd_fill_info(flow, ovs_header->dp_ifindex,
+					     reply, info->snd_portid,
+					     info->snd_seq, 0,
+					     OVS_FLOW_CMD_DEL, ufid_flags);
+		if (WARN_ON_ONCE(err < 0)) {
+			kfree_skb(reply);
+			reply = NULL;
 		}
 	}
+	/* Removal has to happen after ovs_flow_cmd_fill_info(), as it uses
+	 * the flow->mask that can be scheduled to be freed by the
+	 * ovs_flow_tbl_remove() and we're not holding the RCU read lock.
+	 */
+	ovs_flow_tbl_remove(&dp->table, flow);
+	ovs_unlock();
+
+	if (likely(reply))
+		ovs_notify(&dp_flow_genl_family, reply, info);
 
-out_free:
 	ovs_flow_free(flow, true);
 	return 0;
 unlock:
-- 
2.55.0

Re: [PATCH net] net: openvswitch: fix flow mask use-after-free on flow deletion

[Aaron Conole]

Ilya Maximets <i.maximets@ovn.org> writes:

> The commit in the Fixes tag below made so flow->mask free is scheduled
> via RCU right after it is removed from the flow table.  The pointer
> stays in the flow structure and it can be accessible while in the same
> RCU critical section.  This is done to avoid requiring ovs_mutex for
> the ovs_flow_free().
>
> However, while removing the flow during processing of CMD_DEL, we do
> not take RCU read lock before the removal, and ovs_flow_cmd_fill_info()
> uses the flow->mask pointer afterwards.  The RCU read lock is taken,
> but it's already late at that point.  The comment on that line
> acknowledges that the lock is cosmetic and doesn't serve a real purpose.
>
> This leads to use-after-free if the RCU grace period passes between
> removal and the filling.  It is a short race window, but it is there
> and can lead to a real crash in case memory allocation for the info
> takes a bit longer:
>
>  BUG: KASAN: slab-use-after-free in __ovs_nla_put_key
>              net/openvswitch/flow_netlink.c:1996
>  BUG: KASAN: slab-use-after-free in ovs_nla_put_key+0x2463/0x2e30
>              net/openvswitch/flow_netlink.c:2250
>  Read of size 4 at addr ffff88801ee89970 by task ovs_flow_del_ec/9487
>
>  Call Trace:
>   <TASK>
>   __ovs_nla_put_key net/openvswitch/flow_netlink.c:1996
>   ovs_nla_put_key+0x2463/0x2e30 net/openvswitch/flow_netlink.c:2250
>   ovs_flow_cmd_fill_info+0x420/0x9c0 net/openvswitch/datapath.c:930
>   ovs_flow_cmd_del+0x53a/0x970 net/openvswitch/datapath.c:1467
>   ...
>   netlink_rcv_skb+0x156/0x420 net/netlink/af_netlink.c:2556
>   </TASK>
>
>  Allocated by task 9487:
>   mask_alloc net/openvswitch/flow_table.c:967
>   flow_mask_insert net/openvswitch/flow_table.c:1012
>   ovs_flow_tbl_insert+0xea2/0x1a90 net/openvswitch/flow_table.c:1084
>   ovs_flow_cmd_new+0x7e3/0xd90 net/openvswitch/datapath.c:1086
>   ...
>   netlink_rcv_skb+0x156/0x420 net/netlink/af_netlink.c:2556
>
>  Freed by task 9485:
>   rcu_free_sheaf+0x1e/0x100 mm/slub.c:5978
>   rcu_do_batch kernel/rcu/tree.c:2645
>   rcu_core+0x59c/0x10c0 kernel/rcu/tree.c:2897
>   handle_softirqs+0x1e4/0x9a0 kernel/softirq.c:622
>   ...
>   instr_sysvec_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1062
>
> ovs_flow_tbl_remove() must be called after the ovs_flow_cmd_fill_info()
> to avoid this race.  This also helps with cleaning up the forced cast
> and the cosmetic RCU read lock.  Before the commit in the Fixes tag the
> order did not matter as long as the flow object itself was not freed.
>
> A wider RCU critical section could be another option, but we have a
> GFP_KERNEL allocation in the way.
>
> Reported by Trend Micro's Zero Day Initiative as ZDI-CAN-32042.
>
> Fixes: 56c19868e115 ("openvswitch: Make flow mask removal symmetric.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Reviewed-by: Aaron Conole <aconole@redhat.com>

Re: [PATCH net] net: openvswitch: fix flow mask use-after-free on flow deletion

[Ilya Maximets]

On 8/15/26 2:58 AM, Ilya Maximets wrote:
> The commit in the Fixes tag below made so flow->mask free is scheduled
> via RCU right after it is removed from the flow table.  The pointer
> stays in the flow structure and it can be accessible while in the same
> RCU critical section.  This is done to avoid requiring ovs_mutex for
> the ovs_flow_free().
> 
> However, while removing the flow during processing of CMD_DEL, we do
> not take RCU read lock before the removal, and ovs_flow_cmd_fill_info()
> uses the flow->mask pointer afterwards.  The RCU read lock is taken,
> but it's already late at that point.  The comment on that line
> acknowledges that the lock is cosmetic and doesn't serve a real purpose.
> 
> This leads to use-after-free if the RCU grace period passes between
> removal and the filling.  It is a short race window, but it is there
> and can lead to a real crash in case memory allocation for the info
> takes a bit longer:
> 
>  BUG: KASAN: slab-use-after-free in __ovs_nla_put_key
>              net/openvswitch/flow_netlink.c:1996
>  BUG: KASAN: slab-use-after-free in ovs_nla_put_key+0x2463/0x2e30
>              net/openvswitch/flow_netlink.c:2250
>  Read of size 4 at addr ffff88801ee89970 by task ovs_flow_del_ec/9487
> 
>  Call Trace:
>   <TASK>
>   __ovs_nla_put_key net/openvswitch/flow_netlink.c:1996
>   ovs_nla_put_key+0x2463/0x2e30 net/openvswitch/flow_netlink.c:2250
>   ovs_flow_cmd_fill_info+0x420/0x9c0 net/openvswitch/datapath.c:930
>   ovs_flow_cmd_del+0x53a/0x970 net/openvswitch/datapath.c:1467
>   ...
>   netlink_rcv_skb+0x156/0x420 net/netlink/af_netlink.c:2556
>   </TASK>
> 
>  Allocated by task 9487:
>   mask_alloc net/openvswitch/flow_table.c:967
>   flow_mask_insert net/openvswitch/flow_table.c:1012
>   ovs_flow_tbl_insert+0xea2/0x1a90 net/openvswitch/flow_table.c:1084
>   ovs_flow_cmd_new+0x7e3/0xd90 net/openvswitch/datapath.c:1086
>   ...
>   netlink_rcv_skb+0x156/0x420 net/netlink/af_netlink.c:2556
> 
>  Freed by task 9485:
>   rcu_free_sheaf+0x1e/0x100 mm/slub.c:5978
>   rcu_do_batch kernel/rcu/tree.c:2645
>   rcu_core+0x59c/0x10c0 kernel/rcu/tree.c:2897
>   handle_softirqs+0x1e4/0x9a0 kernel/softirq.c:622
>   ...
>   instr_sysvec_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1062
> 
> ovs_flow_tbl_remove() must be called after the ovs_flow_cmd_fill_info()
> to avoid this race.  This also helps with cleaning up the forced cast
> and the cosmetic RCU read lock.  Before the commit in the Fixes tag the
> order did not matter as long as the flow object itself was not freed.
> 
> A wider RCU critical section could be another option, but we have a
> GFP_KERNEL allocation in the way.
> 
> Reported by Trend Micro's Zero Day Initiative as ZDI-CAN-32042.
> 
> Fixes: 56c19868e115 ("openvswitch: Make flow mask removal symmetric.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
Sashiko complains:

> Does this reordering drop the final packet and byte counts for packets
> that hit the flow between the snapshot and the unlink?
> ovs_flow_cmd_fill_info() -> ovs_flow_cmd_fill_stats() -> ovs_flow_stats_get()
> now serializes the counters while the flow is still linked in dp->table,
> and ovs_flow_tbl_remove() only runs afterwards.  The datapath writer runs
> in softirq context and takes only the per-CPU stats lock, never ovs_mutex:
> net/openvswitch/flow.c:ovs_flow_stats_update() {
> 	...
> 	stats = rcu_dereference(flow->stats[cpu]);
> 	/* Check if already have CPU-specific stats. */
> 	if (likely(stats)) {
> 		spin_lock(&stats->lock);
> 	...
> 	stats->packet_count++;
> 	stats->byte_count += len;
> 	...
> }
> So ovs_dp_process_packet() -> ovs_flow_tbl_lookup_stats() still finds the
> flow and bumps flow->stats[cpu] during that window.  Those increments are
> then discarded by:
> 	ovs_flow_free(flow, true);
> Since OVS_FLOW_ATTR_STATS in the DEL reply/notification is the last place
> user space can collect a flow's final counters, would those packets be
> lost from accounting?  Before the patch the unlink preceded the snapshot,
> so no new lookup could match the flow after the counters were read.
> The window here is bounded by the remaining nla_put work in
> ovs_flow_cmd_fill_actions() plus any preemption of the deleting task, not
> by a sleeping allocation, since ovs_flow_cmd_alloc_info() with GFP_KERNEL
> now runs before ovs_flow_cmd_fill_info().  Would it be worth mentioning
> this trade-off in the commit message?

This is not a new issue.  The race window is a bit different, but it was
there before the change.  The datapath processing is only protected by RCU
and we're not synchronizing it between removal and reading the stats.
So, there will always be a chance to not account for some of the packets.

That said, this is also not a concern for any real setup as ovs-vswitchd
doesn't delete active flows under normal circumstances.

Best regards, Ilya Maximets.

Re: [PATCH net] net: openvswitch: fix flow mask use-after-free on flow deletion

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sat, 15 Aug 2026 02:58:56 +0200 you wrote:
> The commit in the Fixes tag below made so flow->mask free is scheduled
> via RCU right after it is removed from the flow table.  The pointer
> stays in the flow structure and it can be accessible while in the same
> RCU critical section.  This is done to avoid requiring ovs_mutex for
> the ovs_flow_free().
> 
> However, while removing the flow during processing of CMD_DEL, we do
> not take RCU read lock before the removal, and ovs_flow_cmd_fill_info()
> uses the flow->mask pointer afterwards.  The RCU read lock is taken,
> but it's already late at that point.  The comment on that line
> acknowledges that the lock is cosmetic and doesn't serve a real purpose.
> 
> [...]

Here is the summary with links:
  - [net] net: openvswitch: fix flow mask use-after-free on flow deletion
    https://git.kernel.org/netdev/net/c/4e30317ff67a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html