[trivial] clear stack variable by function

[isle2983]

The bare bits = 0; raises a clang static analysis issue.

--------------------------------------------------------------------------------
An issue has been found in ./src/ecmult_gen_impl.h:153:5
Type:         Dead assignment
Description:  Value stored to 'bits' is never read

0: ./src/ecmult_gen_impl.h:153:5 - Value stored to 'bits' is never read
--------------------------------------------------------------------------------

The line was introduced in 2f6c801 (when secp256k1_ecmult_gen() lived in src/ecmult_impl.h) with the purpose of clearing stack variables that might have secret info.

However, without that context, the purpose for clearing isn't obvious from reading the code. Aside from making clang happy, the function name should help it be understood.

[gmaxwell]

It's a fine structural change, but if you're going to break this out, can you perhaps make it a generic unsigned char clear that takes a point and length (which you'd use sizeof() on here)? --- and try to get the compiler to not optimize it out? Edit: on second thought, we may prefer a clear function per type, even if all they're doing is wrapping a generic one.

(As a matter of principle, we should never silence a warning which is potentially hiding an actual problem. In this case the problem is that the compiler will simply optimize out this assignment).

[isle2983]

The new branch has two commits with two tentative examples for comment. They focus just on the bits variable for the purpose of discussion even though the problem extends to the other *_clear() functions.

In the first example commit 72c0ebb, __attribute__((optimize("O0"))) achieves what we want and the call to secp256k1_secure_clear() can be seen at the bottom of the -03 assembly for secp256k1_ecmult_gen():

.LBE2707:
	.loc 6 135 0
	cmpq	$64, %rdi	#, ivtmp.241
	je	.L451	#,
	salq	$34, %rdi	#, D.9544
	movl	%r11d, 104(%rsp)	# D.9544, %sfp
	movl	%ebp, %r14d	# D.9544, D.9552
	shrq	$37, %rdi	#, D.9544
	movl	%r8d, 112(%rsp)	# D.9544, %sfp
	movl	%r12d, %r15d	# D.9544, D.9552
	movl	416(%rsp,%rdi,4), %edi	# gnb.d, D.9552
	movl	%r10d, 96(%rsp)	# D.9544, %sfp
	movl	%ecx, %r11d	# D.9544, D.9552
	movl	%esi, 88(%rsp)	# D.9544, %sfp
	movl	%edx, 80(%rsp)	# D.9544, %sfp
	movl	%r9d, %r13d	# D.9544, D.9552
	movl	%eax, 48(%rsp)	# D.9544, %sfp
	jmp	.L452	#
.L451:
.LVL2579:
.LBB2708:
.LBB2709:
	.loc 2 222 0
	leaq	412(%rsp), %rdi	#, tmp5277
.LVL2580:
	movl	$4, %esi	#,
	call	secp256k1_secure_clear	#
.LVL2581:
.LBE2709:
.LBE2708:
	.loc 6 156 0
	addq	$1144, %rsp	#,

(As was assumed, it was indeed being optimized out before, but I didn't quote it here to save space)

However, compiling the same with clang, it throws a warning and doesn't do what we want:

$ clang -I. -g -O3 -Wall -Wextra -Wno-unused-function -c src/gen_context.c -fverbose-asm -S
In file included from src/gen_context.c:13:
src/group_impl.h:217:28: warning: unknown attribute 'optimize' ignored
      [-Wunknown-attributes]
static void __attribute__((optimize("O0"))) secp256k1_secure_clear(void ...
                           ^
1 warning generated.

So, this approach looks like it is not very universal. ugh.

Taking a step back and looking at OpenSSL, they clear stack variables with a function named BN_clear_free() in (openssl)/crypto/bn/bn_lib.c which calls OPENSSL_cleanse() in (openssl)/crypto/mem_clr.c which is implemented with the volatile keyword:

/*
 * Pointer to memset is volatile so that compiler must de-reference
 * the pointer and can't assume that it points to any function in
 * particular (such as memset, which it then might further "optimize")
 */
typedef void *(*memset_t)(void *,int,size_t);

static volatile memset_t memset_func = memset;

void OPENSSL_cleanse(void *ptr, size_t len)
{
    memset_func(ptr, 0, len);
}

So, this approach is also tried in cdd4681. The assembly in -03 turns out a bit different, but still achieves the aim:

.LBE2372:
	.loc 6 135 0
	cmpq	$64, %rax	#, ivtmp.263
	je	.L424	#,
	salq	$34, %rax	#, D.9409
	shrq	$37, %rax	#, D.9409
	movl	400(%rsp,%rax,4), %r9d	# gnb.d, D.9415
	jmp	.L425	#
.L424:
.LVL2528:
.LBB2373:
.LBB2374:
.LBB2375:
	.loc 2 222 0
	leaq	396(%rsp), %rdi	#, tmp5505
.LVL2529:
	movq	secure_memset(%rip), %rax	# secure_memset, D.9410
	movl	$4, %edx	#,
	xorl	%esi, %esi	#
	call	*%rax	# D.9410
.LVL2530:
.LBE2375:
.LBE2374:
.LBE2373:
	.loc 6 156 0
	addq	$1176, %rsp	#,

This works perfectly with clang too, so this is definitely more viable.

Also, for C++ there is discussion on the internet around a new memset_s() call designed for this purpose but it is only introduced in C++11. In the long run, if memset_s() makes it to C, that will probably be the preferred way to do this.

Questions:

1. What kind of benchmarking needs to be done to quantify the performance impacts?
2. Should we look at applying this to all stack vars? or should the focus be a subset of functions that touch private keys?
3. Any cosmetic suggestions for function names/interface/presentation?

[sipa]

memset_s is an optional function in C11 (not C++11), and it doesn't seem like compilers are eager to implement it.

[gmaxwell]

10:59 < gmaxwell> glibc 2.25 has explicit_bzero
11:01 < gmaxwell> void
11:01 < gmaxwell> explicit_bzero (void s, size_t len)
11:01 < gmaxwell> {
11:01 < gmaxwell> memset (s, '\0', len);
11:01 < gmaxwell> / Compiler barrier. */
11:01 < gmaxwell> asm volatile ("" ::: "memory");
11:01 < gmaxwell> }

[isle2983]

closing in favor of #448