[PATCH] iwlwifi: fix a potential NULL pointer dereference

[PATCH] iwlwifi: fix a potential NULL pointer dereference

[Allen Pais]

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

Re: [PATCH] iwlwifi: fix a potential NULL pointer dereference

[kbuild test robot]

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

Re: [PATCH] iwlwifi: fix a potential NULL pointer dereference

[Johannes Berg]

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

Re: [PATCH] iwlwifi: fix a potential NULL pointer dereference

[Allen]

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

Re: [PATCH] iwlwifi: fix a potential NULL pointer dereference

[Johannes Berg]

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

Re: [PATCH] iwlwifi: fix a potential NULL pointer dereference

[Allen]

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