[PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference

[PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference

[Rickard Strandqvist]

This start as a fix for possible null pointer dereference.
But after discussion with especially Wolfram, Jingoo and Emil it was
decided to convert the code to uses Managed Device Resource instead.

Rickard Strandqvist (1):
  i2c: busses: i2c-pxa.c:  Fix for possible null pointer dereference

 drivers/i2c/busses/i2c-pxa.c |   65 ++++++++++++++----------------------------
 1 file changed, 22 insertions(+), 43 deletions(-)

-- 
1.7.10.4

[PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference

[Rickard Strandqvist]

Fix for possible null pointer dereference, and there was a risk for
memory leak if something unexpected happens and the function returns.
It now uses the Managed Device Resource instead.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist-IW2WV5XWFqGZkjO+N0TKoMugMpMbD5Xr@public.gmane.org>
---
 drivers/i2c/busses/i2c-pxa.c |   65 ++++++++++++++----------------------------
 1 file changed, 22 insertions(+), 43 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index be671f7..0d0605c 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1141,11 +1141,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	struct resource *res = NULL;
 	int ret, irq;
 
-	i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
-	if (!i2c) {
-		ret = -ENOMEM;
-		goto emalloc;
-	}
+	i2c = devm_kzalloc(&dev->dev, sizeof(struct pxa_i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
 
 	/* Default adapter num to device id; i2c_pxa_probe_dt can override. */
 	i2c->adap.nr = dev->id;
@@ -1154,19 +1152,16 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	if (ret > 0)
 		ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
 	if (ret < 0)
-		goto eclk;
+		return ret;
 
 	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq(dev, 0);
-	if (res == NULL || irq < 0) {
-		ret = -ENODEV;
-		goto eclk;
-	}
+	if (res == NULL || irq < 0)
+		return -ENODEV;
 
-	if (!request_mem_region(res->start, resource_size(res), res->name)) {
-		ret = -ENOMEM;
-		goto eclk;
-	}
+	if (!devm_request_mem_region(&dev->dev, res->start,
+				resource_size(res), res->name))
+		return -ENOMEM;
 
 	i2c->adap.owner   = THIS_MODULE;
 	i2c->adap.retries = 5;
@@ -1176,17 +1171,13 @@ static int i2c_pxa_probe(struct platform_device *dev)
 
 	strlcpy(i2c->adap.name, "pxa_i2c-i2c", sizeof(i2c->adap.name));
 
-	i2c->clk = clk_get(&dev->dev, NULL);
-	if (IS_ERR(i2c->clk)) {
-		ret = PTR_ERR(i2c->clk);
-		goto eclk;
-	}
+	i2c->clk = devm_clk_get(&dev->dev, NULL);
+	if (IS_ERR(i2c->clk))
+		return PTR_ERR(i2c->clk);
 
-	i2c->reg_base = ioremap(res->start, resource_size(res));
-	if (!i2c->reg_base) {
-		ret = -EIO;
-		goto eremap;
-	}
+	i2c->reg_base = devm_ioremap_resource(&adev->dev, res));
+	if (IS_ERR(i2c->reg_base))
+		return -EIO;
 
 	i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
 	i2c->reg_idbr = i2c->reg_base + pxa_reg_layout[i2c_type].idbr;
@@ -1227,10 +1218,12 @@ static int i2c_pxa_probe(struct platform_device *dev)
 		i2c->adap.algo = &i2c_pxa_pio_algorithm;
 	} else {
 		i2c->adap.algo = &i2c_pxa_algorithm;
-		ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
-				  dev_name(&dev->dev), i2c);
-		if (ret)
-			goto ereqirq;
+		ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler,
+				 IRQF_SHARED, dev_name(&dev->dev), i2c);
+		if (ret) {
+			clk_disable_unprepare(i2c->clk);
+			return ret;
+		}
 	}
 
 	i2c_pxa_reset(i2c);
@@ -1244,7 +1237,7 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	ret = i2c_add_numbered_adapter(&i2c->adap);
 	if (ret < 0) {
 		printk(KERN_INFO "I2C: Failed to add bus\n");
-		goto eadapt;
+		return ret;
 	}
 
 	platform_set_drvdata(dev, i2c);
@@ -1257,20 +1250,6 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	       dev_name(&i2c->adap.dev));
 #endif
 	return 0;
