| 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: <7f767a82-b70e-4520-a9cd-360949961373@intel.com>
Date: Mon, 18 Aug 2025 12:27:56 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jacob Keller <jacob.e.keller@...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 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.
>
> 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).
> 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;
> }
>
>
Powered by blists - more mailing lists