Re: [PATCH iwl-net] ice: wait for in-flight Rx polls before freeing a representor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ice: UAF in RX path due to premature rcu_read_unlock and shared NAPI
--

commit 85b479dcdaa34d1d1340c09f178fce9d7d04ceda
Author: Linkui Xiao <xiaolinkui@kylinos.cn>

ice: wait for in-flight Rx polls before freeing a representor

This commit adds an explicit RCU read-side critical section in the Rx path
when looking up a representor netdev. It also adds a synchronize_net() call
in the detach path to wait for the Rx path to complete before tearing down
the representor.

> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> index c32d6b201369b..0e5b98a05981d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> @@ -566,6 +566,13 @@ static void ice_eswitch_detach(struct ice_pf *pf, struct ice_repr *repr)
>  
>  	xa_erase(&pf->eswitch.reprs, repr->id);
>  
> +	/* The Rx path looks the representor up by its id and dereferences the
> +	 * result without holding a reference to it. Nothing can find it in the
> +	 * xarray anymore, so wait for the polls that picked it up before the
> +	 * erase to finish before the representor is torn down.
> +	 */
> +	synchronize_net();
> +

[Severity: High]
Does this synchronize_net() return too early?

Since the explicitly added RCU read lock is dropped in
ice_process_skb_fields(), the packet is passed to napi_gro_receive()
outside of the RCU critical section:

ice_clean_rx_irq()
  ice_process_skb_fields()
    rcu_read_unlock()
  napi_gro_receive()

This means the packet can be queued in the uplink VSI's NAPI GRO hash.

Because the representor shares the uplink VSI's NAPI, unregistering the
representor will not flush this GRO hash.

Could synchronize_net() return while the packet is still held in the uplink
VSI's GRO hash because the RCU lock was already dropped, leading to the
premature freeing of the representor netdev?

>  	if (xa_empty(&pf->eswitch.reprs))
>  		ice_eswitch_disable_switchdev(pf);

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> index 19307086c238f..e0712db9273cd 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> @@ -235,6 +243,7 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring,
>  		 */
>  		__skb_push(skb, ETH_HLEN);
>  		skb->protocol = eth_type_trans(skb, netdev);
> +		rcu_read_unlock();

[Severity: High]
Is it safe to drop the explicitly added RCU read lock here?

It seems this leaves skb->dev unprotected from being freed while the
packet is still being processed and passed up the stack in
ice_clean_rx_irq(). Should the lock be held longer to ensure the packet
safely traverses the stack or gets flushed from the GRO hash?

>  	}
>  
>  	ice_rx_csum(rx_ring, skb, rx_desc, ptype);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012072959.254206-1-xiaolinkui@126.com?part=1