[PATCH v3] ARM: imx: fix error handling in ipu device registration

[PATCH v3] ARM: imx: fix error handling in ipu device registration

[Emil Goode]

If we fail to allocate struct platform_device pdev we
dereference it after the goto label err.

I have rearranged the error handling a bit to fix the issue
and also make it more clear.

Signed-off-by: Emil Goode <emilgoode@gmail.com>
---
v3: Made subject line more specific.
v2: Changed to return -ENOMEM instead of ret where possible and
    updated the subject line.

 arch/arm/mach-imx/devices/platform-ipu-core.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c
index fc4dd7c..68f2a4a 100644
--- a/arch/arm/mach-imx/devices/platform-ipu-core.c
+++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
@@ -77,34 +77,38 @@ struct platform_device *__init imx_alloc_mx3_camera(
 
 	pdev = platform_device_alloc("mx3-camera", 0);
 	if (!pdev)
-		goto err;
+		return ERR_PTR(-ENOMEM);
 
 	pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
 	if (!pdev->dev.dma_mask)
-		goto err;
+		goto put_pdev;
 
 	*pdev->dev.dma_mask = DMA_BIT_MASK(32);
 	pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 
 	ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
 	if (ret)
-		goto err;
+		goto free_dma_mask;
 
 	if (pdata) {
 		struct mx3_camera_pdata *copied_pdata;
 
 		ret = platform_device_add_data(pdev, pdata, sizeof(*pdata));
-		if (ret) {
-err:
-			kfree(pdev->dev.dma_mask);
-			platform_device_put(pdev);
-			return ERR_PTR(-ENODEV);
-		}
+		if (ret)
+			goto free_dma_mask;
+
 		copied_pdata = dev_get_platdata(&pdev->dev);
 		copied_pdata->dma_dev = &imx_ipu_coredev->dev;
 	}
 
 	return pdev;
+
+free_dma_mask:
+	kfree(pdev->dev.dma_mask);
+put_pdev:
+	platform_device_put(pdev);
+
+	return ERR_PTR(ret);
 }
 
 struct platform_device *__init imx_add_mx3_sdc_fb(
-- 
1.7.10.4

[PATCH v3] ARM: imx: fix error handling in ipu device registration

[Uwe Kleine-König]

Hello Emil,

On Sat, May 17, 2014 at 08:40:33PM +0200, Emil Goode wrote:
> If we fail to allocate struct platform_device pdev we
> dereference it after the goto label err.
> 
> I have rearranged the error handling a bit to fix the issue
> and also make it more clear.
> 
> Signed-off-by: Emil Goode <emilgoode@gmail.com>
> ---
> v3: Made subject line more specific.
> v2: Changed to return -ENOMEM instead of ret where possible and
>     updated the subject line.
> 
>  arch/arm/mach-imx/devices/platform-ipu-core.c |   22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
Considering that you can fix the issue also by just doing:

diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c
index fc4dd7cedc11..6bd7c3f37ac0 100644
--- a/arch/arm/mach-imx/devices/platform-ipu-core.c
+++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
@@ -77,7 +77,7 @@ struct platform_device *__init imx_alloc_mx3_camera(
 
 	pdev = platform_device_alloc("mx3-camera", 0);
 	if (!pdev)
-		goto err;
+		return ERR_PTR(-ENOMEM);
 
 	pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
 	if (!pdev->dev.dma_mask)

or

diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c
index fc4dd7cedc11..c609f3667200 100644
--- a/arch/arm/mach-imx/devices/platform-ipu-core.c
+++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
@@ -96,7 +96,8 @@ struct platform_device *__init imx_alloc_mx3_camera(
 		ret = platform_device_add_data(pdev, pdata, sizeof(*pdata));
 		if (ret) {
 err:
-			kfree(pdev->dev.dma_mask);
+			if (pdev)
+				kfree(pdev->dev.dma_mask);
 			platform_device_put(pdev);
 			return ERR_PTR(-ENODEV);
 		}

I would prefer one of them as it is easier to justify and for the next
cycle convert the function to platform_device_register_full.

Also you should point out in the commit log that the issue was found by
coccinelle.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v3] ARM: imx: fix error handling in ipu device registration

[Dan Carpenter]

On Sat, May 17, 2014 at 09:18:21PM +0200, Uwe Kleine-K?nig wrote:
> diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c
> index fc4dd7cedc11..6bd7c3f37ac0 100644
> --- a/arch/arm/mach-imx/devices/platform-ipu-core.c
> +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
> @@ -77,7 +77,7 @@ struct platform_device *__init imx_alloc_mx3_camera(
>  
>  	pdev = platform_device_alloc("mx3-camera", 0);
>  	if (!pdev)
> -		goto err;
> +		return ERR_PTR(-ENOMEM);
>  
>  	pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
>  	if (!pdev->dev.dma_mask)

Emil, do this one, please and not the second suggestion.

Direct returns are more readable.  Otherwise, you wonder what the goto
is for and where it will take you and be annoyed to discover it is a
waste of time, no-op goto.  Also you will wonder if platform_device_put()
accepts NULL pointers.  Thirdly there is a small ugliness that the error
code is not preserved.  What is the point of setting the error code to
-ENOMEM only to discard it?

Let's look at that error handling again.

err:  <-- the name is not descriptive.  the location is bad.
		kfree(pdev->dev.dma_mask);  <- null dereference.
		platform_device_put(pdev);  <- ok
		return ERR_PTR(-ENODEV);    <- should be "return ERR_PTR(ret);"

3 out of 4 of the lines are bad.

regards,
dan carpenter

[PATCH v3] ARM: imx: fix error handling in ipu device registration

[Emil Goode]

Hello Uwe,

I was to quick to resend the patch, sorry.

On Sat, May 17, 2014 at 09:18:21PM +0200, Uwe Kleine-K?nig wrote:
> Hello Emil,
> 
> On Sat, May 17, 2014 at 08:40:33PM +0200, Emil Goode wrote:
> > If we fail to allocate struct platform_device pdev we
> > dereference it after the goto label err.
> > 
> > I have rearranged the error handling a bit to fix the issue
> > and also make it more clear.
> > 
> > Signed-off-by: Emil Goode <emilgoode@gmail.com>
> > ---
> > v3: Made subject line more specific.
> > v2: Changed to return -ENOMEM instead of ret where possible and
> >     updated the subject line.
> > 
> >  arch/arm/mach-imx/devices/platform-ipu-core.c |   22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> Considering that you can fix the issue also by just doing:
> 
> diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c
> index fc4dd7cedc11..6bd7c3f37ac0 100644
> --- a/arch/arm/mach-imx/devices/platform-ipu-core.c
> +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
> @@ -77,7 +77,7 @@ struct platform_device *__init imx_alloc_mx3_camera(
>  
>  	pdev = platform_device_alloc("mx3-camera", 0);
>  	if (!pdev)
> -		goto err;
> +		return ERR_PTR(-ENOMEM);
>  
>  	pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
>  	if (!pdev->dev.dma_mask)
> 
> or
> 
> diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c
> index fc4dd7cedc11..c609f3667200 100644
> --- a/arch/arm/mach-imx/devices/platform-ipu-core.c
> +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
> @@ -96,7 +96,8 @@ struct platform_device *__init imx_alloc_mx3_camera(
>  		ret = platform_device_add_data(pdev, pdata, sizeof(*pdata));
>  		if (ret) {
>  err:
> -			kfree(pdev->dev.dma_mask);
> +			if (pdev)
> +				kfree(pdev->dev.dma_mask);
>  			platform_device_put(pdev);
>  			return ERR_PTR(-ENODEV);
>  		}
> 
> I would prefer one of them as it is easier to justify and for the next
> cycle convert the function to platform_device_register_full.

Agreed, that makes sense considering the second patch that would convert to
platform_device_register_full().

> 
> Also you should point out in the commit log that the issue was found by
> coccinelle.

Ok, will do that.

Thank you!

Best regards,

Emil Goode

[PATCH v3] ARM: imx: fix error handling in ipu device registration

[Russell King - ARM Linux]

On Sun, May 18, 2014 at 01:08:36AM +0300, Dan Carpenter wrote:
> Let's look at that error handling again.
> 
> err:  <-- the name is not descriptive.  the location is bad.
> 		kfree(pdev->dev.dma_mask);  <- null dereference.
> 		platform_device_put(pdev);  <- ok
> 		return ERR_PTR(-ENODEV);    <- should be "return ERR_PTR(ret);"
> 
> 3 out of 4 of the lines are bad.

2 out of 4.  kfree(NULL) is perfectly legal.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH v3] ARM: imx: fix error handling in ipu device registration

[Emil Goode]

Hello Dan,

On Sun, May 18, 2014 at 01:08:36AM +0300, Dan Carpenter wrote:
> On Sat, May 17, 2014 at 09:18:21PM +0200, Uwe Kleine-K?nig wrote:
> > diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c
> > index fc4dd7cedc11..6bd7c3f37ac0 100644
> > --- a/arch/arm/mach-imx/devices/platform-ipu-core.c
> > +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c
> > @@ -77,7 +77,7 @@ struct platform_device *__init imx_alloc_mx3_camera(
> >  
> >  	pdev = platform_device_alloc("mx3-camera", 0);
> >  	if (!pdev)
> > -		goto err;
> > +		return ERR_PTR(-ENOMEM);
> >  
> >  	pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
> >  	if (!pdev->dev.dma_mask)
> 
> Emil, do this one, please and not the second suggestion.

Yes, I would pick this one to.

> 
> Direct returns are more readable.  Otherwise, you wonder what the goto
> is for and where it will take you and be annoyed to discover it is a
> waste of time, no-op goto.  Also you will wonder if platform_device_put()
> accepts NULL pointers.  Thirdly there is a small ugliness that the error
> code is not preserved.  What is the point of setting the error code to
> -ENOMEM only to discard it?
> 
> Let's look at that error handling again.
> 
> err:  <-- the name is not descriptive.  the location is bad.
> 		kfree(pdev->dev.dma_mask);  <- null dereference.
> 		platform_device_put(pdev);  <- ok
> 		return ERR_PTR(-ENODEV);    <- should be "return ERR_PTR(ret);"
> 
> 3 out of 4 of the lines are bad.

I agree that it's not very pretty.

Now that Uwe solved the issue regarding converting the function to
platform_device_register_full(), I will look into sending a second patch
that would remove these lines.

Thank you!

Best regards,

Emil Goode

[PATCH v3] ARM: imx: fix error handling in ipu device registration

[Dan Carpenter]

On Sat, May 17, 2014 at 11:35:55PM +0100, Russell King - ARM Linux wrote:
> On Sun, May 18, 2014 at 01:08:36AM +0300, Dan Carpenter wrote:
> > Let's look at that error handling again.
> > 
> > err:  <-- the name is not descriptive.  the location is bad.
> > 		kfree(pdev->dev.dma_mask);  <- null dereference.
> > 		platform_device_put(pdev);  <- ok
> > 		return ERR_PTR(-ENODEV);    <- should be "return ERR_PTR(ret);"
> > 
> > 3 out of 4 of the lines are bad.
> 
> 2 out of 4.  kfree(NULL) is perfectly legal.

pdev was NULL though...

The bug is *caused* by trying to use the same "err" label to do all
error handling.  This is a very common anti-patern, but if you follow
canonical kernel style then your error handling is less buggy.

regards,
dan carpenter

[PATCH v3] ARM: imx: fix error handling in ipu device registration

[Emil Goode]

Hello Russell,

On Sat, May 17, 2014 at 11:35:55PM +0100, Russell King - ARM Linux wrote:
> On Sun, May 18, 2014 at 01:08:36AM +0300, Dan Carpenter wrote:
> > Let's look at that error handling again.
> > 
> > err:  <-- the name is not descriptive.  the location is bad.
> > 		kfree(pdev->dev.dma_mask);  <- null dereference.
> > 		platform_device_put(pdev);  <- ok
> > 		return ERR_PTR(-ENODEV);    <- should be "return ERR_PTR(ret);"
> > 
> > 3 out of 4 of the lines are bad.
> 
> 2 out of 4.  kfree(NULL) is perfectly legal.

I believe pdev could potentially be NULL, so it's the dereference
that is the problem.

Best regards,

Emil Goode