linux-kernel - Re: [PATCH] staging: android: ion: Set the length of the DMA sg entries in buffer

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: <56A17ED3.80105@redhat.com>
Date:	Thu, 21 Jan 2016 16:58:59 -0800
From:	Laura Abbott <labbott@...hat.com>
To:	"Jon Medhurst (Tixy)" <tixy@...aro.org>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Arve Hjønnevåg <arve@...roid.com>,
	Riley Andrews <riandrews@...roid.com>,
	Liviu Dudau <Liviu.Dudau@....com>, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org, Robin Murphy <robin.murphy@....com>
Subject: Re: [PATCH] staging: android: ion: Set the length of the DMA sg
 entries in buffer

On 01/21/2016 12:19 PM, Jon Medhurst (Tixy) wrote:
> On Thu, 2016-01-21 at 09:39 -0800, Laura Abbott wrote:
>> On 01/21/2016 03:57 AM, Jon Medhurst (Tixy) wrote:
>>> From: Liviu Dudau <Liviu.Dudau@....com>
>>>
>>> ion_buffer_create() will allocate a buffer and then create a DMA
>>> mapping for it, but it forgot to set the length of the page entries.
>>>
>>> Signed-off-by: Liviu Dudau <Liviu.Dudau@....com>
>>> Signed-off-by: Jon Medhurst <tixy@...aro.org>
>>> ---
>>>    drivers/staging/android/ion/ion.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>>> index e237e9f..df56021 100644
>>> --- a/drivers/staging/android/ion/ion.c
>>> +++ b/drivers/staging/android/ion/ion.c
>>> @@ -251,8 +251,10 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
>>>    	 * memory coming from the heaps is ready for dma, ie if it has a
>>>    	 * cached mapping that mapping has been invalidated
>>>    	 */
>>> -	for_each_sg(buffer->sg_table->sgl, sg, buffer->sg_table->nents, i)
>>> +	for_each_sg(buffer->sg_table->sgl, sg, buffer->sg_table->nents, i) {
>>>    		sg_dma_address(sg) = sg_phys(sg);
>>> +		sg_dma_len(sg) = sg->length;
>>> +	}
>>>    	mutex_lock(&dev->buffer_lock);
>>>    	ion_buffer_add(dev, buffer);
>>>    	mutex_unlock(&dev->buffer_lock);
>>>
>>
>> So Ion is really doing it wrong by setting the sg_dma_address manually as
>> the comment above notes. Ion has moved away from sg_dma_len though
>> (see 06e0dcaeb4fd72a010a1f5ad0c03abd8e0a58ef9). This isn't technically
>> a mapping as well. What's broken by not having sg_dma_len set?
>
> I fear this could end up being embarrassing...
>
> What's broken is that the out-of-tree kernel driver for ARM's Mali GPU
> is getting passed a dma_buf corresponding to the ION buffer. It is then
> calling dma_buf_map_attachment [1] on that and then parsing the
> resultant scatter-gather list to get the physical pages so it can pass
> them to the GPU hardware. In the process, it is using sg_dma_len() to
> get the length, which is garbage for ION buffers if ion_buffer_create()
> doesn't set it.
>
> [1] http://git.linaro.org/landing-teams/working/arm/kernel-release.git/blob/9660bff61ab296be02aad111d0bc2b9919493de5:/drivers/gpu/arm/midgard/mali_kbase_jd.c#l333
>
> Now, I just tried making the Mali driver use sg->length rather than
> sg_dma_len() and, unsurprisingly, that also fixes the problem. So, my
> questions would be...
>
> Is it acceptable for a driver getting a dma_buf to parse the
> scatter-gather list for that by had?
>
> If so, should it use ->length or sg_dma_len() to get the length of each
> element?
>
> If sg_dma_len() is correct or acceptable then it seems to me that the
> ION code should set that length. Especially as the comment in the code
> implies it's faking a call to map_sg and grepping the kernel tree for
> real implementations of that functionality seems to show the dma_address
> getting set.
>
> As you can probably tell, I feel I may be on shaky ground. This is
> because I don't fully understanding the code and suspecting both the ION
> and GPU code is rather dodgy (and possibly the bits in between :-)
>

I blame the Ion code completely. I remember hitting a similar problem
with other out of tree drivers. The solution then was to have drivers
switch to using sg->length instead of sg_dma_len given the state of that
tree. For the Mali driver, if it is ever going to be backed by an IOMMU
you will need to use sg_dma_len so I think at least that part of your
code is correct.

Thinking about it some, I'm okay with the patch going in. I thought
there was some reason why the out of tree code from before didn't just
do this hack but I can't remember it. It may have been an out of tree
use case. This does go well with Ion's behavior of pretending to do
DMA mapping. More out of tree users can plead their case if it breaks.

Acked-by: Laura Abbott <labbott@...hat.com>