Re: [PATCH net 07/10] ice: eswitch: fix use-after-free of metadata_dst in repr release

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: eswitch: fix use-after-free of metadata_dst in repr release

This replaces metadata_dst_free() with dst_release() in
ice_eswitch_release_repr() so that the port representor metadata_dst is
freed only after the last dst_entry reference goes away, instead of being
kfree()d while in-flight skbs may still reference it.

(No Link: tags present in the commit header.)

> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> index b069e6c514fb1..6e7bba4738989 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> @@ -95,7 +95,7 @@ ice_eswitch_release_repr(struct ice_pf *pf, struct ice_repr *repr)
>  		return;
>  
>  	ice_vsi_update_security(vsi, ice_vsi_ctx_set_antispoof);
> -	metadata_dst_free(repr->dst);
> +	dst_release(&repr->dst->dst);
>  	repr->dst = NULL;
>  	ice_fltr_add_mac_and_broadcast(vsi, repr->parent_mac,
>  				       ICE_FWD_TO_VSI);

[Severity: High]

This isn't a bug introduced by this patch, but with the put side now fixed,
is the get side of repr->dst still unsynchronized?

ice_eswitch_port_start_xmit() does three separate plain loads of repr->dst
with no lock, no RCU section and no READ_ONCE():

	skb_dst_drop(skb);
	dst_hold((struct dst_entry *)repr->dst);
	skb_dst_set(skb, (struct dst_entry *)repr->dst);
	skb->dev = repr->dst->u.port_info.lower_dev;

while ice_eswitch_release_repr() drops the last reference and then stores
the NULL:

	dst_release(&repr->dst->dst);
	repr->dst = NULL;

In the normal teardown path the ordering appears to save this, because
ice_eswitch_detach() calls repr->ops.rem() (unregister_netdev(), which
quiesces the representor's ndo_start_xmit) before
ice_eswitch_release_repr().

The attach error path has the opposite order:

	err_xa_alloc:
		ice_eswitch_release_repr(pf, repr);
	err_setup_repr:
		repr->ops.rem(repr);

At err_xa_alloc the representor netdev is already registered, since
repr->ops.add() runs ice_repr_add_vf()/ice_repr_add_sf() ->
ice_repr_reg_netdev() -> register_netdev(), and the attach path holds
devl_lock but not RTNL, so userspace can open the interface and transmit.

Two outcomes seem possible in that window:

  - the xmit CPU loaded a non-NULL repr->dst just before dst_release()
    drove the rcuref to zero, so dst_hold() hits its
    WARN_ON(!rcuref_get(&dst->__rcuref)), takes no reference, and the skb
    carries a metadata_dst that dst_destroy_rcu() frees after the grace
    period, which then gets read by ice_eswitch_set_target_vsi() or by the
    skb free path;

  - the xmit CPU loads repr->dst after the NULL store, giving dst_hold(NULL)
    and a NULL dereference inside rcuref_get().

Would swapping the two error labels so repr->ops.rem() runs before
ice_eswitch_release_repr(), or clearing repr->dst followed by
synchronize_net() before dst_release(), close this window? Also, since
err_setup_repr is reached when metadata_dst_alloc() fails, does that path
leave a registered netdev whose repr->dst is NULL until repr->ops.rem()
runs?

[Severity: High]

This is a pre-existing issue as well, and not something this patch changes,
but does the deferred free help the LAG path at all?

ice_lag_config_eswitch() writes through repr->dst with no NULL check:

	xa_for_each(&lag->pf->eswitch.reprs, id, repr)
		repr->dst->u.port_info.lower_dev = netdev;

xa_for_each() expands to xa_find()/xa_find_after(), which take and drop
rcu_read_lock() internally, so repr is used outside any RCU read-side
section.

The caller ice_lag_process_event() is a work item and holds only
pf->lag_mutex, while the teardown side (ice_free_vfs() ->
ice_eswitch_detach_vf(), and the ice_reset_all_vfs() path) holds
vf->cfg_lock and devl_lock, so the locksets do not intersect.

That seems to allow:

	LAG work				detach thread
	repr = xa_find(...)
						xa_erase(...)
						ice_eswitch_release_repr()
						  repr->dst = NULL;
						ice_repr_destroy()
						  kfree(repr);
	repr->dst->u.port_info.lower_dev = netdev;

which is either a store through NULL or a store into freed memory, since
ice_repr_destroy() does a plain kfree(repr) with no grace period.

The change from metadata_dst_free() to dst_release() only defers freeing the
metadata_dst; the repr->dst = NULL store and the kfree(repr) are still
immediate. Should this path be serialized against the eswitch teardown, or
at least keep the repr lookup inside an RCU read-side section with an
RCU-deferred repr free?