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] [day] [month] [year] [list]

Message-ID: <1452f3a7-fb4e-7a38-4cde-e7cce781102b@metafoo.de>
Date:	Wed, 3 Aug 2016 13:35:20 +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 01:19 PM, Lars-Peter Clausen wrote:
> 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.

Looking at the driver it also seems that just calling tasklet_kill() is not
enough. The tasklet_schedule() in the sdma_int_handler() is not synchronized
to anything. So if the interrupt triggers just at the right time it might
re-schedule the tasklet after tasklet_kill() has been called. Especially if
the tasklet_kill() runs on a different CPU.