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

Date:	Fri, 22 Jan 2016 09:43:23 +0000
From:	"Jon Medhurst (Tixy)" <tixy@...aro.org>
To:	Laura Abbott <labbott@...hat.com>
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 Thu, 2016-01-21 at 16:58 -0800, Laura Abbott wrote:
> On 01/21/2016 12:19 PM, Jon Medhurst (Tixy) wrote:
[...]
> > 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.

Well, the $subject patch is copying the value of 'length' into
'dma_length', and if dma_length was previously uninitialised then I
don't see that it can really cause additional problems for ION users. (I
hate the way I've been using 'if' a lot so I'm going to spend some time
educating myself.)

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

Thanks

-- 
Tixy