On 10/10/26 2:16 PM, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
to consider:
- [High] ice: DMA UAF due to missing control queue teardown in recovery mode
- [High] ice: NULL pointer deref in ice_adapter_put() on recovery mode teardown
both pre-existing, will make separate -net submissions for them
--
commit 298f011950fb287bfb913324af33c969b5683c8d
Author: Przemek Kitszel <[email protected]>
Subject: ice: use shared devlink to store ice_adapters instead of custom xarray
This commit refactors the storage and deduplication logic for ice_adapters
by moving it to a shared devlink instance. It replaces the custom xarray
implementation with nested devlink instances for PF devices.
Link: https://lore.kernel.org/all/[email protected]
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
b/drivers/net/ethernet/intel/ice/ice_main.c
index 454fba07d4ecc..6373cd5ac2884 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
[ ... ]
@@ -4946,7 +4946,12 @@ static int ice_init_devlink(struct ice_pf *pf)
return err;
ice_devlink_init_regions(pf);
- ice_devlink_register(pf);
+ err = ice_devlink_register(pf);
+ if (err) {
+ ice_devlink_destroy_regions(pf);
+ ice_devlink_unregister_params(pf);
+ return err;
+ }
[Severity: High]
Can this new error path lead to a DMA use-after-free regression in recovery
mode?
In ice_probe_recovery_mode(), the hardware control queues are created via
ice_create_all_ctrlq(), which allocates DMA memory using devres. If
ice_init_devlink() fails and returns an error here, the function exits
without calling ice_destroy_all_ctrlq().
The devres framework will unmap and free the DMA memory upon probe failure,
but the hardware ATQ/ARQ registers might still point to those freed
physical addresses since the queues were never gracefully disabled. Could
this allow the active device to continue performing DMA operations on freed
memory?
The new error path cannot be reached in recovery mode. In that mode
pf->adapter is NULL, so ice_devlink_register() skips
devl_nested_devlink_set() and always returns 0.
The missing ice_destroy_all_ctrlq() is a pre-existing issue that this
series does not change:
ice_init_devlink() could already fail in recovery mode via
ice_devlink_register_params().
The recovery-mode branch of ice_remove() doesn't destroy the control
queues either; only ice_deinit_hw() does.
I will address this in a separate fix for -net rather than in this series.
[ ... ]
diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c
b/drivers/net/ethernet/intel/ice/ice_adapter.c
index 2dc3629d6d0f6..c6301341ccaef 100644
--- a/drivers/net/ethernet/intel/ice/ice_adapter.c
+++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
[ ... ]
@@ -93,59 +80,37 @@ static void ice_adapter_free(struct ice_adapter *adapter)
[ ... ]
-void ice_adapter_put(struct pci_dev *pdev)
+void ice_adapter_put(struct ice_adapter *adapter)
{
- struct ice_adapter *adapter;
- unsigned long index;
-
- index = ice_adapter_xa_index(pdev);
- scoped_guard(mutex, &ice_adapters_mutex) {
- adapter = xa_load(&ice_adapters, index);
- if (WARN_ON(!adapter))
- return;
- if (!refcount_dec_and_test(&adapter->refcount))
- return;
-
- WARN_ON(xa_erase(&ice_adapters, index) != adapter);
- }
- ice_adapter_free(adapter);
+ devlink_shd_put(adapter->devlink);
}
[Severity: High]
Will this trigger a NULL pointer dereference regression if the driver is
probed in recovery mode?
If the driver probes in recovery mode, pf->adapter is never allocated and
remains NULL. During ice_remove(), the code checks
ice_is_recovery_mode(&pf->hw) to determine if it should perform a
recovery-mode early return or a full teardown.
Since ice_is_recovery_mode() reads a live volatile hardware register, if
the firmware clears this bit after probe, the check bypasses the early
return and falls through to normal teardown:
drivers/net/ethernet/intel/ice/ice_main.c:ice_remove() {
...
ice_adapter_put(pf->adapter);
ice_deinit_hw(&pf->hw);
...
}
The previous xarray lookup safely returned early on NULL, but the new code
unconditionally dereferences adapter->devlink. Will this crash on driver
unload or device unbind when transitioning out of recovery mode?
This is pre-existing, not a regression of this patch. If GL_MNG_FWSM
changes after a recovery-mode probe, ice_remove() goes down the full
teardown path for a PF that was never fully initialized. It crashes
before reaching ice_adapter_put(): ice_unload() calls
ice_tc_indir_block_unregister() with a NULL main VSI and dereferences
vsi->netdev.
The proper fix is to remember at probe time that the PF is in recovery
mode, and check that in ice_remove() instead of reading the register
again. A NULL check in ice_adapter_put() would only hide one of the
symptoms. I will send that as a separate fix for -net.