-
-eadapt:
-	if (!i2c->use_pio)
-		free_irq(irq, i2c);
-ereqirq:
-	clk_disable_unprepare(i2c->clk);
-	iounmap(i2c->reg_base);
-eremap:
-	clk_put(i2c->clk);
-eclk:
-	kfree(i2c);
-emalloc:
-	release_mem_region(res->start, resource_size(res));
-	return ret;
 }
 
 static int i2c_pxa_remove(struct platform_device *dev)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2745 bytes --]

Hi,

>  	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>  	irq = platform_get_irq(dev, 0);
> -	if (res == NULL || irq < 0) {
> -		ret = -ENODEV;
> -		goto eclk;
> -	}
> +	if (res == NULL || irq < 0)
> +		return -ENODEV;

No need to check for valid 'res' here...

>  
> -	if (!request_mem_region(res->start, resource_size(res), res->name)) {
> -		ret = -ENOMEM;
> -		goto eclk;
> -	}
> +	if (!devm_request_mem_region(&dev->dev, res->start,
> +				resource_size(res), res->name))
> +		return -ENOMEM;

... and no need to use 'devm_request_mem_region' ...

> -	i2c->reg_base = ioremap(res->start, resource_size(res));
> -	if (!i2c->reg_base) {
> -		ret = -EIO;
> -		goto eremap;
> -	}
> +	i2c->reg_base = devm_ioremap_resource(&adev->dev, res));
> +	if (IS_ERR(i2c->reg_base))
> +		return -EIO;

... because devm_ioremap_resource does all that for you. Also, don't
return -EIO but the PTR_ERR given from the function. Just check other
drivers how they do it.

>  
>  	i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
>  	i2c->reg_idbr = i2c->reg_base + pxa_reg_layout[i2c_type].idbr;
> @@ -1227,10 +1218,12 @@ static int i2c_pxa_probe(struct platform_device *dev)
>  		i2c->adap.algo = &i2c_pxa_pio_algorithm;
>  	} else {
>  		i2c->adap.algo = &i2c_pxa_algorithm;
> -		ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
> -				  dev_name(&dev->dev), i2c);
> -		if (ret)
> -			goto ereqirq;
> +		ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler,
> +				 IRQF_SHARED, dev_name(&dev->dev), i2c);
> +		if (ret) {
> +			clk_disable_unprepare(i2c->clk);
> +			return ret;

Maybe you should make a error path with 'goto' out of it...


> +		}
>  	}
>  
>  	i2c_pxa_reset(i2c);
> @@ -1244,7 +1237,7 @@ static int i2c_pxa_probe(struct platform_device *dev)
>  	ret = i2c_add_numbered_adapter(&i2c->adap);
>  	if (ret < 0) {
>  		printk(KERN_INFO "I2C: Failed to add bus\n");
> -		goto eadapt;
> +		return ret;

... since you forgot disable_unprepare here and could simply reuse it.


>  	}
>  
>  	platform_set_drvdata(dev, i2c);
> @@ -1257,20 +1250,6 @@ static int i2c_pxa_probe(struct platform_device *dev)
>  	       dev_name(&i2c->adap.dev));
>  #endif
>  	return 0;
> -
> -eadapt:
> -	if (!i2c->use_pio)
> -		free_irq(irq, i2c);
> -ereqirq:
> -	clk_disable_unprepare(i2c->clk);
> -	iounmap(i2c->reg_base);
> -eremap:
> -	clk_put(i2c->clk);
> -eclk:
> -	kfree(i2c);
> -emalloc:
> -	release_mem_region(res->start, resource_size(res));
> -	return ret;
>  }

You forgot to cleanup the remove function.

BTW I forgot, can you test this patch on real hardware? If not, is there
somebody who does?

