[PATCH net v1] net: mlx5e: Fix use-after-free of the MACsec xarray entry on RX SC deletion

[PATCH net v1] net: mlx5e: Fix use-after-free of the MACsec xarray entry on RX SC deletion

[Binbin Deng]

In macsec_del_rxsc_ctx(), the RX secure channel is torn down with:

	list_del_rcu(&rx_sc->rx_sc_list_element);
	xa_erase(&macsec->sc_xarray, rx_sc->sc_xarray_element->fs_id);
	dst_release(&rx_sc->md_dst->dst);
	kfree(rx_sc->sc_xarray_element);
	kfree_rcu_mightsleep(rx_sc);

The comment above the sequence claims that "xa_erase which uses rcu to
sync" hides the RX SC from the data path, but this is not sufficient:
the RCU protection of xarray covers only its internal nodes (making
xa_load() safe against concurrent erase); it does not extend to the
object pointed to by the entry value.  A reader that has already
obtained the pointer from xa_load() is not covered by any grace period,
and the bare kfree() releases the entry object immediately.

The reader is mlx5e_macsec_offload_handle_rx_skb() in the NAPI RX
path, which runs under rcu_read_lock() only:

	sc_xarray_element = xa_load(&macsec->sc_xarray, fs_id);
	rx_sc = sc_xarray_element ? sc_xarray_element->rx_sc : NULL;

An RCU read-side critical section protects only against deferred frees
(call_rcu()/kfree_rcu()); it provides no protection against the
immediate kfree() above.  If the NAPI reader loads the entry pointer
and is then delayed while another CPU executes the deletion path, the
subsequent read of sc_xarray_element->rx_sc dereferences freed memory.

The path is reachable with CAP_NET_ADMIN while an RX SC has in-flight
traffic: CQEs carrying the fs_id metadata can still arrive after the
offload rule has been removed, so deleting an RX SC concurrently with
reception can race the reader.

Note that the same function already releases rx_sc itself via
kfree_rcu_mightsleep(): the author was aware that this teardown needs a
deferred free, but the intermediate entry object was missed.

Fix this by adding an rcu_head to the entry object and releasing it
with kfree_rcu().

Fixes: b7c9400cbc48 ("net/mlx5e: Implement MACsec Rx data path using MACsec skb_metadata_dst")
Signed-off-by: Binbin Deng <18983559317@163.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 3 ++-
 1 file changed, 2 insertions(+), 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 daff53ba7d09..d4caa7affae6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -75,6 +75,7 @@ struct mlx5e_macsec_rx_sc;
 struct mlx5e_macsec_rx_sc_xarray_element {
 	u32 fs_id;
 	struct mlx5e_macsec_rx_sc *rx_sc;
+	struct rcu_head rcu_head;
 };
 
 struct mlx5e_macsec_rx_sc {
@@ -839,7 +840,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);
 	dst_release(&rx_sc->md_dst->dst);
-	kfree(rx_sc->sc_xarray_element);
+	kfree_rcu(rx_sc->sc_xarray_element, rcu_head);
 	kfree_rcu_mightsleep(rx_sc);
 }
 
-- 
2.43.0

Re: [PATCH net v1] net: mlx5e: Fix use-after-free of the MACsec xarray entry on RX SC deletion

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.