linux-kernel - Re: [PATCH 1/1] Fix NULL pointer dereference in imx serial driver DMA callback

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:	Wed, 3 Aug 2016 13:19:27 +0200
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Fabien Lahoudere <fabien.lahoudere@...labora.co.uk>
Cc:	Hannu Koivisto <hannu.koivisto@...cit.fi>,
	Vinod Koul <vinod.koul@...el.com>,
	Dan Williams <dan.j.williams@...el.com>,
	"open list:DMA GENERIC OFFLOAD ENGINE SUBSYSTEM" 
	<dmaengine@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] Fix NULL pointer dereference in imx serial driver DMA
 callback

On 08/03/2016 12:59 PM, Fabien Lahoudere wrote:
> From: Hannu Koivisto <hannu.koivisto@...cit.fi>
> 
> dma_rx_callback() may see NULL dma_chan_rx if DMA interrupt [1] occurs a
> moment[2] before imx_uart_dma_exit() sets it to NULL.  imx_uart_dma_exit()
> calls dmaengine_terminate_all() and dma_release_channel() but neither of
> those prevent the callback being called after they have returned. A similar
> problem has been discussed by ALSA developers
> (http://mailman.alsa-project.org/pipermail/alsa-devel/2013-October/067239.html)
> and it was pointed out that dmaengine_terminate_all() might be called from
> the callback, so we cannot call tasklet_kill() in imx-sdma's code called by
> dmaengine_terminate_all().
> 
> Hopefully it doesn't make sense to call dma_release_channel() from the
> callback, so instead of adding synchronization to imx serial driver, we add
> tasklet_kill() call to sdma_free_chan_resources().  While most DMA drivers
> don't do that, there is one example that does: pl330.
> 
> [1] It schedules sdma_tasklet, which again calls the dma_rx_callback.
> [2] I tested this by scheduling the sdma tasklet as far as right before the
> imx_stop_tx() call in imx_shutdown() and the problem occurred.
> 
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@...labora.co.uk>

I'd prefer that the driver implements the new synchronization API[1]. This
is a more generic approach and covers of all cases of this race condition.

If the synchronize() callback is implemented the core will automatically
make sure that the channel is synchronized when it is freed.

- Lars

[1]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=b36f09c3c441a6e59eab9315032e7d546571de3f