Patchwork i2c: busses: i2c-pxa.c: Fix for possible null pointer dereferenc

login
register
mail settings
Submitter Rickard Strandqvist
Date June 3, 2014, 9:30 p.m.
Message ID <1401831034-30255-1-git-send-email-rickard_strandqvist@spectrumdigital.se>
Download mbox | patch
Permalink /patch/355659/
State Changes Requested
Headers show

Comments

Rickard Strandqvist - June 3, 2014, 9:30 p.m.
Fix for possible null pointer dereferenc, and there is a risk
for memory leak in when something unexpected happens and the
function returns.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/i2c/busses/i2c-pxa.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)
Wolfram Sang - June 27, 2014, 12:21 p.m.
On Tue, Jun 03, 2014 at 11:30:34PM +0200, Rickard Strandqvist wrote:
> Fix for possible null pointer dereferenc, and there is a risk
> for memory leak in when something unexpected happens and the
> function returns.
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  drivers/i2c/busses/i2c-pxa.c |   19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index bbe6dfb..948a3c7 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -1142,10 +1142,8 @@ static int i2c_pxa_probe(struct platform_device *dev)
>  	int ret, irq;
>  
>  	i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
> -	if (!i2c) {
> -		ret = -ENOMEM;
> -		goto emalloc;
> -	}
> +	if (!i2c)
> +		return -ENOMEM;
>  
>  	/* Default adapter num to device id; i2c_pxa_probe_dt can override. */
>  	i2c->adap.nr = dev->id;
> @@ -1154,11 +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;
> +		goto emalloc;
>  
>  	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> +	if (res == NULL) {
> +		ret = -ENODEV;
> +		goto emalloc;
> +	}
> +
>  	irq = platform_get_irq(dev, 0);
> -	if (res == NULL || irq < 0) {
> +	if (irq < 0) {
>  		ret = -ENODEV;
>  		goto eclk;

Nope, those should be emalloc as well since the region was not requested
here. May I ask you to just convert this driver to managed devices (devm_*
functions)? They are made to exactly avoid this hazzle and cleanup the
driver.

Thanks,

   Wolfram
Wolfram Sang - June 29, 2014, 10:47 a.m.
Hi,

please *always* reply to the list.

> So how do you want me to change it, or maybe it's not even meant for
> me to do that?

If you want, I'd be happy if you do it. Tests on real hardware would be
needed, though. Do you have one? Maybe somebody here can help you?

An example is commit 9b2b98a3b4de. More details, including a list of
available functions, can be found here: Documentation/driver-model/devres.txt

Good luck!

   Wolfram

Patch

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index bbe6dfb..948a3c7 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1142,10 +1142,8 @@  static int i2c_pxa_probe(struct platform_device *dev)
 	int ret, irq;
 
 	i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
-	if (!i2c) {
-		ret = -ENOMEM;
-		goto emalloc;
-	}
+	if (!i2c)
+		return -ENOMEM;
 
 	/* Default adapter num to device id; i2c_pxa_probe_dt can override. */
 	i2c->adap.nr = dev->id;
@@ -1154,11 +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;
+		goto emalloc;
 
 	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (res == NULL) {
+		ret = -ENODEV;
+		goto emalloc;
+	}
+
 	irq = platform_get_irq(dev, 0);
-	if (res == NULL || irq < 0) {
+	if (irq < 0) {
 		ret = -ENODEV;
 		goto eclk;
 	}
@@ -1267,9 +1270,9 @@  ereqirq:
 eremap:
 	clk_put(i2c->clk);
 eclk:
-	kfree(i2c);
-emalloc:
 	release_mem_region(res->start, resource_size(res));
+emalloc:
+	kfree(i2c);
 	return ret;
 }