Re: [PATCH net 1/8] ice: fix UAF/NULL deref when VSI rebuild and XDP attach race

[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: fix UAF/NULL deref when VSI rebuild and XDP attach race

This commit addresses a race condition between a VSI rebuild and an XDP
attach operation. It prevents rx_ring->xdp_prog from being populated before
rx_ring->xdp_ring is valid by rejecting the XDP attach when a VSI rebuild
is pending. XDP detachment is still permitted during this window to clear
the existing program.

Fixes: 2504b8405768 ("ice: protect XDP configuration with a mutex")

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
[ ... ]
> @@ -2912,12 +2912,21 @@ ice_xdp_setup_prog(struct ice_vsi *vsi, struct bpf_prog *prog,
>  	}
>  
>  	/* hot swap progs and avoid toggling link */
> -	if (ice_is_xdp_ena_vsi(vsi) == !!prog ||
> -	    test_bit(ICE_VSI_REBUILD_PENDING, vsi->state)) {
> +	if (ice_is_xdp_ena_vsi(vsi) == !!prog) {
>  		ice_vsi_assign_bpf_prog(vsi, prog);
>  		return 0;
>  	}
>  
> +	if (test_bit(ICE_VSI_REBUILD_PENDING, vsi->state)) {

[Severity: High]
This is a pre-existing issue, but is the check for ICE_VSI_REBUILD_PENDING
prone to a TOCTOU race because it relies on ad-hoc flag synchronization
instead of proper locking with the device reset path?

The asynchronous reset path (ice_prepare_for_reset()) sets
ICE_VSI_REBUILD_PENDING without holding vsi->xdp_state_lock or rtnl_lock().

If ice_xdp_setup_prog() (which holds both locks) checks the bit before it
is set, it proceeds to configure XDP and sets ICE_VSI_DOWN. If the reset
path then sets the bit and calls ice_pf_dis_all_vsi(), it will skip this
VSI because ICE_VSI_DOWN is already set.

When ice_xdp_setup_prog() finishes, it calls ice_up() which clears
ICE_VSI_DOWN and starts enabling hardware queues while the device is
actively undergoing a hardware reset (ice_do_reset()).

Can writing to hardware registers during an active reset cause hardware
hangs or PCI master aborts?

> +		if (prog) {
> +			NL_SET_ERR_MSG_MOD(extack, "VSI rebuild is pending");
> +			return -EAGAIN;
> +		}
> +
> +		ice_vsi_assign_bpf_prog(vsi, NULL);

[Severity: High]
This is a pre-existing issue, but does detaching an XDP program while a VSI
rebuild has failed lead to a NULL pointer dereference here?

If a VSI rebuild fails, ice_vsi_decfg() frees vsi->rx_rings and sets the
pointer to NULL via ice_vsi_free_arrays(), but leaves vsi->num_rxq
non-zero. The ICE_VSI_REBUILD_PENDING bit remains set because it is only
cleared on the success path in ice_vsi_rebuild().

If a user subsequently detaches XDP, ice_xdp_setup_prog() intercepts it
because ICE_VSI_REBUILD_PENDING is set, and it calls
ice_vsi_assign_bpf_prog(vsi, NULL).

This function loops up to vsi->num_rxq:

ice_vsi_assign_bpf_prog() {
    ...
    ice_for_each_rxq(vsi, i)
        WRITE_ONCE(vsi->rx_rings[i]->xdp_prog, vsi->xdp_prog);
    ...
}

Since vsi->rx_rings is NULL, does this cause a guaranteed NULL pointer
dereference panic?

> +		return 0;
> +	}
> +
>  	if_running = netif_running(vsi->netdev) &&
>  		     !test_and_set_bit(ICE_VSI_DOWN, vsi->state);
-- 
pw-bot: cr