| Message ID | 1568830262-5529-1-git-send-email-allen.pais@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Luca Coelho |
| Headers | show |
| Series | iwlwifi: fix a potential NULL pointer dereference | expand |
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
> > 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); > } >
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
>>> >>> + 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.
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
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(+)