* [PATCH iwl-net 0/2] ice: Fix NULL pointer dereference and leak in IDC @ 2025-06-24 14:26 Emil Tantilov 2025-06-24 14:26 ` [PATCH iwl-net 1/2] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset Emil Tantilov 2025-06-24 14:26 ` [PATCH iwl-net 2/2] ice: fix possible leak in ice_plug_aux_dev() error path Emil Tantilov 0 siblings, 2 replies; 5+ messages in thread From: Emil Tantilov @ 2025-06-24 14:26 UTC (permalink / raw) To: intel-wired-lan Cc: netdev, Aleksandr.Loktionov, przemyslaw.kitszel, david.m.ertman, anthony.l.nguyen, michal.swiatkowski, andrew+netdev, davem, edumazet, kuba, pabeni 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH iwl-net 1/2] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset 2025-06-24 14:26 [PATCH iwl-net 0/2] ice: Fix NULL pointer dereference and leak in IDC Emil Tantilov @ 2025-06-24 14:26 ` Emil Tantilov 2025-06-24 17:13 ` Brett Creeley 2025-06-24 14:26 ` [PATCH iwl-net 2/2] ice: fix possible leak in ice_plug_aux_dev() error path Emil Tantilov 1 sibling, 1 reply; 5+ messages in thread From: Emil Tantilov @ 2025-06-24 14:26 UTC (permalink / raw) To: intel-wired-lan Cc: netdev, Aleksandr.Loktionov, przemyslaw.kitszel, david.m.ertman, anthony.l.nguyen, michal.swiatkowski, andrew+netdev, davem, edumazet, kuba, pabeni 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH iwl-net 1/2] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset 2025-06-24 14:26 ` [PATCH iwl-net 1/2] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset Emil Tantilov @ 2025-06-24 17:13 ` Brett Creeley 2025-06-25 22:13 ` Tantilov, Emil S 0 siblings, 1 reply; 5+ messages in thread From: Brett Creeley @ 2025-06-24 17:13 UTC (permalink / raw) To: Emil Tantilov, intel-wired-lan Cc: netdev, Aleksandr.Loktionov, przemyslaw.kitszel, david.m.ertman, anthony.l.nguyen, michal.swiatkowski, andrew+netdev, davem, edumazet, kuba, pabeni 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH iwl-net 1/2] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset 2025-06-24 17:13 ` Brett Creeley @ 2025-06-25 22:13 ` Tantilov, Emil S 0 siblings, 0 replies; 5+ messages in thread From: Tantilov, Emil S @ 2025-06-25 22:13 UTC (permalink / raw) To: Brett Creeley, intel-wired-lan Cc: netdev, Aleksandr.Loktionov, przemyslaw.kitszel, david.m.ertman, anthony.l.nguyen, michal.swiatkowski, andrew+netdev, davem, edumazet, kuba, pabeni 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 >> >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH iwl-net 2/2] ice: fix possible leak in ice_plug_aux_dev() error path 2025-06-24 14:26 [PATCH iwl-net 0/2] ice: Fix NULL pointer dereference and leak in IDC Emil Tantilov 2025-06-24 14:26 ` [PATCH iwl-net 1/2] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset Emil Tantilov @ 2025-06-24 14:26 ` Emil Tantilov 1 sibling, 0 replies; 5+ messages in thread From: Emil Tantilov @ 2025-06-24 14:26 UTC (permalink / raw) To: intel-wired-lan Cc: netdev, Aleksandr.Loktionov, przemyslaw.kitszel, david.m.ertman, anthony.l.nguyen, michal.swiatkowski, andrew+netdev, davem, edumazet, kuba, pabeni 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-06-25 22:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-06-24 14:26 [PATCH iwl-net 0/2] ice: Fix NULL pointer dereference and leak in IDC Emil Tantilov 2025-06-24 14:26 ` [PATCH iwl-net 1/2] ice: fix NULL pointer dereference in ice_unplug_aux_dev() on reset Emil Tantilov 2025-06-24 17:13 ` Brett Creeley 2025-06-25 22:13 ` Tantilov, Emil S 2025-06-24 14:26 ` [PATCH iwl-net 2/2] ice: fix possible leak in ice_plug_aux_dev() error path Emil Tantilov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).