Message ID | Pine.LNX.4.64.0908062203380.19100@ask.diku.dk (mailing list archive) |
---|---|
State | Accepted, archived |
Delegated to: | Kumar Gala |
Headers | show |
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
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.
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 >
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