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

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Fri, May 22, 2026 at 05:16:16PM -0700, Jakub Kicinski wrote:
> 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?

CC: Larysa

Seems we should check if vsi->rx_rings != NULL plus
ICE_VSI_REBUILD_PENDING should be cleared on error path in
ice_vsi_rebuild().

Do you have any comment on TOCTOU issue mentioned above?

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