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 --
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? [ ... ] > 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? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
