| 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
| ||
|
Message-ID: <DM6PR11MB273133BC65028765D6739FB8F030A@DM6PR11MB2731.namprd11.prod.outlook.com>
Date: Mon, 10 Jul 2023 12:53:41 +0000
From: "Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>
To: "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, mschmidt
<mschmidt@...hat.com>, "Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>
Subject: RE: [PATCH iwl-net v3] ice: Fix memory management in
ice_ethtool_fdir.c
From: Nguyen, Anthony L <anthony.l.nguyen@...el.com>
Sent: Fri, 7 July 2023 19:16
>On 7/6/2023 2:19 AM, 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.
>>
>> 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@...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
>> ---
>> .../net/ethernet/intel/ice/ice_ethtool_fdir.c | 62 +++++++++----------
>> 1 file changed, 28 insertions(+), 34 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..619b32f4bc53 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 exit;
>
>IIRC individual checks and goto's are preferred over combining them.
For both cases there is the same behavior so it was done due to limit
the line redundancy, but if you think it is better to split them up i
can do this
>
>> }
>>
>> switch (fsp->flow_type & ~FLOW_EXT) { @@ -1264,7 +1259,7 @@
>> ice_cfg_fdir_xtrct_seq(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp,
>> ret = -EINVAL;
>> }
>> if (ret)
>> - goto err_exit;
>> + goto exit;
>>
>> /* tunnel segments are shifted up one. */
>> memcpy(&tun_seg[1], seg, sizeof(*seg)); @@ -1281,42 +1276,39 @@
>> 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 exit;
>> + } else if (ret) {
>> /* could not write filter, free memory */
>> - goto err_exit;
>> + ret = -EOPNOTSUPP;
>> + goto exit;
>> + }
>>
>> /* make tunneled filter HW entries if possible */
>> memcpy(&tun_seg[1], seg, sizeof(*seg));
>> ret = ice_fdir_set_hw_fltr_rule(pf, tun_seg, fltr_idx,
>> ICE_FD_HW_SEG_TUN);
>> - if (ret == -EEXIST) {
>> + if (ret == -EEXIST)
>> /* Rule already exists, free memory and count as success */
>> - devm_kfree(dev, tun_seg);
>> ret = 0;
>> - } else if (ret) {
>> - /* could not write tunnel filter, but outer filter exists */
>> - 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);
>> +exit:
>> + kfree(tun_seg);
>> + kfree(seg);
>
>Previously, success would not free these. They look to be set into hw_prof via
ice_fdir_set_hw_fltr_rule(). Is it safe to be freeing them now?
Yeah, I will restore the previous approach to avoid confusion
>
>> return ret;
>> -
>> -err_exit:
>> - devm_kfree(dev, tun_seg);
>> - devm_kfree(dev, seg);
>> -
>> - return -EOPNOTSUPP;
>> }
>>
>> /**
>> @@ -1914,7 +1906,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)
Powered by blists - more mailing lists