Re: [PATCH v1] i40e: Lock VF cloud filter list operations

[sashiko-bot@kernel.org]

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 <18983559317@163.com>

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/20261020025856.519623-1-18983559317@163.com?part=1