linux-kernel - Re: [PATCH 1/2] drivers/media/platform/soc_camera/pxa_camera.c: reposition free

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.02.1301071214150.1908@hadrien>
Date:	Mon, 7 Jan 2013 12:18:05 +0100 (CET)
From:	Julia Lawall <julia.lawall@...6.fr>
To:	Guennadi Liakhovetski <g.liakhovetski@....de>
cc:	Julia Lawall <Julia.Lawall@...6.fr>,
	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, 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.

> 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.

> 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
>
--
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/