Message-ID: <675593567.5328.1456237963352.JavaMail.zimbra@efficios.com>
Date:	Tue, 23 Feb 2016 14:32:43 +0000 (UTC)
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Will Deacon <will.deacon@....com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Stephan Mueller <smueller@...onox.de>,
	Daniel Borkmann <daniel@...earbox.net>,
	Theodore Ts'o <tytso@....edu>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	mancha security <mancha1@...o.com>,
	Mark Charlebois <charlebm@...il.com>,
	Behan Webster <behanw@...verseincode.com>,
	Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: interesting commit about llvm introducing barrier_data()

----- On Feb 23, 2016, at 9:23 AM, Paul E. McKenney paulmck@...ux.vnet.ibm.com wrote:

> On Tue, Feb 23, 2016 at 02:02:26PM +0000, Mathieu Desnoyers wrote:
>> commit 7829fb09a2b4268b30dd9bc782fa5ebee278b137
>> Author: Daniel Borkmann <daniel@...earbox.net>
>> Date:   Thu Apr 30 04:13:52 2015 +0200
>> 
>>     lib: make memzero_explicit more robust against dead store elimination
>> 
>> ^ interesting commit. Any idea on the impact of this on kernel RCU
>> implementation and liburcu cmm_barrier() ?
> 
> First I knew of it!  But I bet that more like this are needed.  ;-)

I recommend you check my IRC discussion with peterz on the matter of
this new "barrier_data()".

Let us know if we are missing anything,

(opening up the discussion to a larger audience)

09:03 < Compudj> peterz: have you seem commit 7829fb09a2b4268b30dd9bc782fa5ebee278b137 "lib: make memzero_explicit more robust against dead store elimination" which introduces a barrier_data() ? Any thoughts on 
                 its impact on preempt disable ?
09:04 < Compudj> (icc/llvm related)
09:04  * peterz goes look
09:06 < peterz> yuck
09:06 < Compudj> yeah, that sucks :/
09:09 < Compudj> but it makes me wonder if that patch is not just bogus...
09:09 < Compudj> I tries to keep a memset to zero from being eliminated as a "dead store"
09:10 < Compudj> but if the compiler can prove that the zeroed memory is only ever in registers, and never used otherwise (in read), it could very well eliminate the zeroing
09:10 < Compudj> the only issue I see is with DMA writes
09:10 < peterz> but the whole point of barrier() was to be a compiler barrier, that is force emit prior stores and reads, such that subsequent code can rely on it
09:11 < peterz> imagine it being a function call
09:11 < peterz> no saying what inside the function call will touch buf
09:11 < Compudj> yes, but DMA-aside, if no subsequent code need to access what has been written (proven by the compiler), do we care about the store ?
09:11 < Compudj> peterz: in this case I would expect the compiler to be conservative, and assume nothing
09:12 < peterz> hmm, so the example uses a function local buffer, whose address is never taken
09:13 < Compudj> peterz: yes... hard to be more local than that
09:14 < Compudj> it really starts to look like a bogus "fix" for an issue that has never been really observed outside of this peculiar trivial example
09:14 < peterz> and I suppose the whole point here is to wipe keys from memory, even if local?
09:14 < Compudj> "Arguably, in the current kernel tree it's more of a theoretical issue, but imho, it's better to be pedantic about it.
09:14 < Compudj> "
09:15 < Compudj> not even sure why they care
09:15 < peterz> no, if llvm goes around and breaks all kinds of assumptions, it simply isn't a viable compiler for the kernel, full stop
09:16 < Compudj> perhaps, but I don't see a clear explanation of *why* this would be a bogus behavior in the patch changelog
09:16 < Compudj> besides the trivial main() example with an array on the stack
09:17 < Compudj> I would expect the compiler to be able to optimize away this store without hurting anything if it can prove that nobody cares about this stack content
09:19 < peterz> right, so the 'real' case I can imagine, is if the memset is the last time before the variable goes out of scope
09:19 < peterz> s/time/thing/
09:19 -!- acme [~acme@....65.76.58] has joined #linux-rt
09:19 < peterz> given the purpose of not leaking the 'key' for longer than needed, you want that memset to happen
09:20 < peterz> but the compiler can see its a 'pointless' store, since the variable is about to not exist after that
09:20 < Compudj> peterz: I can see the security concern indeed
09:20 < Compudj> but not as a correctness one.. ?
09:20 < peterz> right, not yet
09:20 < Compudj> so the only case where we should care about it is for memset then... AFAIU
09:20 < peterz> they need to do more crazy for correctness to be an issue
09:21 < Compudj> (that should be clearly documented above barrier_data())
09:22 < Compudj> peterz: moreover, it would only "leak" a key in registers
09:22 < Compudj> not in memory
09:22 < Compudj> I'm kind of doubtful that it's really a threat
09:22 < peterz> no, it would actually leak it in memory
09:22 < peterz> it makes an 'as-if' argument
09:23 < Compudj> even with the memory asm clobber ?
09:24 < Compudj> that clobber really acts as if any memory could be touched
09:24 < Compudj> which cannot allow the compiler to assume the zeroes are untouched (if sitting in memory)
09:25 < peterz> "Yes, we will do this even if you use more bytes in 'buf' than you have bytes of registers (whether it's really in registers or memory is immaterial, what matters is that the compiler has proven 
                that the inline asm may not rely on 'buf' being in memory)." -- comment 5

Thanks,

Mathieu


> 
> 							Thanx, Paul

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Powered by blists - more mailing lists