Patchwork [1/3] arch/powerpc: Add kmalloc NULL tests

login
register
mail settings
Submitter Julia Lawall
Date Aug. 6, 2009, 8:04 p.m.
Message ID <Pine.LNX.4.64.0908062203380.19100@ask.diku.dk>
Download mbox | patch
Permalink /patch/30877/
State Accepted, archived
Delegated to: Kumar Gala
Headers show

Comments

Julia Lawall - Aug. 6, 2009, 8:04 p.m.
From: Julia Lawall <julia@diku.dk>

Check that the result of kmalloc/kzalloc is not NULL before dereferencing it.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression *x;
identifier f;
constant char *C;
@@

x = \(kmalloc\|kcalloc\|kzalloc\)(...);
... when != x == NULL
    when != x != NULL
    when != (x || ...)
(
kfree(x)
|
f(...,C,...,x,...)
|
*f(...,x,...)
|
*x->f
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 arch/powerpc/sysdev/fsl_rio.c       |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Kumar Gala - Aug. 7, 2009, 2:26 a.m.
On Aug 6, 2009, at 3:04 PM, Julia Lawall wrote:

> From: Julia Lawall <julia@diku.dk>
>
> Check that the result of kmalloc/kzalloc is not NULL before  
> dereferencing it.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression *x;
> identifier f;
> constant char *C;
> @@
>
> x = \(kmalloc\|kcalloc\|kzalloc\)(...);
> ... when != x == NULL
>    when != x != NULL
>    when != (x || ...)
> (
> kfree(x)
> |
> f(...,C,...,x,...)
> |
> *f(...,x,...)
> |
> *x->f
> )
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
>
> ---
> arch/powerpc/sysdev/fsl_rio.c       |   18 ++++++++++++++----
> 1 files changed, 14 insertions(+), 4 deletions(-)


applied to next

- k
Daniel K. - Aug. 7, 2009, 6:34 a.m.
Julia Lawall wrote:
> --- a/arch/powerpc/sysdev/fsl_rio.c
> +++ b/arch/powerpc/sysdev/fsl_rio.c
> @@ -1057,6 +1057,10 @@ int fsl_rio_setup(struct of_device *dev)
>  			law_start, law_size);
>  
>  	ops = kmalloc(sizeof(struct rio_ops), GFP_KERNEL);
> +	if (!ops) {
> +		rc = -ENOMEM;
> +		goto err_ops;
> +	}
>  	ops->lcread = fsl_local_config_read;
>  	ops->lcwrite = fsl_local_config_write;
>  	ops->cread = fsl_rio_config_read;
> @@ -1064,6 +1068,10 @@ int fsl_rio_setup(struct of_device *dev)
>  	ops->dsend = fsl_rio_doorbell_send;
>  
>  	port = kzalloc(sizeof(struct rio_mport), GFP_KERNEL);
> +	if (!port) {
> +		rc = -ENOMEM;
> +		goto err_port;
> +	}
>  	port->id = 0;
>  	port->index = 0;
>  
> @@ -1071,7 +1079,7 @@ int fsl_rio_setup(struct of_device *dev)
>  	if (!priv) {
>  		printk(KERN_ERR "Can't alloc memory for 'priv'\n");
>  		rc = -ENOMEM;
> -		goto err;
> +		goto err_priv;
>  	}
>  
>  	INIT_LIST_HEAD(&port->dbells);
> @@ -1169,13 +1177,15 @@ int fsl_rio_setup(struct of_device *dev)
>  
>  	return 0;
>  err:
> -	if (priv)
> -		iounmap(priv->regs_win);
> -	kfree(ops);
> +	iounmap(priv->regs_win);
> +err_priv:
>  	kfree(priv);
> +err_port:
>  	kfree(port);
> +err_ops:
> +	kfree(ops);
>  	return rc;

There seems to be a goto-off-by-one error here.

If xxxx = kxalloc() fails, you goto err_xxxx, and do a kfree(xxxx) where xxxx is
already proven to be NULL.

Is there a reason for this that eludes me?


I'd expect that last hunk to look something like

@@ -1169,13 +1177,15 @@ int fsl_rio_setup(struct of_device *dev)
 
 	return 0;
 err:
-	if (priv)
-		iounmap(priv->regs_win);
-	kfree(ops);
+	iounmap(priv->regs_win);
 	kfree(priv);
+err_priv:
 	kfree(port);
+err_port:
+	kfree(ops);
+err_ops:
 	return rc;
 }


Daniel K.
Julia Lawall - Aug. 7, 2009, 6:51 a.m.
On Fri, 7 Aug 2009, Daniel K. wrote:

