linux-kernel - Re: [PATCH v2 02/31] cinergyT2-core: don't do DMA on stack

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, 17 Oct 2016 15:12:37 -0200
From:   Mauro Carvalho Chehab <mchehab@...pensource.com>
To:     Johannes Stezenbach <js@...uxtv.org>
Cc:     Linux Media Mailing List <linux-media@...r.kernel.org>,
        Mauro Carvalho Chehab <mchehab@...radead.org>,
        Andy Lutomirski <luto@...capital.net>,
        Jiri Kosina <jikos@...nel.org>,
        Patrick Boettcher <patrick.boettcher@...teo.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Michael Krufky <mkrufky@...uxtv.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Jörg Otte <jrg.otte@...il.com>
Subject: Re: [PATCH v2 02/31] cinergyT2-core: don't do DMA on stack

Em Sat, 15 Oct 2016 22:54:49 +0200
Johannes Stezenbach <js@...uxtv.org> escreveu:

> On Tue, Oct 11, 2016 at 07:09:17AM -0300, Mauro Carvalho Chehab wrote:
> > --- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
> > +++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
> > @@ -41,6 +41,8 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
> >  
> >  struct cinergyt2_state {
> >  	u8 rc_counter;
> > +	unsigned char data[64];
> > +	struct mutex data_mutex;
> >  };  
> 
> Sometimes my thinking is slow but it just occured to me
> that this creates a potential issue with cache line sharing.
> On an architecture which manages cache coherence in software
> (ARM, MIPS etc.) a write to e.g. rc_counter in this example
> would dirty the cache line, and a later writeback from the
> cache could overwrite parts of data[] which was received via DMA.
> In contrast, if the DMA buffer is allocated seperately via
> kmalloc it is guaranteed to be safe wrt cache line sharing.
> (see bottom of Documentation/DMA-API-HOWTO.txt).
> 

Interesting point. I'm not sure well this would work with non-fully
coherent cache lines. I guess that will depend on how the USB
driver will be handling it.

> But of course DMA on stack also had the same issue
> and no one ever noticed so it's apparently not critical...

Yes, this shouldn't do it any worse than what we currently have.
In the past, I tested some drivers that uses a shared buffed for control
URB transfers in the past, on arm32 and arm64. I don't remember seeing
anything weird there that could be related to cache coherency, although
I remember several problems with USB on OMAP and RPi version 1, leading
troubles after several minutes of ISOC transfers on analog TV,
but they seemed to be unrelated to URB control traffic.

I'd say that we should keep our eyes on those drivers, after applying
this patch series and see if people will notice bad behavior on non-x86
archs.

Thanks,
Mauro