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(). 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. 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") Signed-off-by: Jacob Keller --- 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; } -- 2.50.0.349.ga842a77808e9