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 <[email protected]> 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/[email protected]?part=1
