[PATCH net v3] net/mlx5e: macsec: fix use-after-free of metadata_dst on RX SC delete

[PATCH net v3] net/mlx5e: macsec: fix use-after-free of metadata_dst on RX SC delete

[Doruk Tan Ozturk]

When an offloaded MACsec RX SC is deleted, macsec_del_rxsc_ctx() released
the per-SC metadata_dst with metadata_dst_free(), which calls kfree()
unconditionally and ignores the dst reference count. The RX datapath in
mlx5e_macsec_offload_handle_rx_skb() looks up the SC under rcu_read_lock()
via xa_load() and, while still holding only the RCU read lock, takes a
reference with dst_hold() and attaches the dst to the skb with
skb_dst_set().

A reader that has already obtained the rx_sc pointer can therefore race
with the delete path:

  CPU0 (del_rxsc)			CPU1 (rx datapath)
  --------------			------------------
					rcu_read_lock();
					rx_sc = xa_load(...)->rx_sc;
  xa_erase(...);
  metadata_dst_free(rx_sc->md_dst);	/* kfree(), ignores refcount */
					dst_hold(&rx_sc->md_dst->dst); /* UAF */
					skb_dst_set(skb, &rx_sc->md_dst->dst);

metadata_dst_free() frees the object even though the datapath still holds
(or is about to take) a reference, so the subsequent dst_hold() /
skb_dst_set() and the later skb free operate on freed memory.

Fix the owner side by dropping the reference with dst_release() instead of
freeing unconditionally. dst_release() only schedules the RCU-deferred
dst_destroy() once the reference count reaches zero, so a concurrent reader
that still holds a reference keeps the object alive.

Dropping the owner reference is not sufficient on its own: once the owner
reference is the last one, dst_release() drops the count to zero and the
destroy is merely RCU-deferred. A racing reader that runs plain dst_hold()
on that already-dead dst gets rcuref_get() == false but dst_hold() only
WARNs and attaches the dying dst to the skb anyway; the later skb free then
calls dst_release() on an object whose destroy is already scheduled, again
a use-after-free.

Convert the RX datapath to dst_hold_safe(), which returns false (without
warning) when the dst is already dead, and only attach it to the skb when a
reference was successfully taken. When the SC is being deleted the in-flight
packet simply proceeds without the offload metadata_dst: skb_metadata_dst()
returns NULL, the MACsec core sees !is_macsec_md_dst and skips this secy
(rx_uses_md_dst path), which is the correct behaviour for a packet whose SC
is going away.

While reworking the datapath lookup, also guard the two NULL dereferences
on the same path that an automated review (forwarded by Simon Horman)
flagged: xa_load() can return NULL when the fs_id has just been erased, and
mlx5e_macsec_add_rxsc() publishes sc_xarray_element via xa_alloc() before
rx_sc->md_dst is allocated, so a packet carrying a freshly recycled fs_id
can observe a non-NULL rx_sc whose md_dst is still NULL. Check both before
dereferencing.

Note: macsec_del_rxsc_ctx() also kfree()s rx_sc->sc_xarray_element without
an RCU grace period while the same datapath reads it under rcu_read_lock();
that is a separate pre-existing issue and is left to a follow-up patch.

Fixes: b7c9400cbc48 ("net/mlx5e: Implement MACsec Rx data path using MACsec skb_metadata_dst")
Cc: stable@vger.kernel.org
Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
---
v3:
 - Also guard the RX-datapath NULL dereferences flagged by the automated
   review: NULL-check the xa_load() result and rx_sc->md_dst before use.
 - Note the unrelated non-RCU kfree(sc_xarray_element) in the delete path
   as a separate follow-up rather than folding it in here.
v2:
 - Convert the RX datapath dst_hold() to dst_hold_safe() so a reader racing
   the SC delete cannot attach a dst whose last reference was just dropped
   (per the automated review forwarded by Simon Horman).
v1: https://lore.kernel.org/netdev/20260615140534.52691-1-doruk@0sec.ai/

 .../net/ethernet/mellanox/mlx5/core/en_accel/macsec.c  | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index 71b3a05..fb2c64d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -829,7 +829,7 @@ static void macsec_del_rxsc_ctx(struct mlx5e_macsec *macsec, struct mlx5e_macsec
 	 */
 	list_del_rcu(&rx_sc->rx_sc_list_element);
 	xa_erase(&macsec->sc_xarray, rx_sc->sc_xarray_element->fs_id);
-	metadata_dst_free(rx_sc->md_dst);
+	dst_release(&rx_sc->md_dst->dst);
 	kfree(rx_sc->sc_xarray_element);
 	kfree_rcu_mightsleep(rx_sc);
 }
