| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Pine.LNX.4.64.1301071229070.23972@axis700.grange>
Date: Mon, 7 Jan 2013 12:33:55 +0100 (CET)
From: Guennadi Liakhovetski <g.liakhovetski@....de>
To: Julia Lawall <julia.lawall@...6.fr>
cc: kernel-janitors@...r.kernel.org,
Mauro Carvalho Chehab <mchehab@...hat.com>,
Linux Media Mailing List <linux-media@...r.kernel.org>,
linux-kernel@...r.kernel.org,
Robert Jarzmik <robert.jarzmik@...e.fr>
Subject: Re: [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c:
reposition free_irq to avoid access to invalid data
On Mon, 7 Jan 2013, Julia Lawall wrote:
> On Mon, 7 Jan 2013, Guennadi Liakhovetski wrote:
>
> > (adding Robert to CC)
> >
> > Hi Julia
> >
> > Thanks for the patch.
> >
> > On Mon, 7 Jan 2013, Julia Lawall wrote:
> >
> > > From: Julia Lawall <Julia.Lawall@...6.fr>
> > >
> > > The data referenced by an interrupt handler should not be freed before the
> > > interrupt is ended. The handler is pxa_camera_irq. This handler may call
> > > pxa_dma_start_channels, which references the channels that are freed on the
> > > lines before the call to free_irq.
> >
> > I don't think any data is freed by pxa_free_dma(), it only disables DMA on
> > a certain channel.
>
> OK, I seem to have been thrown off by the clearing fo the name field, but
> that doesn't seem to be very important.
Exactly.
> > Theoretically there could be a different problem:
> > pxa_free_dma() deactivates DMA, whereas pxa_dma_start_channels() activates
> > it. But I think we're also protected against that: by the time
> > pxa_camera_remove() is called, and operation on the interface has been
> > stopped, client devices have been detached, pxa_camera_remove_device() has
> > been called, which has also stopped the interface clock. And with clock
> > stopped no interrupts can be generated. And the case of interrupt having
> > been generated before clk_disabled() and only delivered to the driver so
> > much later, that we're already unloading the module, seems really
> > impossible to me. Robert, you agree?
>
> OK, thanks for the explanation.
>
> > OTOH, it would be nice to convert also this driver to managed allocations,
> > which also would include devm_request(_threaded)_irq(), but that would
> > mean, that free_irq() would be called even later than now, also after
> > pxa_free_dma().
>
> OK, if it is safe to call free_irq much later, then I can propose a patch
> for that.
I think it should be safe. In any case we cannot rely on the fact, that
free_irq() in the current code happens "soon" after pxa_free_dma(), so,
putting it even later will either emphasise our certainty, that we're safe
there, or help up catch the bug, since statistically the window will
become larger;-) Of course, all of clk_get(), request_mem_region() +
ioremap(), kzalloc(), request_irq() would have to be replaced.
Thanks
Guennadi
> > Speaking about managed allocations, those can be dangerous too: if you
> > request an IRQ before, say, remapping memory, or if you only use managed
> > IRQ requesting and ioremap() memory in your driver manually, that would be
> > wrong. But from a quick grep looks like most (all?) drivers get ir right -
> > first ioremap(), then request IRQ, but to be certain maybe coccinelle
> > could run a test for that too;-)
>
> Sure. Thanks for the suggestion!
>
> julia
>
> > Thanks
> > Guennadi
> >
> > > The semantic match that finds this problem is as follows:
> > > (http://coccinelle.lip6.fr/)
> > >
> > > // <smpl>
> > > @fn exists@
> > > expression list es;
> > > expression a,b;
> > > identifier f;
> > > @@
> > >
> > > if (...) {
> > > ... when any
> > > free_irq(a,b);
> > > ... when any
> > > f(es);
> > > ... when any
> > > return ...;
> > > }
> > >
> > > @@
> > > expression list fn.es;
> > > expression fn.a,fn.b;
> > > identifier fn.f;
> > > @@
> > >
> > > *f(es);
> > > ... when any
> > > *free_irq(a,b);
> > > // </smpl>
> > >
> > > Signed-off-by: Julia Lawall <Julia.Lawall@...6.fr>
> > >
> > > ---
> > > Not compiled. I have not observed the problem in practice; the code just
> > > looks suspicious.
> > >
> > > drivers/media/platform/soc_camera/pxa_camera.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c
> > > index f91f7bf..2a19aba 100644
> > > --- a/drivers/media/platform/soc_camera/pxa_camera.c
> > > +++ b/drivers/media/platform/soc_camera/pxa_camera.c
> > > @@ -1810,10 +1810,10 @@ static int pxa_camera_remove(struct platform_device *pdev)
> > >
> > > clk_put(pcdev->clk);
> > >
> > > + free_irq(pcdev->irq, pcdev);
> > > pxa_free_dma(pcdev->dma_chans[0]);
> > > pxa_free_dma(pcdev->dma_chans[1]);
> > > pxa_free_dma(pcdev->dma_chans[2]);
> > > - free_irq(pcdev->irq, pcdev);
> > >
> > > soc_camera_host_unregister(soc_host);
> > >
> > >
> >
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > the body of a message to majordomo@...r.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists