Re: [Intel-wired-lan] [PATCH iwl-net v1] ice: Fix memory management in ice_ethtool_fdir.c

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 7/3/23 09:44, Jedrzej Jagielski wrote:
> Fix ethtool FDIR logic to not use momory after its release.
> In the ice_ethtool_fdir.c file there are 2 spots where code can
> refer to pointers which may be missing.
> 
> In the ice_cfg_fdir_xtrct_seq() function seg may be freed but
> even then may be still used by memcpy(&tun_seg[1], seg, sizeof(*seg)).
> 
> In the ice_add_fdir_ethtool() function struct ice_fdir_fltr *input
> may firstly fail to be added via ice_fdir_update_list_entry() but then
> may be tried to being deleted by ice_fdir_update_list_entry.
> 
> Terminate in both cases when the returned value of the previous
> operation is other than 0, free memory and don't use it anymore.
> 
> Replace managed memory alloc with kzalloc/kfree in
> ice_cfg_fdir_xtrct_seq() since seg/tun_seg are used only by
> ice_fdir_set_hw_fltr_rule().
> 
> Reported-by: Michal Schmidt <mschmidt@redhat.com>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2208423
> Fixes: cac2a27cd9ab ("ice: Support IPv4 Flow Director filters")
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> ---
>   .../net/ethernet/intel/ice/ice_ethtool_fdir.c | 55 ++++++++++---------
>   1 file changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c
> index ead6d50fc0ad..89d6a1d2e7e3 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c
> @@ -1204,21 +1204,16 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp,
>   		       struct ice_rx_flow_userdef *user)
>   {
>   	struct ice_flow_seg_info *seg, *tun_seg;
> -	struct device *dev = ice_pf_to_dev(pf);
>   	enum ice_fltr_ptype fltr_idx;
>   	struct ice_hw *hw = &pf->hw;
>   	bool perfect_filter;
>   	int ret;
>   
> -	seg = devm_kzalloc(dev, sizeof(*seg), GFP_KERNEL);
> -	if (!seg)
> -		return -ENOMEM;
> -
> -	tun_seg = devm_kcalloc(dev, ICE_FD_HW_SEG_MAX, sizeof(*tun_seg),
> -			       GFP_KERNEL);
> -	if (!tun_seg) {
> -		devm_kfree(dev, seg);
> -		return -ENOMEM;
> +	seg = kzalloc(sizeof(*seg), GFP_KERNEL);
> +	tun_seg = kcalloc(ICE_FD_HW_SEG_MAX, sizeof(*tun_seg), GFP_KERNEL);
> +	if (!tun_seg || !seg) {
> +		ret = -ENOMEM;
> +		goto err_exit;
>   	}
>   
>   	switch (fsp->flow_type & ~FLOW_EXT) {
> @@ -1281,16 +1276,25 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp,
>   				     ICE_FLOW_FLD_OFF_INVAL);
>   	}
>   
> -	/* add filter for outer headers */
>   	fltr_idx = ice_ethtool_flow_to_fltr(fsp->flow_type & ~FLOW_EXT);
> +
> +	if (perfect_filter)
> +		set_bit(fltr_idx, hw->fdir_perfect_fltr);
> +	else
> +		clear_bit(fltr_idx, hw->fdir_perfect_fltr);
> +
> +	/* add filter for outer headers */
>   	ret = ice_fdir_set_hw_fltr_rule(pf, seg, fltr_idx,
>   					ICE_FD_HW_SEG_NON_TUN);
> -	if (ret == -EEXIST)
> -		/* Rule already exists, free memory and continue */
> -		devm_kfree(dev, seg);
> -	else if (ret)
> +	if (ret == -EEXIST) {
> +		/* Rule already exists, free memory and count as success */
> +		ret = 0;
> +		goto err_exit;
> +	} else if (ret) {
>   		/* could not write filter, free memory */
> +		ret = -EOPNOTSUPP;
>   		goto err_exit;
> +	}
>   
>   	/* make tunneled filter HW entries if possible */
>   	memcpy(&tun_seg[1], seg, sizeof(*seg));
> @@ -1298,25 +1302,20 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp,
>   					ICE_FD_HW_SEG_TUN);
>   	if (ret == -EEXIST) {
>   		/* Rule already exists, free memory and count as success */
> -		devm_kfree(dev, tun_seg);
> +		kfree(tun_seg);
>   		ret = 0;
>   	} else if (ret) {
>   		/* could not write tunnel filter, but outer filter exists */
> -		devm_kfree(dev, tun_seg);
> +		kfree(tun_seg);
>   	}
>   
> -	if (perfect_filter)
> -		set_bit(fltr_idx, hw->fdir_perfect_fltr);
> -	else
> -		clear_bit(fltr_idx, hw->fdir_perfect_fltr);
> -
> -	return ret;
> +		return ret;

Sorry for late report, but now you leak `seg`.

I would rename 'err_exit' to just 'exit', and keep all memory freeing 
there. (That would simplify the if (ret ...) above even more.

Remember to cc netdev and our intel Maintainer for v2.

>   
>   err_exit:
> -	devm_kfree(dev, tun_seg);
> -	devm_kfree(dev, seg);
> +	kfree(tun_seg);
> +	kfree(seg);
>   
> -	return -EOPNOTSUPP;
> +	return ret;
>   }
>   
>   /**
> @@ -1914,7 +1913,9 @@ int ice_add_fdir_ethtool(struct ice_vsi *vsi, struct ethtool_rxnfc *cmd)
>   	input->comp_report = ICE_FXD_FLTR_QW0_COMP_REPORT_SW_FAIL;
>   
>   	/* input struct is added to the HW filter list */
> -	ice_fdir_update_list_entry(pf, input, fsp->location);
> +	ret = ice_fdir_update_list_entry(pf, input, fsp->location);
> +	if (ret)
> +		goto release_lock;
>   
>   	ret = ice_fdir_write_all_fltr(pf, input, true);
>   	if (ret)

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan