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

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net/mlx5e: macsec: fix use-after-free of metadata_dst on RX SC delete

This patch replaces metadata_dst_free() with dst_release() during MACsec
RX SC deletion. It relies on the dst subsystem's refcounting to defer freeing
the metadata_dst until the last reference is dropped, aiming to fix a
use-after-free when an skb still references it.

> 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 71b3a059c9647..2a4e7ed76d315 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);

[Severity: High]
Does this change introduce a new use-after-free in the RX datapath?

When dst_release() drops the refcount to 0, concurrent readers in
mlx5e_macsec_offload_handle_rx_skb() can still find the object via RCU
and call dst_hold(). Because dst_hold() uses rcuref_get(), it fails on a
0-refcount object without incrementing the count.

However, the datapath still attaches the 0-refcount dst to the skb. When the
skb is later freed, skb_dst_drop() will call dst_release() on already-freed
memory.

Should the datapath be updated to use dst_hold_safe() instead?

>  	kfree(rx_sc->sc_xarray_element);

[Severity: High]
This is a pre-existing issue, but does this synchronous kfree() cause a
use-after-free for concurrent datapath readers?

The element was just removed from the RCU-protected xarray via xa_erase(),
but the RX datapath can still be accessing it via xa_load() under
rcu_read_lock() in mlx5e_macsec_offload_handle_rx_skb().

Should this use an RCU-deferred free mechanism instead?

[Severity: Critical]
This is a pre-existing issue, but does mlx5e_macsec_offload_handle_rx_skb()
blindly dereference the result of xa_load()?

drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c:mlx5e_macsec_offload_handle_rx_skb() {
    ...
    sc_xarray_element = xa_load(&macsec->sc_xarray, fs_id);
    rx_sc = sc_xarray_element->rx_sc;
    ...
}

If xa_load() returns NULL (e.g., if the entry was concurrently deleted by
xa_erase() or the packet has an unrecognized fs_id), will this cause a
deterministic kernel panic?

Is a NULL check needed before accessing the returned element?

>  	kfree_rcu_mightsleep(rx_sc);
>  }