[PATCH v2] iommu: fix an incorrect NULL check on list iterator

[PATCH v2] iommu: fix an incorrect NULL check on list iterator

[Xiaomeng Tong]

The bug is here:
	if (!iommu || iommu->dev->of_node != spec->np) {

The list iterator value 'iommu' will *always* be set and non-NULL by
list_for_each_entry(), so it is incorrect to assume that the iterator
value will be NULL if the list is empty or no element is found (in fact,
it will point to a invalid structure object containing HEAD).

To fix the bug, use a new value 'iter' as the list iterator, while use
the old value 'iommu' as a dedicated variable to point to the found one,
and remove the unneeded check for 'iommu->dev->of_node != spec->np'
outside the loop.

Cc: stable@vger.kernel.org
Fixes: f78ebca8ff3d6 ("iommu/msm: Add support for generic master bindings")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
---
changes since v1:
 - add a new iter variable (suggested by Joerg Roedel)

v1: https://lore.kernel.org/all/20220327053558.2821-1-xiam0nd.tong@gmail.com/
---
 drivers/iommu/msm_iommu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 3a38352b603f..41a3231a6d13 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -615,16 +615,17 @@ static void insert_iommu_master(struct device *dev,
 static int qcom_iommu_of_xlate(struct device *dev,
 			       struct of_phandle_args *spec)
 {
-	struct msm_iommu_dev *iommu;
+	struct msm_iommu_dev *iommu = NULL, *iter;
 	unsigned long flags;
 
 	spin_lock_irqsave(&msm_iommu_lock, flags);
-	list_for_each_entry(iommu, &qcom_iommu_devices, dev_node)
-		if (iommu->dev->of_node == spec->np)
+	list_for_each_entry(iter, &qcom_iommu_devices, dev_node)
+		if (iter->dev->of_node == spec->np) {
+			iommu = iter;
 			break;
+		}
 
-	if (!iommu || iommu->dev->of_node != spec->np) {
+	if (!iommu) {
 		ret = -ENODEV;
 		goto fail;
 	}
-- 
2.17.1

Re: [PATCH v2] iommu: fix an incorrect NULL check on list iterator

[Joerg Roedel]

On Sun, May 01, 2022 at 09:28:23PM +0800, Xiaomeng Tong wrote:
> The bug is here:
> 	if (!iommu || iommu->dev->of_node != spec->np) {
> 
> The list iterator value 'iommu' will *always* be set and non-NULL by
> list_for_each_entry(), so it is incorrect to assume that the iterator
> value will be NULL if the list is empty or no element is found (in fact,
> it will point to a invalid structure object containing HEAD).
> 
> To fix the bug, use a new value 'iter' as the list iterator, while use
> the old value 'iommu' as a dedicated variable to point to the found one,
> and remove the unneeded check for 'iommu->dev->of_node != spec->np'
> outside the loop.
> 
> Cc: stable@vger.kernel.org
> Fixes: f78ebca8ff3d6 ("iommu/msm: Add support for generic master bindings")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> ---
> changes since v1:
>  - add a new iter variable (suggested by Joerg Roedel)

This is now applied. I had to manually apply it because the patch was
malformed at line 36 and git-am complained.

Regards,

	Joerg

[PATCH v2] iommu: fix an incorrect NULL check on list iterator

[Xiaomeng Tong]

The bug is here:
	if (!iommu || iommu->dev->of_node != spec->np) {

The list iterator value 'iommu' will *always* be set and non-NULL by
list_for_each_entry(), so it is incorrect to assume that the iterator
value will be NULL if the list is empty or no element is found (in fact,
it will point to a invalid structure object containing HEAD).

To fix the bug, use a new value 'iter' as the list iterator, while use
the old value 'iommu' as a dedicated variable to point to the found one,
and remove the unneeded check for 'iommu->dev->of_node != spec->np'
outside the loop.

Cc: stable@vger.kernel.org
Fixes: f78ebca8ff3d6 ("iommu/msm: Add support for generic master bindings")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
---
changes since v1:
 - add a new iter variable (suggested by Joerg Roedel)

v1: https://lore.kernel.org/all/20220327053558.2821-1-xiam0nd.tong@gmail.com/
---
 drivers/iommu/msm_iommu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 3a38352b603f..41a3231a6d13 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -615,16 +615,17 @@ static void insert_iommu_master(struct device *dev,
 static int qcom_iommu_of_xlate(struct device *dev,
 			       struct of_phandle_args *spec)
 {
-	struct msm_iommu_dev *iommu;
+	struct msm_iommu_dev *iommu = NULL, *iter;
 	unsigned long flags;
-	int ret = 0;
 
 	spin_lock_irqsave(&msm_iommu_lock, flags);
-	list_for_each_entry(iommu, &qcom_iommu_devices, dev_node)
-		if (iommu->dev->of_node == spec->np)
+	list_for_each_entry(iter, &qcom_iommu_devices, dev_node)
+		if (iter->dev->of_node == spec->np) {
+			iommu = iter;
 			break;
+		}
 
-	if (!iommu || iommu->dev->of_node != spec->np) {
+	if (!iommu) {
 		ret = -ENODEV;
 		goto fail;
 	}
-- 
2.17.1

Re: [PATCH v2] iommu: fix an incorrect NULL check on list iterator

[kernel test robot]

Hi Xiaomeng,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on joro-iommu/next]
[also build test ERROR on v5.18-rc5 next-20220429]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Xiaomeng-Tong/iommu-fix-an-incorrect-NULL-check-on-list-iterator/20220501-211400
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20220502/202205021754.GETHfNnS-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/99e334beef5d5be25ed19d3142d16000f0a1986d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Xiaomeng-Tong/iommu-fix-an-incorrect-NULL-check-on-list-iterator/20220501-211400
        git checkout 99e334beef5d5be25ed19d3142d16000f0a1986d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/iommu/msm_iommu.c: In function 'qcom_iommu_of_xlate':
>> drivers/iommu/msm_iommu.c:629:17: error: 'ret' undeclared (first use in this function); did you mean 'net'?
     629 |                 ret = -ENODEV;
         |                 ^~~
         |                 net
   drivers/iommu/msm_iommu.c:629:17: note: each undeclared identifier is reported only once for each function it appears in
   drivers/iommu/msm_iommu.c:638:1: error: control reaches end of non-void function [-Werror=return-type]
     638 | }
         | ^
   cc1: some warnings being treated as errors


vim +629 drivers/iommu/msm_iommu.c

f78ebca8ff3d61 Sricharan R   2016-06-13  614  
f78ebca8ff3d61 Sricharan R   2016-06-13  615  static int qcom_iommu_of_xlate(struct device *dev,
f78ebca8ff3d61 Sricharan R   2016-06-13  616  			       struct of_phandle_args *spec)
f78ebca8ff3d61 Sricharan R   2016-06-13  617  {
99e334beef5d5b Xiaomeng Tong 2022-05-01  618  	struct msm_iommu_dev *iommu = NULL, *iter;
f78ebca8ff3d61 Sricharan R   2016-06-13  619  	unsigned long flags;
f78ebca8ff3d61 Sricharan R   2016-06-13  620  
f78ebca8ff3d61 Sricharan R   2016-06-13  621  	spin_lock_irqsave(&msm_iommu_lock, flags);
99e334beef5d5b Xiaomeng Tong 2022-05-01  622  	list_for_each_entry(iter, &qcom_iommu_devices, dev_node)
99e334beef5d5b Xiaomeng Tong 2022-05-01  623  		if (iter->dev->of_node == spec->np) {
99e334beef5d5b Xiaomeng Tong 2022-05-01  624  			iommu = iter;
f78ebca8ff3d61 Sricharan R   2016-06-13  625  			break;
99e334beef5d5b Xiaomeng Tong 2022-05-01  626  		}
f78ebca8ff3d61 Sricharan R   2016-06-13  627  
99e334beef5d5b Xiaomeng Tong 2022-05-01  628  	if (!iommu) {
f78ebca8ff3d61 Sricharan R   2016-06-13 @629  		ret = -ENODEV;
f78ebca8ff3d61 Sricharan R   2016-06-13  630  		goto fail;
f78ebca8ff3d61 Sricharan R   2016-06-13  631  	}
f78ebca8ff3d61 Sricharan R   2016-06-13  632  
bb5bdc5ab7f133 Xiaoke Wang   2022-04-28  633  	ret = insert_iommu_master(dev, &iommu, spec);
f78ebca8ff3d61 Sricharan R   2016-06-13  634  fail:
f78ebca8ff3d61 Sricharan R   2016-06-13  635  	spin_unlock_irqrestore(&msm_iommu_lock, flags);
f78ebca8ff3d61 Sricharan R   2016-06-13  636  
f78ebca8ff3d61 Sricharan R   2016-06-13  637  	return ret;
f78ebca8ff3d61 Sricharan R   2016-06-13  638  }
f78ebca8ff3d61 Sricharan R   2016-06-13  639  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp