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 The ice_cfg_tx_topo function attempts to apply Tx scheduler topology configuration based on NVM parameters, selecting either a 5 or 9 layer topology. As part of this flow, the driver acquires the "Global Configuration Lock", which is a hardware resource associated with programming the DDP package to the device. This "lock" is implemented by firmware as a way to guarantee that only one PF can program the DDP for a device. Unlike a traditional lock, once a PF has acquired this lock, no other PF will be able to acquire it again (including that PF) until a CORER of the device. Future requests to acquire the lock report that global configuration has already completed. The following flow is used to program the Tx topology: * Read the DDP package for scheduler configuration data * Acquire the global configuration lock * Program Tx scheduler topology according to DDP package data * Trigger a CORER which clears the global configuration lock This is followed by the flow for programming the DDP package: * Acquire the global configuration lock (again) * Download the DDP package to the device * Release the global configuration lock. However, if configuration of the Tx topology fails, (i.e. ice_get_set_tx_topo returns an error code), the driver exits ice_cfg_tx_topo() immediately, and fails to trigger CORER. While the global configuration lock is held, the firmware rejects most AdminQ commands, as it is waiting for the DDP package download (or Tx scheduler topology programming) to occur. The current driver flows assume that the global configuration lock has been reset by CORER after programming the Tx topology. Thus, the same PF attempts to acquire the global lock again, and fails. This results in the driver reporting "an unknown error occurred when loading the DDP package". It then attempts to enter safe mode, but ultimately fails to finish ice_probe() since nearly all AdminQ command report error codes, and the driver stops loading the device at some point during its initialization. The only currently known way that ice_get_set_tx_topo() can fail is with certain older DDP packages which contain invalid topology configuration, on firmware versions which strictly validate this data. The most recent releases of the DDP have resolved the invalid data. However, it is still poor practice to essentially brick the device, and prevent access to the device even through safe mode or recovery mode. It is also plausible that this command could fail for some other reason in the future. We cannot simply release the global lock after a failed call to ice_get_set_tx_topo(). Releasing the lock indicates to firmware that global configuration (downloading of the DDP) has completed. Future attempts by this or other PFs to load the DDP will fail with a report that the DDP package has already been downloaded. Then, PFs will enter safe mode as they realize that the package on the device does not meet the minimum version requirement to load. The reported error messages are confusing, as they indicate the version of the default "safe mode" package in the NVM, rather than the version of the file loaded from /lib/firmware. Instead, we need to trigger CORER to clear global configuration. This is the lowest level of hardware reset which clears the global configuration lock and related state. It also clears any already downloaded DDP. Crucially, it does *not* clear the Tx scheduler topology configuration. Refactor ice_cfg_tx_topo() to always trigger a CORER after acquiring the global lock, regardless of success or failure of the topology configuration. We need to re-initialize the HW structure when we trigger the CORER. Thus, it makes sense for this to be the responsibility of ice_cfg_tx_topo() rather than its caller, ice_init_tx_topology(). This avoids needless re-initialization in cases where we don't attempt to update the Tx scheduler topology, such as if it has already been programmed. There is one catch: failure to re-initialize the HW struct should stop ice_probe(). If this function fails, we won't have a valid HW structure and cannot ensure the device is functioning properly. To handle this, ensure ice_cfg_tx_topo() returns a limited set of error codes. Set aside one specifically, -ENODEV, to indicate that the ice_init_tx_topology() should fail and stop probe. Other error codes indicate failure to apply the Tx scheduler topology. This is treated as a non-fatal error, with an informational message informing the system administrator that the updated Tx topology did not apply. This allows the device to load and function with the default Tx scheduler topology, rather than failing to load entirely. Note that this use of CORER will not result in loops with future PFs attempting to also load the invalid Tx topology configuration. The first PF will acquire the global configuration lock as part of programming the DDP. Each PF after this will attempt to acquire the global lock as part of programming the Tx topology, and will fail with the indication from firmware that global configuration is already complete. Tx scheduler topology configuration is only performed during driver init (probe or devlink reload) and not during cleanup for a CORER that happens after probe completes. Fixes: 91427e6d9030 ("ice: Support 5 layer topology") Signed-off-by: Jacob Keller --- drivers/net/ethernet/intel/ice/ice_ddp.c | 44 ++++++++++++++++++++++--------- drivers/net/ethernet/intel/ice/ice_main.c | 14 ++++++---- 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c index 59323c019544..bc525de019de 100644 --- a/drivers/net/ethernet/intel/ice/ice_ddp.c +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c @@ -2374,7 +2374,13 @@ ice_get_set_tx_topo(struct ice_hw *hw, u8 *buf, u16 buf_size, * The function will apply the new Tx topology from the package buffer * if available. * - * Return: zero when update was successful, negative values otherwise. + * Return: + * * 0 - Successfully applied topology configuration. + * * -EBUSY - Failed to acquire global configuration lock. + * * -EEXIST - Topology configuration has already been applied. + * * -EIO - Unable to apply topology configuration. + * * -ENODEV - Failed to re-initialize device after applying configuration. + * * Other negative error codes indicate unexpected failures. */ int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len) { @@ -2407,7 +2413,7 @@ int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len) if (status) { ice_debug(hw, ICE_DBG_INIT, "Get current topology is failed\n"); - return status; + return -EIO; } /* Is default topology already applied ? */ @@ -2494,31 +2500,45 @@ int ice_cfg_tx_topo(struct ice_hw *hw, const void *buf, u32 len) ICE_GLOBAL_CFG_LOCK_TIMEOUT); if (status) { ice_debug(hw, ICE_DBG_INIT, "Failed to acquire global lock\n"); - return status; + return -EBUSY; } /* Check if reset was triggered already. */ reg = rd32(hw, GLGEN_RSTAT); if (reg & GLGEN_RSTAT_DEVSTATE_M) { - /* Reset is in progress, re-init the HW again */ ice_debug(hw, ICE_DBG_INIT, "Reset is in progress. Layer topology might be applied already\n"); ice_check_reset(hw); - return 0; + /* Reset is in progress, re-init the HW again */ + goto reinit_hw; } /* Set new topology */ status = ice_get_set_tx_topo(hw, new_topo, size, NULL, NULL, true); if (status) { - ice_debug(hw, ICE_DBG_INIT, "Failed setting Tx topology\n"); - return status; + ice_debug(hw, ICE_DBG_INIT, "Failed to set Tx topology, status %pe\n", + ERR_PTR(status)); + /* only report -EIO here as the caller checks the error value + * and reports an informational error message informing that + * the driver failed to program Tx topology. + */ + status = -EIO; } - /* New topology is updated, delay 1 second before issuing the CORER */ + /* Even if Tx topology config failed, we need to CORE reset here to + * clear the global configuration lock. Delay 1 second to allow + * hardware to settle then issue a CORER + */ msleep(1000); ice_reset(hw, ICE_RESET_CORER); - /* CORER will clear the global lock, so no explicit call - * required for release. - */ + ice_check_reset(hw); - return 0; +reinit_hw: + /* Since we triggered a CORER, re-initialize hardware */ + ice_deinit_hw(hw); + if (ice_init_hw(hw)) { + ice_debug(hw, ICE_DBG_INIT, "Failed to re-init hardware after setting Tx topology\n"); + return -ENODEV; + } + + return status; } diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index c44bb8153ad0..b31b3aa2b662 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -4532,17 +4532,21 @@ ice_init_tx_topology(struct ice_hw *hw, const struct firmware *firmware) dev_info(dev, "Tx scheduling layers switching feature disabled\n"); else dev_info(dev, "Tx scheduling layers switching feature enabled\n"); - /* if there was a change in topology ice_cfg_tx_topo triggered - * a CORER and we need to re-init hw + return 0; + } else if (err == -ENODEV) { + /* If we failed to re-initialize the device, we can no longer + * continue loading. */ - ice_deinit_hw(hw); - err = ice_init_hw(hw); - + dev_warn(dev, "Failed to initialize hardware after applying Tx scheduling configuration.\n"); return err; } else if (err == -EIO) { dev_info(dev, "DDP package does not support Tx scheduling layers switching feature - please update to the latest DDP package and try again\n"); + return 0; } + /* Do not treat this as a fatal error. */ + dev_info(dev, "Failed to apply Tx scheduling configuration, err %pe\n", + ERR_PTR(err)); return 0; } -- 2.50.0.349.ga842a77808e9