@@ -1695,10 +1695,10 @@ void mlx5e_macsec_offload_handle_rx_skb(struct net_device *netdev,
 
 	rcu_read_lock();
 	sc_xarray_element = xa_load(&macsec->sc_xarray, fs_id);
-	rx_sc = sc_xarray_element->rx_sc;
-	if (rx_sc) {
-		dst_hold(&rx_sc->md_dst->dst);
-		skb_dst_set(skb, &rx_sc->md_dst->dst);
+	rx_sc = sc_xarray_element ? sc_xarray_element->rx_sc : NULL;
+	if (rx_sc && rx_sc->md_dst) {
+		if (dst_hold_safe(&rx_sc->md_dst->dst))
+			skb_dst_set(skb, &rx_sc->md_dst->dst);
 	}
 
 	rcu_read_unlock();
-- 
2.53.0

Re: [PATCH net v3] net/mlx5e: macsec: fix use-after-free of metadata_dst on RX SC delete

[Simon Horman]

On Thu, Jun 18, 2026 at 04:55:45PM +0200, Doruk Tan Ozturk wrote:
> When an offloaded MACsec RX SC is deleted, macsec_del_rxsc_ctx() released
> the per-SC metadata_dst with metadata_dst_free(), which calls kfree()
> unconditionally and ignores the dst reference count. The RX datapath in
> mlx5e_macsec_offload_handle_rx_skb() looks up the SC under rcu_read_lock()
> via xa_load() and, while still holding only the RCU read lock, takes a
> reference with dst_hold() and attaches the dst to the skb with
> skb_dst_set().
> 
> A reader that has already obtained the rx_sc pointer can therefore race
> with the delete path:
> 
>   CPU0 (del_rxsc)			CPU1 (rx datapath)
>   --------------			------------------
> 					rcu_read_lock();
> 					rx_sc = xa_load(...)->rx_sc;
>   xa_erase(...);
>   metadata_dst_free(rx_sc->md_dst);	/* kfree(), ignores refcount */
> 					dst_hold(&rx_sc->md_dst->dst); /* UAF */
> 					skb_dst_set(skb, &rx_sc->md_dst->dst);
> 
> metadata_dst_free() frees the object even though the datapath still holds
> (or is about to take) a reference, so the subsequent dst_hold() /
> skb_dst_set() and the later skb free operate on freed memory.
> 
> Fix the owner side by dropping the reference with dst_release() instead of
> freeing unconditionally. dst_release() only schedules the RCU-deferred
> dst_destroy() once the reference count reaches zero, so a concurrent reader
> that still holds a reference keeps the object alive.
> 
> Dropping the owner reference is not sufficient on its own: once the owner
> reference is the last one, dst_release() drops the count to zero and the
> destroy is merely RCU-deferred. A racing reader that runs plain dst_hold()
> on that already-dead dst gets rcuref_get() == false but dst_hold() only
> WARNs and attaches the dying dst to the skb anyway; the later skb free then
> calls dst_release() on an object whose destroy is already scheduled, again
> a use-after-free.
> 
> Convert the RX datapath to dst_hold_safe(), which returns false (without
> warning) when the dst is already dead, and only attach it to the skb when a
> reference was successfully taken. When the SC is being deleted the in-flight
> packet simply proceeds without the offload metadata_dst: skb_metadata_dst()
> returns NULL, the MACsec core sees !is_macsec_md_dst and skips this secy
> (rx_uses_md_dst path), which is the correct behaviour for a packet whose SC
> is going away.
> 
> While reworking the datapath lookup, also guard the two NULL dereferences
> on the same path that an automated review (forwarded by Simon Horman)
> flagged: xa_load() can return NULL when the fs_id has just been erased, and
> mlx5e_macsec_add_rxsc() publishes sc_xarray_element via xa_alloc() before
> rx_sc->md_dst is allocated, so a packet carrying a freshly recycled fs_id
> can observe a non-NULL rx_sc whose md_dst is still NULL. Check both before
> dereferencing.
> 
> Note: macsec_del_rxsc_ctx() also kfree()s rx_sc->sc_xarray_element without
> an RCU grace period while the same datapath reads it under rcu_read_lock();
> that is a separate pre-existing issue and is left to a follow-up patch.
> 
> Fixes: b7c9400cbc48 ("net/mlx5e: Implement MACsec Rx data path using MACsec skb_metadata_dst")
> Cc: stable@vger.kernel.org
> Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
> ---
> v3:
>  - Also guard the RX-datapath NULL dereferences flagged by the automated
>    review: NULL-check the xa_load() result and rx_sc->md_dst before use.

The review of this patch on sashiko.dev flags that this change doesn't
appear to be complete:

  "This is a pre-existing issue, but since xa_alloc() in mlx5e_macsec_add_rxsc()
   publishes sc_xarray_element before rx_sc->md_dst is allocated and initialized,
   is it safe to use a plain read here?
   drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c:mlx5e_macsec_add_rxsc() {
      ...
      err = xa_alloc(&macsec->sc_xarray, &sc_xarray_element->fs_id, sc_xarray_element, ...);
      ...
      rx_sc->md_dst = metadata_dst_alloc(0, METADATA_MACSEC, GFP_KERNEL);
      ...
   }
   Because there are no memory barriers around the assignment and initialization
   of md_dst, could a concurrent datapath reader observe a non-NULL md_dst
   pointer but read uninitialized memory from it in dst_hold_safe()?"

...