All the best,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4] i2c: busses: i2c-pxa.c: Fix for possible null pointer dereference

[Rickard Strandqvist]

2014-08-02 13:30 GMT+02:00 Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>:
> Hi,
>
>>       res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>>       irq = platform_get_irq(dev, 0);
>> -     if (res == NULL || irq < 0) {
>> -             ret = -ENODEV;
>> -             goto eclk;
>> -     }
>> +     if (res == NULL || irq < 0)
>> +             return -ENODEV;
>
> No need to check for valid 'res' here...
>
>>
>> -     if (!request_mem_region(res->start, resource_size(res), res->name)) {
>> -             ret = -ENOMEM;
>> -             goto eclk;
>> -     }
>> +     if (!devm_request_mem_region(&dev->dev, res->start,
>> +                             resource_size(res), res->name))
>> +             return -ENOMEM;
>
> ... and no need to use 'devm_request_mem_region' ...
>
>> -     i2c->reg_base = ioremap(res->start, resource_size(res));
>> -     if (!i2c->reg_base) {
>> -             ret = -EIO;
>> -             goto eremap;
>> -     }
>> +     i2c->reg_base = devm_ioremap_resource(&adev->dev, res));
>> +     if (IS_ERR(i2c->reg_base))
>> +             return -EIO;
>
> ... because devm_ioremap_resource does all that for you. Also, don't
> return -EIO but the PTR_ERR given from the function. Just check other
> drivers how they do it.
>
>>
>>       i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
>>       i2c->reg_idbr = i2c->reg_base + pxa_reg_layout[i2c_type].idbr;
>> @@ -1227,10 +1218,12 @@ static int i2c_pxa_probe(struct platform_device *dev)
>>               i2c->adap.algo = &i2c_pxa_pio_algorithm;
>>       } else {
>>               i2c->adap.algo = &i2c_pxa_algorithm;
>> -             ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
>> -                               dev_name(&dev->dev), i2c);
>> -             if (ret)
>> -                     goto ereqirq;
>> +             ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler,
>> +                              IRQF_SHARED, dev_name(&dev->dev), i2c);
>> +             if (ret) {
>> +                     clk_disable_unprepare(i2c->clk);
>> +                     return ret;
>
> Maybe you should make a error path with 'goto' out of it...
>
>
>> +             }
>>       }
>>
>>       i2c_pxa_reset(i2c);
>> @@ -1244,7 +1237,7 @@ static int i2c_pxa_probe(struct platform_device *dev)
>>       ret = i2c_add_numbered_adapter(&i2c->adap);
>>       if (ret < 0) {
>>               printk(KERN_INFO "I2C: Failed to add bus\n");
>> -             goto eadapt;
>> +             return ret;
>
> ... since you forgot disable_unprepare here and could simply reuse it.
>
>
>>       }
>>
>>       platform_set_drvdata(dev, i2c);
>> @@ -1257,20 +1250,6 @@ static int i2c_pxa_probe(struct platform_device *dev)
>>              dev_name(&i2c->adap.dev));
>>  #endif
>>       return 0;
>> -
>> -eadapt:
>> -     if (!i2c->use_pio)
>> -             free_irq(irq, i2c);
>> -ereqirq:
>> -     clk_disable_unprepare(i2c->clk);
>> -     iounmap(i2c->reg_base);
>> -eremap:
>> -     clk_put(i2c->clk);
>> -eclk:
>> -     kfree(i2c);
>> -emalloc:
>> -     release_mem_region(res->start, resource_size(res));
>> -     return ret;
>>  }
>
> You forgot to cleanup the remove function.
>
> BTW I forgot, can you test this patch on real hardware? If not, is there
> somebody who does?
>
> All the best,
>
>    Wolfram



Hi

I have made a lot of changes on changes to changes, and have really
tried to ensure that all would be right.

But I feel like that's enough, Wolfram or anyone else may end this.

And no as I replied earlier, I lack this kind of hardware...


Kind regards
Rickard Strandqvist