Message-ID: <474498f1-5ad3-41ef-aca4-16830ad208ac@intel.com>
Date: Mon, 18 Aug 2025 15:38:04 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>, Anthony Nguyen
	<anthony.l.nguyen@...el.com>
CC: <vgrinber@...hat.com>, <netdev@...r.kernel.org>, Aleksandr Loktionov
	<aleksandr.loktionov@...el.com>, Intel Wired LAN
	<intel-wired-lan@...ts.osuosl.org>, Simon Horman <horms@...nel.org>, "Marcin
 Szycik" <marcin.szycik@...ux.intel.com>
Subject: Re: [PATCH iwl-net 1/2] ice: fix double-call to ice_deinit_hw()
 during probe failure



On 8/18/2025 3:27 AM, Przemek Kitszel wrote:
> On 7/17/25 18:57, Jacob Keller wrote:
>> The following (and similar) KFENCE bugs have recently been found occurring
>> during certain error flows of the ice_probe() function:
>>
>> kernel: ==================================================================
>> kernel: BUG: KFENCE: use-after-free read in ice_cleanup_fltr_mgmt_struct+0x1d
>> kernel: Use-after-free read at 0x00000000e72fe5ed (in kfence-#223):
>> kernel:  ice_cleanup_fltr_mgmt_struct+0x1d/0x200 [ice]
>> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
>> kernel:  ice_probe+0x245/0x2e0 [ice]
>> kernel:
>> kernel: kfence-#223: <..snip..>
>> kernel: allocated by task 7553 on cpu 0 at 2243.527621s (198.108303s ago):
>> kernel:  devm_kmalloc+0x57/0x120
>> kernel:  ice_init_hw+0x491/0x8e0 [ice]
>> kernel:  ice_probe+0x203/0x2e0 [ice]
>> kernel:
>> kernel: freed by task 7553 on cpu 0 at 2441.509158s (0.175707s ago):
>> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
>> kernel:  ice_init+0x1ad/0x570 [ice]
>> kernel:  ice_probe+0x22b/0x2e0 [ice]
>> kernel:
>> kernel: ==================================================================
>>
>> These occur as the result of a double-call to ice_deinit_hw(). This double
>> call happens if ice_init() fails at any point after calling
>> ice_init_dev().
>>
>> Upon errors, ice_init() calls ice_deinit_dev(), which is supposed to be the
>> inverse of ice_init_dev(). However, currently ice_init_dev() does not call
>> ice_init_hw(). Instead, ice_init_hw() is called by ice_probe(). Thus,
>> ice_probe() itself calls ice_deinit_hw() as part of its error cleanup
>> logic.
>>
>> This results in two calls to ice_deinit_hw() which results in straight
>> forward use-after-free violations due to double calling kfree and other
>> cleanup functions.
>>
>> To avoid this double call, move the call to ice_init_hw() into
>> ice_init_dev(), and remove the now logically unnecessary cleanup from
>> ice_probe(). This is simpler than the alternative of moving ice_deinit_hw()
>> *out* of ice_deinit_dev().
> 
> [1]
> 
>>
>> Moving the calls to ice_deinit_hw() requires validating all cleanup paths,
>> and changing significantly more code. Moving the calls of ice_init_hw()
>> requires only validating that the new placement is still prior to all HW
>> structure accesses.
>>
>> For ice_probe(), this now delays ice_init_hw() from before
>> ice_adapter_get() to just after it. This is safe, as ice_adapter_get() does
>> not rely on the HW structure.
> 
> I introduced the order change by
> commit fb59a520bbb1 ("ice: ice_probe: init ice_adapter after HW init").
> You are right that current driver does not yet utilizes that, but it
> will in the future.
> 

If you have a better fix, I'm all for it. The current driver code is
buggy at least in the error handling phase.

>>
>> For ice_devlink_reinit_up(), the ice_init_hw() is now called after
>> ice_set_min_max_msix(). This is also safe as that function does not access
>> the HW structure either.
>>
>> This flow makes more logical sense, as ice_init_dev() is mirrored by
>> ice_deinit_dev(), so it reasonably should be the caller of ice_init_hw().
>> It also reduces one extra call to ice_init_hw() since both ice_probe() and
>> ice_devlink_reinit_up() call ice_init_dev().
>>
>> This resolves the double-free and avoids memory corruption and other
>> invalid memory accesses in the event of a failed probe.
>>
>> Fixes: 5b246e533d01 ("ice: split probe into smaller functions")
> 
> The blame should be on me here. But instead of fixing current bug in
> a way that I would need to invert later, it would be better to fix w/o
> order change. (If unwilling to wait, simple revert would be also better)
> 
> I would like to do [1] above, either by my or Jake hands (will sync).
> 

I hadn't noticed your change when looking through history. I am fine
with either a revert, or dropping this fix from the DDP fix and
submitting that separately. This issue only occurs if probe fails at
certain points and cleanup logic is triggered. That should be rare.

Alternatively I'm fine with reverting the change if we want to get this
potential issue fixed now.

>> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
>> ---
>>   drivers/net/ethernet/intel/ice/devlink/devlink.c | 10 +---------
>>   drivers/net/ethernet/intel/ice/ice_main.c        | 24 +++++++++++-------------
>>   2 files changed, 12 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c
>> index 4af60e2f37df..ef49da0590b3 100644
>> --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c
>> +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c
>> @@ -1233,18 +1233,12 @@ static int ice_devlink_reinit_up(struct ice_pf *pf)
>>   	struct ice_vsi *vsi = ice_get_main_vsi(pf);
>>   	int err;
>>   
>> -	err = ice_init_hw(&pf->hw);
>> -	if (err) {
>> -		dev_err(ice_pf_to_dev(pf), "ice_init_hw failed: %d\n", err);
>> -		return err;
>> -	}
>> -
>>   	/* load MSI-X values */
>>   	ice_set_min_max_msix(pf);
>>   
>>   	err = ice_init_dev(pf);
>>   	if (err)
>> -		goto unroll_hw_init;
>> +		return err;
>>   
>>   	vsi->flags = ICE_VSI_FLAG_INIT;
>>   
>> @@ -1267,8 +1261,6 @@ static int ice_devlink_reinit_up(struct ice_pf *pf)
>>   	rtnl_unlock();
>>   err_vsi_cfg:
>>   	ice_deinit_dev(pf);
>> -unroll_hw_init:
>> -	ice_deinit_hw(&pf->hw);
>>   	return err;
>>   }
>>   
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index 0a11b4281092..c44bb8153ad0 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -4739,6 +4739,12 @@ int ice_init_dev(struct ice_pf *pf)
>>   	struct ice_hw *hw = &pf->hw;
>>   	int err;
>>   
>> +	err = ice_init_hw(hw);
>> +	if (err) {
>> +		dev_err(dev, "ice_init_hw failed: %d\n", err);
>> +		return err;
>> +	}
>> +
>>   	ice_init_feature_support(pf);
>>   
>>   	err = ice_init_ddp_config(hw, pf);
>> @@ -4759,7 +4765,7 @@ int ice_init_dev(struct ice_pf *pf)
>>   	err = ice_init_pf(pf);
>>   	if (err) {
>>   		dev_err(dev, "ice_init_pf failed: %d\n", err);
>> -		return err;
>> +		goto unroll_hw_init;
>>   	}
>>   
>>   	pf->hw.udp_tunnel_nic.set_port = ice_udp_tunnel_set_port;
>> @@ -4803,6 +4809,8 @@ int ice_init_dev(struct ice_pf *pf)
>>   	ice_clear_interrupt_scheme(pf);
>>   unroll_pf_init:
>>   	ice_deinit_pf(pf);
>> +unroll_hw_init:
>> +	ice_deinit_hw(hw);
>>   	return err;
>>   }
>>   
>> @@ -5330,17 +5338,9 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>>   	if (ice_is_recovery_mode(hw))
>>   		return ice_probe_recovery_mode(pf);
>>   
>> -	err = ice_init_hw(hw);
>> -	if (err) {
>> -		dev_err(dev, "ice_init_hw failed: %d\n", err);
>> -		return err;
>> -	}
>> -
>>   	adapter = ice_adapter_get(pdev);
>> -	if (IS_ERR(adapter)) {
>> -		err = PTR_ERR(adapter);
>> -		goto unroll_hw_init;
>> -	}
>> +	if (IS_ERR(adapter))
>> +		return PTR_ERR(adapter);
>>   	pf->adapter = adapter;
>>   
>>   	err = ice_init(pf);
>> @@ -5366,8 +5366,6 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>>   	ice_deinit(pf);
>>   unroll_adapter:
>>   	ice_adapter_put(pdev);
>> -unroll_hw_init:
>> -	ice_deinit_hw(hw);
>>   	return err;
>>   }
>>   
>>
> 



Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)

Powered by blists - more mailing lists