> Julia Lawall wrote:
> > --- a/arch/powerpc/sysdev/fsl_rio.c
> > +++ b/arch/powerpc/sysdev/fsl_rio.c
> > @@ -1057,6 +1057,10 @@ int fsl_rio_setup(struct of_device *dev)
> >     law_start, law_size);
> >  
> > 	ops = kmalloc(sizeof(struct rio_ops), GFP_KERNEL);
> > +	if (!ops) {
> > +		rc = -ENOMEM;
> > +		goto err_ops;
> > +	}
> >   ops->lcread = fsl_local_config_read;
> >   ops->lcwrite = fsl_local_config_write;
> >   ops->cread = fsl_rio_config_read;
> > @@ -1064,6 +1068,10 @@ int fsl_rio_setup(struct of_device *dev)
> >   ops->dsend = fsl_rio_doorbell_send;
> >  
> > 	port = kzalloc(sizeof(struct rio_mport), GFP_KERNEL);
> > +	if (!port) {
> > +		rc = -ENOMEM;
> > +		goto err_port;
> > +	}
> >   port->id = 0;
> >   port->index = 0;
> >  
> > @@ -1071,7 +1079,7 @@ int fsl_rio_setup(struct of_device *dev)
> >   if (!priv) {
> >    printk(KERN_ERR "Can't alloc memory for 'priv'\n");
> >    rc = -ENOMEM;
> > -		goto err;
> > +		goto err_priv;
> >   }
> >  
> >   INIT_LIST_HEAD(&port->dbells);
> > @@ -1169,13 +1177,15 @@ int fsl_rio_setup(struct of_device *dev)
> >  
> >  	return 0;
> > err:
> > -	if (priv)
> > -		iounmap(priv->regs_win);
> > -	kfree(ops);
> > +	iounmap(priv->regs_win);
> > +err_priv:
> > 	kfree(priv);
> > +err_port:
> > 	kfree(port);
> > +err_ops:
> > +	kfree(ops);
> >   return rc;
> 
> There seems to be a goto-off-by-one error here.
> 
> If xxxx = kxalloc() fails, you goto err_xxxx, and do a kfree(xxxx) where xxxx
> is
> already proven to be NULL.
> 
> Is there a reason for this that eludes me?

No, I messed up...  I will fix it.

julia


> I'd expect that last hunk to look something like
> 
> @@ -1169,13 +1177,15 @@ int fsl_rio_setup(struct of_device *dev)
> 
> 	return 0;
> err:
> -	if (priv)
> -		iounmap(priv->regs_win);
> -	kfree(ops);
> +	iounmap(priv->regs_win);
> 	kfree(priv);
> +err_priv:
> 	kfree(port);
> +err_port:
> +	kfree(ops);
> +err_ops:
> 	return rc;
> }
> 
> 
> Daniel K.
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Patch

diff --git a/arch/powerpc/sysdev/fsl_rio.c b/arch/powerpc/sysdev/fsl_rio.c
index cbb3bed..598789c 100644
--- a/arch/powerpc/sysdev/fsl_rio.c
+++ b/arch/powerpc/sysdev/fsl_rio.c
@@ -1057,6 +1057,10 @@  int fsl_rio_setup(struct of_device *dev)
 			law_start, law_size);
 
 	ops = kmalloc(sizeof(struct rio_ops), GFP_KERNEL);
+	if (!ops) {
+		rc = -ENOMEM;
+		goto err_ops;
+	}
 	ops->lcread = fsl_local_config_read;
 	ops->lcwrite = fsl_local_config_write;
 	ops->cread = fsl_rio_config_read;
@@ -1064,6 +1068,10 @@  int fsl_rio_setup(struct of_device *dev)
 	ops->dsend = fsl_rio_doorbell_send;
 
 	port = kzalloc(sizeof(struct rio_mport), GFP_KERNEL);
+	if (!port) {
+		rc = -ENOMEM;
+		goto err_port;
+	}
 	port->id = 0;
 	port->index = 0;
 
@@ -1071,7 +1079,7 @@  int fsl_rio_setup(struct of_device *dev)
 	if (!priv) {
 		printk(KERN_ERR "Can't alloc memory for 'priv'\n");
 		rc = -ENOMEM;
-		goto err;
+		goto err_priv;
 	}
 
 	INIT_LIST_HEAD(&port->dbells);
@@ -1169,13 +1177,15 @@  int fsl_rio_setup(struct of_device *dev)
 
 	return 0;
 err:
-	if (priv)
-		iounmap(priv->regs_win);
-	kfree(ops);
+	iounmap(priv->regs_win);
+err_priv:
 	kfree(priv);
+err_port:
 	kfree(port);
+err_ops:
+	kfree(ops);
 	return rc;
 }
 
_______________________________________________
Linuxppc-dev mailing list