ice: fix double-call to ice_deinit_hw() during probe failure

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-torvalds#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-torvalds#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: 5b246e5 ("ice: split probe into smaller functions")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Author: jacob-keller

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;
 }
 

drivers/net/ethernet/intel/ice/ice_main.c

@@ -4743,6 +4743,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);
@@ -4763,7 +4769,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;
@@ -4806,6 +4812,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;
 }
 
@@ -5333,17 +5341,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);
@@ -5369,8 +5369,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;
 }