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

When an offloaded MACsec RX SC is deleted, macsec_del_rxsc_ctx() freed
the per-SC metadata_dst with metadata_dst_free(), which kfree()s the
object 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(), takes a reference with dst_hold() and
attaches the dst to the skb with skb_dst_set(). A reader that already
obtained the rx_sc pointer can race with the delete path and operate on
freed memory.

Fix the owner side by dropping the reference with dst_release() instead
of freeing unconditionally, and convert the RX datapath to
dst_hold_safe() so a reader racing the SC delete cannot attach a dst
whose last reference was just dropped; only attach it when a reference
was actually taken.

mlx5e_macsec_add_rxsc() also published sc_xarray_element via xa_alloc()
before rx_sc->md_dst was allocated and initialised, so a datapath reader
that looked the SC up by fs_id could observe rx_sc with md_dst still
NULL or, on weakly-ordered architectures, a non-NULL md_dst pointer
whose contents were not yet visible. NULL-check the xa_load() result and
md_dst on the datapath, and reorder add_rxsc() so the xa_alloc() publish
happens only after md_dst is fully initialised; the xarray RCU publish
then pairs with the rcu_read_lock()/xa_load() in the datapath.

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 left to a
follow-up patch.

Found by 0sec automated security-research tooling (https://0sec.ai).

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>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
Link: https://patch.msgid.link/20260627223059.29917-1-doruk@0sec.ai
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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 71b3a059c9647a95c3fa13b415ca42b5fbde6d34..daff53ba7d09f92979e0cbe18cf4b52d7f33bd89 100644 (file)
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -714,34 +714,43 @@ static int mlx5e_macsec_add_rxsc(struct macsec_context *ctx)
        }
 
        sc_xarray_element->rx_sc = rx_sc;
-       err = xa_alloc(&macsec->sc_xarray, &sc_xarray_element->fs_id, sc_xarray_element,
-                      XA_LIMIT(1, MLX5_MACEC_RX_FS_ID_MAX), GFP_KERNEL);
-       if (err) {
-               if (err == -EBUSY)
-                       netdev_err(ctx->netdev,
-                                  "MACsec offload: unable to create entry for RX SC (%d Rx SCs already allocated)\n",
-                                  MLX5_MACEC_RX_FS_ID_MAX);
-               goto destroy_sc_xarray_elemenet;
-       }
 
        rx_sc->md_dst = metadata_dst_alloc(0, METADATA_MACSEC, GFP_KERNEL);
        if (!rx_sc->md_dst) {
                err = -ENOMEM;
-               goto erase_xa_alloc;
+               goto destroy_sc_xarray_elemenet;
        }
 
        rx_sc->sci = ctx_rx_sc->sci;
        rx_sc->active = ctx_rx_sc->active;
-       list_add_rcu(&rx_sc->rx_sc_list_element, rx_sc_list);
-
        rx_sc->sc_xarray_element = sc_xarray_element;
        rx_sc->md_dst->u.macsec_info.sci = rx_sc->sci;
+
+       /*
+        * Publish the fully-initialised SC last: xa_alloc() makes
+        * sc_xarray_element->rx_sc (and rx_sc->md_dst) reachable from the RX
+        * datapath via xa_load().  Doing it only after md_dst is allocated and
+        * initialised pairs with the rcu_read_lock()/xa_load() in
+        * mlx5e_macsec_offload_handle_rx_skb(), so a reader can never observe
+        * a non-NULL md_dst with uninitialised contents.
+        */
+       err = xa_alloc(&macsec->sc_xarray, &sc_xarray_element->fs_id, sc_xarray_element,
+                      XA_LIMIT(1, MLX5_MACEC_RX_FS_ID_MAX), GFP_KERNEL);
+       if (err) {
+               if (err == -EBUSY)
+                       netdev_err(ctx->netdev,
+                                  "MACsec offload: unable to create entry for RX SC (%d Rx SCs already allocated)\n",
+                                  MLX5_MACEC_RX_FS_ID_MAX);
+               goto destroy_md_dst;
+       }
+
+       list_add_rcu(&rx_sc->rx_sc_list_element, rx_sc_list);
        mutex_unlock(&macsec->lock);
 
        return 0;
 
-erase_xa_alloc:
-       xa_erase(&macsec->sc_xarray, sc_xarray_element->fs_id);
+destroy_md_dst:
+       dst_release(&rx_sc->md_dst->dst);
 destroy_sc_xarray_elemenet:
        kfree(sc_xarray_element);
 destroy_rx_sc:
@@ -829,7 +838,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 +1704,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();