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]

Date:	Mon, 7 Jan 2013 22:09:27 +0100 (CET)
From:	Julia Lawall <julia.lawall@...6.fr>
To:	Robert Jarzmik <robert.jarzmik@...e.fr>
cc:	Guennadi Liakhovetski <g.liakhovetski@....de>,
	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
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, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@....de> writes:
> 
> > (adding Robert to CC)
> > I don't think any data is freed by pxa_free_dma(), it only disables DMA on 
> > a certain channel. 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?
> 
> Agreed that pxa_free_dma() doesn't free anything, that one is easy :)
> 
> And agreed too for the second part, with a slighly different explanation :
>  - pxa_camera_remove_device() has been called as you said
>  - inside this function, check comment
>    "/* disable capture, disable interrupts */"
>    => this ensures no interrupt can be generated anymore
> 
> So after pxa_camera_remove_device() has been called, no interrupts can be
> generated.
> 
> Yet as you said, it leaves the "almost impossible" scenario :
>  - a user begins a capture
>  - the user closes the capture device and unloads pxa-camera.ko:
>      soc_camera_close()
>        pxa_camera_remove_device()
>          the IRQ line is asserted but doesn't trigger yet the interrupt handler
>          (yes I know, improbable)
>          meanwhile, IRQs are disabled, DMA channels are stopped
>      switch_to(rmmod)
>        => yes I know, impossible, the interrupt handler must be run before, but
>        let's continue for love of discussion ...
>      rmmod pxa-camera
>        pxa_camera_remove()
>          pxa_free_dma() * 3
>          ----> here the IRQ handler kicks in !!!
>                => pxa_camera_irq()
>                     pxa_dma_start_channels()
>          ----> it hurts !
> 
> My call is that this is impossible because the switch_to() should run the IRQ
> handler before pxa_camera_remove() is called.
> 
> So all this to say that I think we're safe, unless a heavy ion or a cosmic ray
> strikes the PXA :)

Thanks for the explanation.

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