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

[Simon Horman <horms@kernel.org>]

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()?"

...