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

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

[Doruk Tan Ozturk]

macsec_del_rxsc_ctx() frees the RX SC metadata_dst via
metadata_dst_free(), which directly kfree()s the object and ignores the
dst_entry refcount. The MACsec RX offload datapath
mlx5e_macsec_offload_handle_rx_skb() takes a reference on this dst with
dst_hold() and attaches it to the skb via skb_dst_set(). If such an skb
is still in flight when the RX SC is deleted, the metadata_dst is freed
while the skb still references it; the subsequent dst_release() on skb
free then operates on already-freed memory.

Replace metadata_dst_free() with dst_release() so the metadata_dst is
freed only after the last reference is dropped. The dst subsystem frees
metadata_dst objects from dst_destroy() once the refcount reaches zero
(DST_METADATA is set by metadata_dst_alloc()).

Same class of bug and fix as commit c32b26aaa2f9 ("netfilter:
nft_tunnel: fix use-after-free on object destroy").

Fixes: 9b9e23c4dc2b ("net/mlx5e: MACsec, fix memory leak when MACsec device is deleted")
Cc: stable@vger.kernel.org
Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 71b3a059c964..2a4e7ed76d31 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);
 }
-- 
2.43.0

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);
>  }