Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] i40e: TOCTOU race and VSI UAF in i40e_vc_add_cloud_filter() and reset - [Low] i40e: mutex definition breaks kernel-doc for i40e_del_all_cloud_filters() --
commit 03ab6f762cec8d24f3f9bbef8a127d3ed4ca9ef7 Author: Binbin Deng <[email protected]> i40e: Lock VF cloud filter list operations This commit adds a file-local mutex, i40e_vf_cloud_filter_lock, to serialize VF cloud filter additions, deletions, and bulk removals across i40e_vc_add_cloud_filter(), i40e_vc_del_cloud_filter(), and i40e_del_all_cloud_filters(). > diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > index c6732a24b6400..d62e59c7ac814 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c > @@ -3740,6 +3740,8 @@ static struct i40e_vsi *i40e_find_vsi_from_seid(struct > i40e_vf *vf, u16 seid) > * > * This function deletes all cloud filters > **/ > +static DEFINE_MUTEX(i40e_vf_cloud_filter_lock); > + > static void i40e_del_all_cloud_filters(struct i40e_vf *vf) [Severity: Low] Will placing DEFINE_MUTEX(i40e_vf_cloud_filter_lock) between the kernel-doc comment block and i40e_del_all_cloud_filters() cause scripts/kernel-doc to emit a warning? Because DEFINE_MUTEX() sits immediately after the closing **/ of the comment, kernel-doc parses DEFINE_MUTEX() as the documented prototype instead of i40e_del_all_cloud_filters(). [ ... ] > @@ -3856,6 +3860,7 @@ static int i40e_vc_del_cloud_filter(struct i40e_vf *vf, > u8 *msg) > } > > /* get the vsi to which the tc belongs to */ > + mutex_lock(&i40e_vf_cloud_filter_lock); > vsi = pf->vsi[vf->ch[vcf->action_meta].vsi_idx]; > cfilter.seid = vsi->seid; > cfilter.flags = vcf->field_flags; [ ... ] > @@ -4002,6 +4009,7 @@ static int i40e_vc_add_cloud_filter(struct i40e_vf *vf, > u8 *msg) > } > > /* get the VSI to which the TC belongs to */ > + mutex_lock(&i40e_vf_cloud_filter_lock); > vsi = pf->vsi[vf->ch[vcf->action_meta].vsi_idx]; > cfilter->seid = vsi->seid; > cfilter->flags = vcf->field_flags; [Severity: High] Can i40e_vc_add_cloud_filter() and i40e_vc_del_cloud_filter() still race with i40e_ndo_set_vf_trust() and dereference a NULL or freed struct i40e_vsi, or install a cloud filter after trust has been revoked? In both i40e_vc_add_cloud_filter() and i40e_vc_del_cloud_filter(), the checks for i40e_sync_vf_state(vf, I40E_VF_STATE_ACTIVE), vf->adq_enabled, i40e_validate_cloud_filter(vf, vcf), and vf->num_cloud_filters run before acquiring i40e_vf_cloud_filter_lock. Meanwhile, when host-side trust is revoked, i40e_ndo_set_vf_trust() resets the VF outside i40e_vf_cloud_filter_lock before calling i40e_del_all_cloud_filters(): drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:i40e_ndo_set_vf_trust() { ... if (!setting && (vf->adq_enabled || vf->num_cloud_filters > 0 || test_bit(I40E_VF_STATE_UC_PROMISC, &vf->vf_states) || test_bit(I40E_VF_STATE_MC_PROMISC, &vf->vf_states))) { i40e_vc_reset_vf(vf, true); i40e_del_all_cloud_filters(vf); } ... } During i40e_vc_reset_vf() -> i40e_reset_vf() -> i40e_cleanup_reset_vf() -> i40e_free_vf_res(), the ADq channel VSIs are released and their indices are cleared without holding i40e_vf_cloud_filter_lock: drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c:i40e_free_vf_res() { ... if (vf->adq_enabled && vf->ch[0].vsi_idx) { for (j = 0; j < vf->num_tc; j++) { if (j) i40e_vsi_release(pf->vsi[vf->ch[j].vsi_idx]); vf->ch[j].vsi_idx = 0; vf->ch[j].vsi_id = 0; } } ... } If i40e_vc_add_cloud_filter() or i40e_vc_del_cloud_filter() passes the initial unlocked validation checks and then executes: mutex_lock(&i40e_vf_cloud_filter_lock); vsi = pf->vsi[vf->ch[vcf->action_meta].vsi_idx]; cfilter->seid = vsi->seid; concurrently with i40e_vc_reset_vf(), vsi can be NULL or already freed by i40e_vsi_release(), or vf->ch[vcf->action_meta].vsi_idx can be zeroed so that pf->vsi[0] (the PF main VSI) is used instead. Additionally, if i40e_vc_add_cloud_filter() passes i40e_validate_cloud_filter() before trust is revoked and acquires i40e_vf_cloud_filter_lock after both i40e_vc_reset_vf() and i40e_del_all_cloud_filters() have finished, wouldn't it program the filter into hardware and append cfilter to vf->cloud_filter_list for an untrusted VF? [ ... ] > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
