* Re: interesting commit about llvm introducing barrier_data() [not found] ` <20160223142336.GK3522@linux.vnet.ibm.com> @ 2016-02-23 14:32 ` Mathieu Desnoyers 2016-02-23 14:46 ` Stephan Mueller 0 siblings, 1 reply; 4+ messages in thread From: Mathieu Desnoyers @ 2016-02-23 14:32 UTC (permalink / raw) To: Paul E. McKenney Cc: Peter Zijlstra, Will Deacon, Linus Torvalds, LKML, Stephan Mueller, Daniel Borkmann, Theodore Ts'o, Hannes Frederic Sowa, mancha security, Mark Charlebois, Behan Webster, Herbert Xu ----- On Feb 23, 2016, at 9:23 AM, Paul E. McKenney paulmck@linux.vnet.ibm.com wrote: > On Tue, Feb 23, 2016 at 02:02:26PM +0000, Mathieu Desnoyers wrote: >> commit 7829fb09a2b4268b30dd9bc782fa5ebee278b137 >> Author: Daniel Borkmann <daniel@iogearbox.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@187.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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: interesting commit about llvm introducing barrier_data() 2016-02-23 14:32 ` interesting commit about llvm introducing barrier_data() Mathieu Desnoyers @ 2016-02-23 14:46 ` Stephan Mueller 2016-02-23 14:58 ` Mathieu Desnoyers 0 siblings, 1 reply; 4+ messages in thread From: Stephan Mueller @ 2016-02-23 14:46 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Paul E. McKenney, Peter Zijlstra, Will Deacon, Linus Torvalds, LKML, Daniel Borkmann, Theodore Ts'o, Hannes Frederic Sowa, mancha security, Mark Charlebois, Behan Webster, Herbert Xu [-- Attachment #1: Type: text/plain, Size: 913 bytes --] Am Dienstag, 23. Februar 2016, 14:32:43 schrieb Mathieu Desnoyers: Hi Mathieu, > ----- On Feb 23, 2016, at 9:23 AM, Paul E. McKenney paulmck@linux.vnet.ibm.com wrote: > > On Tue, Feb 23, 2016 at 02:02:26PM +0000, Mathieu Desnoyers wrote: > >> commit 7829fb09a2b4268b30dd9bc782fa5ebee278b137 > >> Author: Daniel Borkmann <daniel@iogearbox.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()". > The key idea of the memzero_explicit is about forcing the compiler to do a memset. See the trivial test attached. Ciao Stephan [-- Attachment #2: memset_secure.c.xz --] [-- Type: application/x-xz, Size: 1708 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: interesting commit about llvm introducing barrier_data() 2016-02-23 14:46 ` Stephan Mueller @ 2016-02-23 14:58 ` Mathieu Desnoyers 2016-02-23 16:10 ` Paul E. McKenney 0 siblings, 1 reply; 4+ messages in thread From: Mathieu Desnoyers @ 2016-02-23 14:58 UTC (permalink / raw) To: Stephan Mueller Cc: Paul E. McKenney, Peter Zijlstra, Will Deacon, Linus Torvalds, LKML, Daniel Borkmann, Theodore Tso, Hannes Frederic Sowa, mancha security, Mark Charlebois, Behan Webster, Herbert Xu, rostedt ----- On Feb 23, 2016, at 9:46 AM, Stephan Mueller smueller@chronox.de wrote: > Am Dienstag, 23. Februar 2016, 14:32:43 schrieb Mathieu Desnoyers: > > Hi Mathieu, > >> ----- On Feb 23, 2016, at 9:23 AM, Paul E. McKenney > paulmck@linux.vnet.ibm.com wrote: >> > On Tue, Feb 23, 2016 at 02:02:26PM +0000, Mathieu Desnoyers wrote: >> >> commit 7829fb09a2b4268b30dd9bc782fa5ebee278b137 >> >> Author: Daniel Borkmann <daniel@iogearbox.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()". >> > The key idea of the memzero_explicit is about forcing the compiler to do a > memset. > > See the trivial test attached. My question is mainly about documentation of the new "barrier_data()" added to include/linux/compiler-gcc.h. Its comment does not clearly state where it should be used, and where it should not be needed. If it is useful for clearing memory for security purposes, it should be stated in the comment above the macro, and in the memory-barriers.txt Documentation file. If it is useful for securely clearing local variables in registers and on stack, it should be documented. Or if variables sitting on stack are not a target here, it should be documented too. If there is any way this could have impacts on DMA reads/writes (typically only global and allocated variables), it should be documented. If beyond the "clearing memory for security" use-case, this new barrier is needed rather than barrier() for code correctness, it should also be documented. Thanks, Mathieu > > Ciao > Stephan -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: interesting commit about llvm introducing barrier_data() 2016-02-23 14:58 ` Mathieu Desnoyers @ 2016-02-23 16:10 ` Paul E. McKenney 0 siblings, 0 replies; 4+ messages in thread From: Paul E. McKenney @ 2016-02-23 16:10 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Stephan Mueller, Peter Zijlstra, Will Deacon, Linus Torvalds, LKML, Daniel Borkmann, Theodore Tso, Hannes Frederic Sowa, mancha security, Mark Charlebois, Behan Webster, Herbert Xu, rostedt On Tue, Feb 23, 2016 at 02:58:37PM +0000, Mathieu Desnoyers wrote: > ----- On Feb 23, 2016, at 9:46 AM, Stephan Mueller smueller@chronox.de wrote: > > > Am Dienstag, 23. Februar 2016, 14:32:43 schrieb Mathieu Desnoyers: > > > > Hi Mathieu, > > > >> ----- On Feb 23, 2016, at 9:23 AM, Paul E. McKenney > > paulmck@linux.vnet.ibm.com wrote: > >> > On Tue, Feb 23, 2016 at 02:02:26PM +0000, Mathieu Desnoyers wrote: > >> >> commit 7829fb09a2b4268b30dd9bc782fa5ebee278b137 > >> >> Author: Daniel Borkmann <daniel@iogearbox.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()". > >> > > The key idea of the memzero_explicit is about forcing the compiler to do a > > memset. > > > > See the trivial test attached. > > My question is mainly about documentation of the new "barrier_data()" > added to include/linux/compiler-gcc.h. Its comment does not clearly > state where it should be used, and where it should not be needed. > > If it is useful for clearing memory for security purposes, it > should be stated in the comment above the macro, and in the > memory-barriers.txt Documentation file. > > If it is useful for securely clearing local variables in > registers and on stack, it should be documented. Or if > variables sitting on stack are not a target here, it should > be documented too. > > If there is any way this could have impacts on DMA reads/writes > (typically only global and allocated variables), it should be > documented. > > If beyond the "clearing memory for security" use-case, this > new barrier is needed rather than barrier() for code correctness, > it should also be documented. Looks like this is an issue only for code that doesn't use WRITE_ONCE() or better for writes to shared variables. Of which there does appear to be a great deal in the kernel, to be sure... Thanx, Paul ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-23 16:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1168693945.5302.1456236146207.JavaMail.zimbra@efficios.com>
[not found] ` <20160223142336.GK3522@linux.vnet.ibm.com>
2016-02-23 14:32 ` interesting commit about llvm introducing barrier_data() Mathieu Desnoyers
2016-02-23 14:46 ` Stephan Mueller
2016-02-23 14:58 ` Mathieu Desnoyers
2016-02-23 16:10 ` Paul E. McKenney
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.