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

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

[Jedrzej Jagielski]

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;
 
 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)
-- 
2.31.1

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

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

[Przemek Kitszel]

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

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

[Jagielski, Jedrzej]

From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com> 
Sent: Mon, 3 July 2023 10:34
>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`.

Actually i haven't changed the flow of freeing seg when
firstly it is applied and then applying tun_seg returns error.
There is wrong indentation at the final return line,
it will be fixed.

>
>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.

Sure

>
>>   
>>   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

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

[Przemek Kitszel]

On 7/3/23 11:59, Jagielski, Jedrzej wrote:
> From: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> Sent: Mon, 3 July 2023 10:34
>> 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`.
> 
> Actually i haven't changed the flow of freeing seg when
> firstly it is applied and then applying tun_seg returns error.

But, per my suggestion, you have switched from managed devm to 
kzalloc(), and now you have to kfree() explicitly here (devm would free 
it on unload).

> There is wrong indentation at the final return line,
> it will be fixed.
> 
>>
>> 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.
> 
> Sure
> 
>>
>>>    
>>>    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

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

[Jesse Brandeburg]

On 7/3/2023 12:44 AM, Jedrzej Jagielski wrote:
> Fix ethtool FDIR logic to not use momory after its release.

s/momory/memory/

> 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 first

> may be tried to being deleted by ice_fdir_update_list_entry.

s/may be tried to being/may be/

> 
> 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>



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