| Message ID | 20100307205017.GB25405@pengutronix.de |
|---|---|
| State | Accepted |
| Headers | show |
On Sun, Mar 07, 2010 at 09:50:17PM +0100, Uwe Kleine-König wrote: > Hello, > > On Sun, Mar 07, 2010 at 05:14:59PM +0300, Dan Carpenter wrote: > > 4c014e872e0 rtc/mc13783: protect rtc {,un}registration by mc13783 lock > > > > This introduces a use after free bug for "priv". > > Dan: thanks for pointing that out. Do you use a tool to find these issues? > Yeah. I'm using smatch. http://smatch.sf.net It's easy to use, but false positive heavy. regards, dan carpenter > Andrew: this affects > rtc-mc13783-protect-rtc-unregistration-by-mc13783-lock.patch > in mmotm. > > Below is an incremental fix for that, Andrew, can you please squash it > into the patch above? rtc-mc13783-implement-alarm.patch conflicts with > it. (The hunks for mc13783_rtc_probe using "priv->mc13783" should now > use "mc13783" only and the offsets are different now.) If it's easier > for you, I can resend an updated version. Just tell me so. > > Best regards > Uwe > > diff --git a/drivers/rtc/rtc-mc13783.c b/drivers/rtc/rtc-mc13783.c > index 6a36201..f6d1fad 100644 > --- a/drivers/rtc/rtc-mc13783.c > +++ b/drivers/rtc/rtc-mc13783.c > @@ -169,30 +169,33 @@ static int __devinit mc13783_rtc_probe(struct platform_device *pdev) > { > int ret; > struct mc13783_rtc *priv; > + struct mc13783 *mc13783; > int rtcrst_pending; > > priv = kzalloc(sizeof(*priv), GFP_KERNEL); > if (!priv) > return -ENOMEM; > > - priv->mc13783 = dev_get_drvdata(pdev->dev.parent); > + mc13783 = dev_get_drvdata(pdev->dev.parent); > + priv->mc13783 = mc13783; > + > platform_set_drvdata(pdev, priv); > > - mc13783_lock(priv->mc13783); > + mc13783_lock(mc13783); > > - ret = mc13783_irq_request(priv->mc13783, MC13783_IRQ_RTCRST, > + ret = mc13783_irq_request(mc13783, MC13783_IRQ_RTCRST, > mc13783_rtc_reset_handler, DRIVER_NAME, priv); > if (ret) > goto err_reset_irq_request; > > - ret = mc13783_irq_status(priv->mc13783, MC13783_IRQ_RTCRST, > + ret = mc13783_irq_status(mc13783, MC13783_IRQ_RTCRST, > NULL, &rtcrst_pending); > if (ret) > goto err_reset_irq_status; > > priv->valid = !rtcrst_pending; > > - ret = mc13783_irq_request_nounmask(priv->mc13783, MC13783_IRQ_1HZ, > + ret = mc13783_irq_request_nounmask(mc13783, MC13783_IRQ_1HZ, > mc13783_rtc_update_handler, DRIVER_NAME, priv); > if (ret) > goto err_update_irq_request; > @@ -202,19 +205,19 @@ static int __devinit mc13783_rtc_probe(struct platform_device *pdev) > if (IS_ERR(priv->rtc)) { > ret = PTR_ERR(priv->rtc); > > - mc13783_irq_free(priv->mc13783, MC13783_IRQ_1HZ, priv); > + mc13783_irq_free(mc13783, MC13783_IRQ_1HZ, priv); > err_update_irq_request: > > err_reset_irq_status: > > - mc13783_irq_free(priv->mc13783, MC13783_IRQ_RTCRST, priv); > + mc13783_irq_free(mc13783, MC13783_IRQ_RTCRST, priv); > err_reset_irq_request: > > platform_set_drvdata(pdev, NULL); > kfree(priv); > } > > - mc13783_unlock(priv->mc13783); > + mc13783_unlock(mc13783); > > return ret; > } > > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
On Mon, Mar 08, 2010 at 02:59:44PM +0300, Dan Carpenter wrote: > On Sun, Mar 07, 2010 at 09:50:17PM +0100, Uwe Kleine-König wrote: > > Hello, > > > > On Sun, Mar 07, 2010 at 05:14:59PM +0300, Dan Carpenter wrote: > > > 4c014e872e0 rtc/mc13783: protect rtc {,un}registration by mc13783 lock > > > > > > This introduces a use after free bug for "priv". > > > > Dan: thanks for pointing that out. Do you use a tool to find these issues? > > > > Yeah. I'm using smatch. http://smatch.sf.net It's easy to use, but false > positive heavy. I already got a hint you're using that by private mail. s/janitor.kernelnewbies.net/janitor.kernelnewbies.org/ on http://smatch.sf.net BTW. Best regards Uwe
diff --git a/drivers/rtc/rtc-mc13783.c b/drivers/rtc/rtc-mc13783.c index 6a36201..f6d1fad 100644 --- a/drivers/rtc/rtc-mc13783.c +++ b/drivers/rtc/rtc-mc13783.c @@ -169,30 +169,33 @@ static int __devinit mc13783_rtc_probe(struct platform_device *pdev) { int ret; struct mc13783_rtc *priv; + struct mc13783 *mc13783; int rtcrst_pending; priv = kzalloc(sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; - priv->mc13783 = dev_get_drvdata(pdev->dev.parent); + mc13783 = dev_get_drvdata(pdev->dev.parent); + priv->mc13783 = mc13783; + platform_set_drvdata(pdev, priv); - mc13783_lock(priv->mc13783); + mc13783_lock(mc13783); - ret = mc13783_irq_request(priv->mc13783, MC13783_IRQ_RTCRST, + ret = mc13783_irq_request(mc13783, MC13783_IRQ_RTCRST, mc13783_rtc_reset_handler, DRIVER_NAME, priv); if (ret) goto err_reset_irq_request; - ret = mc13783_irq_status(priv->mc13783, MC13783_IRQ_RTCRST, + ret = mc13783_irq_status(mc13783, MC13783_IRQ_RTCRST, NULL, &rtcrst_pending); if (ret) goto err_reset_irq_status; priv->valid = !rtcrst_pending; - ret = mc13783_irq_request_nounmask(priv->mc13783, MC13783_IRQ_1HZ, + ret = mc13783_irq_request_nounmask(mc13783, MC13783_IRQ_1HZ, mc13783_rtc_update_handler, DRIVER_NAME, priv); if (ret) goto err_update_irq_request; @@ -202,19 +205,19 @@ static int __devinit mc13783_rtc_probe(struct platform_device *pdev) if (IS_ERR(priv->rtc)) { ret = PTR_ERR(priv->rtc); - mc13783_irq_free(priv->mc13783, MC13783_IRQ_1HZ, priv); + mc13783_irq_free(mc13783, MC13783_IRQ_1HZ, priv); err_update_irq_request: err_reset_irq_status: - mc13783_irq_free(priv->mc13783, MC13783_IRQ_RTCRST, priv); + mc13783_irq_free(mc13783, MC13783_IRQ_RTCRST, priv); err_reset_irq_request: platform_set_drvdata(pdev, NULL); kfree(priv); } - mc13783_unlock(priv->mc13783); + mc13783_unlock(mc13783); return ret; }