Re: [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c:reposition free_irq to avoid access to invalid data

[Guennadi Liakhovetski]

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@xxxxxxx>
> > >
> > > 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@xxxxxxx>
> > >
> > > ---
> > > 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@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/