Re: interesting commit about llvm introducing barrier_data()

Re: interesting commit about llvm introducing barrier_data()

[Mathieu Desnoyers]

----- 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

Re: interesting commit about llvm introducing barrier_data()

[Stephan Mueller]

[-- 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 --]

Re: interesting commit about llvm introducing barrier_data()

[Mathieu Desnoyers]

----- 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

Re: interesting commit about llvm introducing barrier_data()

[Paul E. McKenney]

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