netdev - RE: [PATCH iwl-net v4] ice: Fix memory management in ice_ethtool

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Wed, 12 Jul 2023 13:08:20 +0000
From: "Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>
To: Leon Romanovsky <leon@...nel.org>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, mschmidt <mschmidt@...hat.com>, "Kitszel,
 Przemyslaw" <przemyslaw.kitszel@...el.com>
Subject: RE: [PATCH iwl-net v4] ice: Fix memory management in
 ice_ethtool_fdir.c

From: Leon Romanovsky <leon@...nel.org> 
Sent: Wed, 12 July 2023 08:30
>On Tue, Jul 11, 2023 at 12:04:50PM +0200, Jedrzej Jagielski wrote:
>> Fix ethtool FDIR logic to not use memory 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 first fail to be added via ice_fdir_update_list_entry() but then
>> may be 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.
>> 
>> Reported-by: Michal Schmidt <mschmidt@...hat.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@...el.com>
>> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
>> ---
>> v2: extend CC list, fix freeing memory before return
>> v3: correct typos in the commit msg
>> v4: restore devm() approach
>> ---
>>  .../net/ethernet/intel/ice/ice_ethtool_fdir.c | 30 +++++++++++--------
>>  1 file changed, 18 insertions(+), 12 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..b6308780362b 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool_fdir.c
>> @@ -1281,16 +1281,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);
>> +
>
>The code above is assign_bit(fltr_idx, hw->fdir_perfect_fltr, perfect_filter);
>
>> +	/* 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;
>
>I see that original code returned -EOPNOTSUPP, but why?
>Why do you rewrite return value? Why can't you return "ret" as is?
>
>Thanks

My intention was just to not interfere with returned value.
However i see no reason why an unchanged value shouldn't be returned

Thanks

>
>>  		goto err_exit;
>> +	}
>>  
>>  	/* make tunneled filter HW entries if possible */
>>  	memcpy(&tun_seg[1], seg, sizeof(*seg));
>> @@ -1305,18 +1314,13 @@ ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp,
>>  		devm_kfree(dev, tun_seg);
>>  	}
>>  
>> -	if (perfect_filter)
>> -		set_bit(fltr_idx, hw->fdir_perfect_fltr);
>> -	else
>> -		clear_bit(fltr_idx, hw->fdir_perfect_fltr);
>> -
>>  	return ret;
>>  
>>  err_exit:
>>  	devm_kfree(dev, tun_seg);
>>  	devm_kfree(dev, seg);
>>  
>> -	return -EOPNOTSUPP;
>> +	return ret;
>>  }
>>  
>>  /**
>> @@ -1914,7 +1918,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
>> 
>>