Re: [PATCH net-next V4 07/14] net/mlx5e: macsec: Move RX marker from ft_metadata to flow_tag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Deterministic NULL pointer dereference in macsec_fs_id_del during error recovery.
- [High] The function `macsec_fs_id_del` frees an RCU-protected `struct mlx5_fs_id` synchronously with `kfree()` instead of deferring it via RCU, risking a use-after-free.
--

commit b66a5cce46cf15521232a2fb7c33b033c4391c99
Author: Cosmin Ratiu <cratiu@nvidia.com>
net/mlx5e: macsec: Move RX marker from ft_metadata to flow_tag

This commit changes the MACsec marker from bit 30 of ft_metadata to a
dedicated flow_tag protocol ID. It updates steering rules and helper macros
accordingly to prevent conflicts with PSP SPI values.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.c
> index efc5167493c12..4ca3a6ded2841 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.c
> @@ -1803,6 +1805,11 @@ macsec_fs_rx_add_rule(struct mlx5_macsec_fs *macsec_fs,
>  
>  		macsec_fs_rx_setup_fte(spec, &flow_act, attrs, false);
>  
> +		spec->flow_context.flags |= FLOW_CONTEXT_HAS_TAG;
> +		spec->flow_context.flow_tag =
> +			FIELD_PREP(MLX5E_ACCEL_FLOW_TAG_PROTO_MASK,
> +				   MLX5E_ACCEL_FLOW_TAG_PROTO_MACSEC);
> +
>  		flow_act.modify_hdr = modify_hdr;
>  		flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST |
>  				  MLX5_FLOW_CONTEXT_ACTION_CRYPTO_DECRYPT |

[Severity: High]
This isn't a bug introduced by this patch, but later in this function during
error recovery, there is a deterministic NULL pointer dereference:

macsec_fs_rx_add_rule() {
    ...
	err = macsec_fs_id_add(&macsec_fs->macsec_devices_list, fs_id,
			       macsec_ctx->secy->netdev,
			       &macsec_fs->fs_id_hash, attrs->sci, false);
	if (err) {
		mlx5_core_err(mdev, "Failed to save fs_id, err=%d\n", err);
		goto err;
	}
    ...
err:
	macsec_fs_rx_del_rule(macsec_fs, rx_rule, macsec_ctx->secy->netdev,
			      fs_id);
	macsec_rule = NULL;
}

When macsec_fs_id_add() fails (e.g., due to memory allocation failure), it
cleans up by freeing the newly allocated macsec_device and removing it from
the list. However, the err block then unconditionally calls
macsec_fs_rx_del_rule(), which in turn calls macsec_fs_id_del():

macsec_fs_id_del() {
    ...
	fs_id_xa = (is_tx) ? &macsec_device->tx_id_xa :
			     &macsec_device->rx_id_xa;
	xa_lock(fs_id_xa);
    ...
}

Since macsec_device was never successfully added, the lookup fails and
returns NULL. Computing fs_id_xa from a NULL macsec_device pointer and
calling xa_lock() on it causes a direct NULL pointer dereference. Could
the error path be updated to avoid calling macsec_fs_id_del() when the
device was not added?

[Severity: High]
This is a pre-existing issue, but macsec_fs_id_del() appears to free
an RCU-protected structure synchronously instead of deferring it via RCU:

macsec_fs_id_del() {
    ...
	rhashtable_remove_fast(fs_id_hash, &fs_id_found->rhash_node,
			       macsec_fs_id_rhash_params);
    ...
	kfree(fs_id_found);
}

Because it does not use kfree_rcu() or wait for an RCU grace period,
concurrent lockless readers running under rcu_read_lock() can access the
freed memory:

mlx5_macsec_fs_get_fs_id_from_hashtable() {
    ...
	mlx5_fs_id = rhashtable_lookup(&macsec_fs->sci_hash, sci, rhash_sci);
	if (mlx5_fs_id)
		fs_id = mlx5_fs_id->id;
}

If a reader obtains a pointer just before removal, it will dereference it
after the writer has freed it. Should this memory be freed using kfree_rcu()
to prevent use-after-free during concurrent datapath packet processing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908065244.3799142-1-tariqt@nvidia.com?part=7