* [PATCH] iwlwifi: fix a potential NULL pointer dereference @ 2019-09-18 18:11 Allen Pais 2019-09-18 20:19 ` kbuild test robot 2019-09-19 7:08 ` Johannes Berg 0 siblings, 2 replies; 6+ messages in thread From: Allen Pais @ 2019-09-18 18:11 UTC (permalink / raw) To: kvalo; +Cc: davem, linux-wireless, linux-kernel alloc_workqueue is not checked for errors and as a result, a potential NULL dereference could occur. Signed-off-by: Allen Pais <allen.pais@oracle.com> --- drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c index db62c83..276c26b 100644 --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c @@ -3655,6 +3655,11 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev, trans_pcie->rba.alloc_wq = alloc_workqueue("rb_allocator", WQ_HIGHPRI | WQ_UNBOUND, 1); + if (unlikely(!trans_pcie->rba.alloc_wq)) { + return -ENOMEM; + goto out_free_ict; + } + INIT_WORK(&trans_pcie->rba.rx_alloc, iwl_pcie_rx_allocator_work); #ifdef CONFIG_IWLWIFI_PCIE_RTPM -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iwlwifi: fix a potential NULL pointer dereference 2019-09-18 18:11 [PATCH] iwlwifi: fix a potential NULL pointer dereference Allen Pais @ 2019-09-18 20:19 ` kbuild test robot 2019-09-19 7:08 ` Johannes Berg 1 sibling, 0 replies; 6+ messages in thread From: kbuild test robot @ 2019-09-18 20:19 UTC (permalink / raw) To: Allen Pais; +Cc: kbuild-all, kvalo, davem, linux-wireless, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2728 bytes --] Hi Allen, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [cannot apply to v5.3 next-20190917] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Allen-Pais/iwlwifi-fix-a-potential-NULL-pointer-dereference/20190919-021453 config: mips-allmodconfig (attached as .config) compiler: mips-linux-gcc (GCC) 7.4.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.4.0 make.cross ARCH=mips If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All warnings (new ones prefixed by >>): drivers/net/wireless/intel/iwlwifi/pcie/trans.c: In function 'iwl_trans_pcie_alloc': >> drivers/net/wireless/intel/iwlwifi/pcie/trans.c:3659:10: warning: return makes pointer from integer without a cast [-Wint-conversion] return -ENOMEM; ^ vim +3659 drivers/net/wireless/intel/iwlwifi/pcie/trans.c 3625 3626 iwl_pcie_set_interrupt_capa(pdev, trans); 3627 trans->hw_id = (pdev->device << 16) + pdev->subsystem_device; 3628 snprintf(trans->hw_id_str, sizeof(trans->hw_id_str), 3629 "PCI ID: 0x%04X:0x%04X", pdev->device, pdev->subsystem_device); 3630 3631 /* Initialize the wait queue for commands */ 3632 init_waitqueue_head(&trans_pcie->wait_command_queue); 3633 3634 init_waitqueue_head(&trans_pcie->d0i3_waitq); 3635 3636 if (trans_pcie->msix_enabled) { 3637 ret = iwl_pcie_init_msix_handler(pdev, trans_pcie); 3638 if (ret) 3639 goto out_no_pci; 3640 } else { 3641 ret = iwl_pcie_alloc_ict(trans); 3642 if (ret) 3643 goto out_no_pci; 3644 3645 ret = devm_request_threaded_irq(&pdev->dev, pdev->irq, 3646 iwl_pcie_isr, 3647 iwl_pcie_irq_handler, 3648 IRQF_SHARED, DRV_NAME, trans); 3649 if (ret) { 3650 IWL_ERR(trans, "Error allocating IRQ %d\n", pdev->irq); 3651 goto out_free_ict; 3652 } 3653 trans_pcie->inta_mask = CSR_INI_SET_MASK; 3654 } 3655 3656 trans_pcie->rba.alloc_wq = alloc_workqueue("rb_allocator", 3657 WQ_HIGHPRI | WQ_UNBOUND, 1); 3658 if (unlikely(!trans_pcie->rba.alloc_wq)) { > 3659 return -ENOMEM; 3660 goto out_free_ict; 3661 } 3662 3663 INIT_WORK(&trans_pcie->rba.rx_alloc, iwl_pcie_rx_allocator_work); 3664 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 61642 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iwlwifi: fix a potential NULL pointer dereference 2019-09-18 18:11 [PATCH] iwlwifi: fix a potential NULL pointer dereference Allen Pais 2019-09-18 20:19 ` kbuild test robot @ 2019-09-19 7:08 ` Johannes Berg 2019-09-19 14:07 ` Allen 1 sibling, 1 reply; 6+ messages in thread From: Johannes Berg @ 2019-09-19 7:08 UTC (permalink / raw) To: Allen Pais, kvalo; +Cc: davem, linux-wireless, linux-kernel On Wed, 2019-09-18 at 23:41 +0530, Allen Pais wrote: > alloc_workqueue is not checked for errors and as a result, > a potential NULL dereference could occur. Wonder why this is coming out now ... but I don't think kmalloc() was ever 'fixed' to fail for small allocations, so I guess this will never fail? Anyway, as 0-day bot pointed out, this isn't really right. The cleanup paths here are also tricky, so I arrived at this patch a few days ago: diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c index eb544811759d..882fdf7e5e7b 100644 --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c @@ -3530,6 +3530,15 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev, spin_lock_init(&trans_pcie->reg_lock); mutex_init(&trans_pcie->mutex); init_waitqueue_head(&trans_pcie->ucode_write_waitq); + + trans_pcie->rba.alloc_wq = alloc_workqueue("rb_allocator", + WQ_HIGHPRI | WQ_UNBOUND, 1); + if (!trans_pcie->rba.alloc_wq) { + ret = -ENOMEM; + goto out_free_trans; + } + INIT_WORK(&trans_pcie->rba.rx_alloc, iwl_pcie_rx_allocator_work); + trans_pcie->tso_hdr_page = alloc_percpu(struct iwl_tso_hdr_page); if (!trans_pcie->tso_hdr_page) { ret = -ENOMEM; @@ -3664,10 +3673,6 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev, trans_pcie->inta_mask = CSR_INI_SET_MASK; } - trans_pcie->rba.alloc_wq = alloc_workqueue("rb_allocator", - WQ_HIGHPRI | WQ_UNBOUND, 1); - INIT_WORK(&trans_pcie->rba.rx_alloc, iwl_pcie_rx_allocator_work); - #ifdef CPTCFG_IWLWIFI_DEBUGFS trans_pcie->fw_mon_data.state = IWL_FW_MON_DBGFS_STATE_CLOSED; mutex_init(&trans_pcie->fw_mon_data.mutex); @@ -3681,6 +3686,8 @@ out_free_ict: iwl_pcie_free_ict(trans); out_no_pci: free_percpu(trans_pcie->tso_hdr_page); + destroy_workqueue(trans_pcie->rba.alloc_wq); +out_free_trans: iwl_trans_free(trans); return ERR_PTR(ret); } johannes ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iwlwifi: fix a potential NULL pointer dereference 2019-09-19 7:08 ` Johannes Berg @ 2019-09-19 14:07 ` Allen 2019-09-19 14:47 ` Johannes Berg 0 siblings, 1 reply; 6+ messages in thread From: Allen @ 2019-09-19 14:07 UTC (permalink / raw) To: Johannes Berg, kvalo; +Cc: davem, linux-wireless, linux-kernel > > Anyway, as 0-day bot pointed out, this isn't really right. The cleanup > paths here are also tricky, so I arrived at this patch a few days ago: My bad, I should have looked at the cleanup path. > > diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c > index eb544811759d..882fdf7e5e7b 100644 > --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c > @@ -3530,6 +3530,15 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev, > spin_lock_init(&trans_pcie->reg_lock); > mutex_init(&trans_pcie->mutex); > init_waitqueue_head(&trans_pcie->ucode_write_waitq); > + > + trans_pcie->rba.alloc_wq = alloc_workqueue("rb_allocator", > + WQ_HIGHPRI | WQ_UNBOUND, 1); > + if (!trans_pcie->rba.alloc_wq) { I would like to stick to if(unlikely(!trans_pcie->rba.alloc_wq) just for consistency. Let me know if I could add your SOB and send out V2. - Allen > + ret = -ENOMEM; > + goto out_free_trans; > + } > + INIT_WORK(&trans_pcie->rba.rx_alloc, iwl_pcie_rx_allocator_work); > + > trans_pcie->tso_hdr_page = alloc_percpu(struct iwl_tso_hdr_page); > if (!trans_pcie->tso_hdr_page) { > ret = -ENOMEM; > @@ -3664,10 +3673,6 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev, > trans_pcie->inta_mask = CSR_INI_SET_MASK; > } > > - trans_pcie->rba.alloc_wq = alloc_workqueue("rb_allocator", > - WQ_HIGHPRI | WQ_UNBOUND, 1); > - INIT_WORK(&trans_pcie->rba.rx_alloc, iwl_pcie_rx_allocator_work); > - > #ifdef CPTCFG_IWLWIFI_DEBUGFS > trans_pcie->fw_mon_data.state = IWL_FW_MON_DBGFS_STATE_CLOSED; > mutex_init(&trans_pcie->fw_mon_data.mutex); > @@ -3681,6 +3686,8 @@ out_free_ict: > iwl_pcie_free_ict(trans); > out_no_pci: > free_percpu(trans_pcie->tso_hdr_page); > + destroy_workqueue(trans_pcie->rba.alloc_wq); > +out_free_trans: > iwl_trans_free(trans); > return ERR_PTR(ret); > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iwlwifi: fix a potential NULL pointer dereference 2019-09-19 14:07 ` Allen @ 2019-09-19 14:47 ` Johannes Berg 2019-09-19 15:29 ` Allen 0 siblings, 1 reply; 6+ messages in thread From: Johannes Berg @ 2019-09-19 14:47 UTC (permalink / raw) To: Allen, kvalo; +Cc: davem, linux-wireless, linux-kernel On Thu, 2019-09-19 at 19:37 +0530, Allen wrote: > > > > + trans_pcie->rba.alloc_wq = alloc_workqueue("rb_allocator", > > + WQ_HIGHPRI | WQ_UNBOUND, 1); > > + if (!trans_pcie->rba.alloc_wq) { > > I would like to stick to if(unlikely(!trans_pcie->rba.alloc_wq) just > for consistency. That's just clutter, this path gets called exactly once in the lifetime of most systems ... > Let me know if I could add your SOB and send out V2. No no, I've already sent the patch on the way internally :) johannes ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iwlwifi: fix a potential NULL pointer dereference 2019-09-19 14:47 ` Johannes Berg @ 2019-09-19 15:29 ` Allen 0 siblings, 0 replies; 6+ messages in thread From: Allen @ 2019-09-19 15:29 UTC (permalink / raw) To: Johannes Berg, kvalo; +Cc: davem, linux-wireless, linux-kernel >>> >>> + trans_pcie->rba.alloc_wq = alloc_workqueue("rb_allocator", >>> + WQ_HIGHPRI | WQ_UNBOUND, 1); >>> + if (!trans_pcie->rba.alloc_wq) { >> >> I would like to stick to if(unlikely(!trans_pcie->rba.alloc_wq) just >> for consistency. > > That's just clutter, this path gets called exactly once in the lifetime > of most systems ... > >> Let me know if I could add your SOB and send out V2. > > No no, I've already sent the patch on the way internally :) Great. Thank you. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-19 15:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-18 18:11 [PATCH] iwlwifi: fix a potential NULL pointer dereference Allen Pais 2019-09-18 20:19 ` kbuild test robot 2019-09-19 7:08 ` Johannes Berg 2019-09-19 14:07 ` Allen 2019-09-19 14:47 ` Johannes Berg 2019-09-19 15:29 ` Allen
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).