Re: [PATCH iwl-net 1/2] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset

[Brett Creeley <bcreeley@amd.com>]

On 6/24/2025 7:26 AM, Emil Tantilov wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> Issuing a reset when the driver is loaded without RDMA support, will
> results in a crash as it attempts to remove RDMA's non-existent auxbus
> device:
> echo 1 > /sys/class/net/<if>/device/reset
> 
> BUG: kernel NULL pointer dereference, address: 0000000000000008
> ...
> RIP: 0010:ice_unplug_aux_dev+0x29/0x70 [ice]
> ...
> Call Trace:
> <TASK>
> ice_prepare_for_reset+0x77/0x260 [ice]
> pci_dev_save_and_disable+0x2c/0x70
> pci_reset_function+0x88/0x130
> reset_store+0x5a/0xa0
> kernfs_fop_write_iter+0x15e/0x210
> vfs_write+0x273/0x520
> ksys_write+0x6b/0xe0
> do_syscall_64+0x79/0x3b0
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> ice_unplug_aux_dev() checks pf->cdev_info->adev for NULL pointer, but
> pf->cdev_info will also be NULL, leading to the deref in the trace above.

What about in ice_deinit_rdma(), can the cdev_info also be NULL there? 
If so kfree(pf->cdev_info->iddc_priv) will result in a similar trace on 
driver unload.

> 
> Introduce a flag to be set when the creation of the auxbus device is
> successful, to avoid multiple NULL pointer checks in ice_unplug_aux_dev().

IMHO adding a state flag to prevent NULL pointer checks in the control 
path isn't enough justification unless there's something I'm missing here.

> 
> Fixes: c24a65b6a27c7 ("iidc/ice/irdma: Update IDC to support multiple consumers")
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice.h     |  1 +
>   drivers/net/ethernet/intel/ice/ice_idc.c | 10 ++++++----
>   2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index ddd0ad68185b..0ef11b7ab477 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -509,6 +509,7 @@ enum ice_pf_flags {
>          ICE_FLAG_LINK_LENIENT_MODE_ENA,
>          ICE_FLAG_PLUG_AUX_DEV,
>          ICE_FLAG_UNPLUG_AUX_DEV,
> +       ICE_FLAG_AUX_DEV_CREATED,
>          ICE_FLAG_MTU_CHANGED,
>          ICE_FLAG_GNSS,                  /* GNSS successfully initialized */
>          ICE_FLAG_DPLL,                  /* SyncE/PTP dplls initialized */
> diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
> index 6ab53e430f91..420d45c2558b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_idc.c
> +++ b/drivers/net/ethernet/intel/ice/ice_idc.c
> @@ -336,6 +336,7 @@ int ice_plug_aux_dev(struct ice_pf *pf)
>          mutex_lock(&pf->adev_mutex);
>          cdev->adev = adev;
>          mutex_unlock(&pf->adev_mutex);
> +       set_bit(ICE_FLAG_AUX_DEV_CREATED, pf->flags);

What if this bit is set already, should ice_plug_aux_dev() be executed?

> 
>          return 0;
>   }
> @@ -347,15 +348,16 @@ void ice_unplug_aux_dev(struct ice_pf *pf)
>   {
>          struct auxiliary_device *adev;
> 
> +       if (!test_and_clear_bit(ICE_FLAG_AUX_DEV_CREATED, pf->flags))
> +               return;
> +

To re-iterate my comment above, I think the driver should just check if 
pf->cdev_info is valid before de-referencing it. Also, the local adev 
variable will have to be set to NULL to handle this case.

Brett

>          mutex_lock(&pf->adev_mutex);
>          adev = pf->cdev_info->adev;
>          pf->cdev_info->adev = NULL;


>          mutex_unlock(&pf->adev_mutex);
> 
> -       if (adev) {
> -               auxiliary_device_delete(adev);
> -               auxiliary_device_uninit(adev);
> -       }
> +       auxiliary_device_delete(adev);
> +       auxiliary_device_uninit(adev);
>   }
> 
>   /**
> --
> 2.37.3
> 
>