[PATCH iwl-net 0/2] ice: Fix NULL pointer dereference and leak in IDC

[PATCH iwl-net 0/2] ice: Fix NULL pointer dereference and leak in IDC

[Emil Tantilov]

Resolve NULL pointer dereference on driver load when RDMA is not supported.
During reset the driver attempts to remove an auxiliary device that does
not exist. In addition, fix a memory leak in ice_plug_aux_dev() error path.

Emil Tantilov (2):
  ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset
  ice: fix possible leak in ice_plug_aux_dev() error path

 drivers/net/ethernet/intel/ice/ice.h     |  1 +
 drivers/net/ethernet/intel/ice/ice_idc.c | 29 ++++++++++++++----------
 2 files changed, 18 insertions(+), 12 deletions(-)

-- 
2.37.3

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

[Emil Tantilov]

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.

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().

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);
 
 	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;
+
 	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

[PATCH iwl-net 2/2] ice: fix possible leak in ice_plug_aux_dev() error path

[Emil Tantilov]

Fix a memory leak in the error path where kfree(iadev) was not called
following an error in auxiliary_device_add().

Fixes: f9f5301e7e2d ("ice: Register auxiliary device to provide RDMA")
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_idc.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
index 420d45c2558b..8c4a3dc22a7c 100644
--- a/drivers/net/ethernet/intel/ice/ice_idc.c
+++ b/drivers/net/ethernet/intel/ice/ice_idc.c
@@ -322,16 +322,12 @@ int ice_plug_aux_dev(struct ice_pf *pf)
 		"roce" : "iwarp";
 
 	ret = auxiliary_device_init(adev);
-	if (ret) {
-		kfree(iadev);
-		return ret;
-	}
+	if (ret)
+		goto free_iadev;
 
 	ret = auxiliary_device_add(adev);
-	if (ret) {
-		auxiliary_device_uninit(adev);
-		return ret;
-	}
+	if (ret)
+		goto aux_dev_uninit;
 
 	mutex_lock(&pf->adev_mutex);
 	cdev->adev = adev;
@@ -339,6 +335,13 @@ int ice_plug_aux_dev(struct ice_pf *pf)
 	set_bit(ICE_FLAG_AUX_DEV_CREATED, pf->flags);
 
 	return 0;
+
+aux_dev_uninit:
+	auxiliary_device_uninit(adev);
+free_iadev:
+	kfree(iadev);
+
+	return ret;
 }
 
 /* ice_unplug_aux_dev - unregister and free AUX device
-- 
2.37.3

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

[Brett Creeley]

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

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

[Tantilov, Emil S]

On 6/24/2025 10:13 AM, Brett Creeley wrote:
> 
> 
> 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.

This bug is seen on a NIC with no RDMA support, so the RDMA init/deinit
flow will not happen. It is protected by the !ice_is_rdma_ena(pf)
check.

> 
>>
>> 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.
> 

Avoid defensive programming. The use of the flag makes this path
deterministic (only attempt to remove the auxbus device if we know for a
fact that it was created). The NULL pointer checks provide protection 
for this crash, but only by association. Say there was another bug in 
that path that cleared them prematurely, with the NULL pointer checks in 
place you will avoid crashing, but fail to remove the auxbus device, or 
if such a patch was introduced, you'd want to crash, which gives 
validation a chance to catch it.

>>
>> 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?

The purpose of this bit is to mark the successful creation of the auxbus 
device, I am not aware of any action that needs to be taken should 
ice_plug_aux_dev() be called with the bit set and there was no check 
like this previously.

> 
>>
>>          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.

Understood. I am not opposed to changing the patch to add another NULL 
check if that is the consensus. Another option is to add the same check 
for !ice_is_rdma_ena(pf) like in the other functions. This should work 
at present, because the driver fails to load if RDMA init errors out.

Thanks,
Emil

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