Suppress a harmless variable-time optimization by clang in memczero

Summary:
 * Suppress a harmless variable-time optimization by clang in memczero

This has been not been caught by the new constant-time tests because
valgrind currently gives us a zero exit code even if finds errors, see
bitcoin-core/secp256k1#723 (comment) .

This commit also simplifies the arithmetic in memczero.

Note that the timing leak here was the bit whether a secret key was
out of range. This leak is harmless and not exploitable. It is just
our overcautious practice to prefer constant-time code even here.

 * Add test for memczero()

This is a backport of libsecp256k1 [[bitcoin-core/secp256k1#728 | PR728]]

Test Plan:
  ninja all check check-secp256k1

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D6363

src/secp256k1/src/tests.c

@@ -5173,6 +5173,21 @@ void run_ecdsa_openssl(void) {
 # include "modules/schnorr/tests_impl.h"
 #endif
 
+void run_memczero_test(void) {
+    unsigned char buf1[6] = {1, 2, 3, 4, 5, 6};
+    unsigned char buf2[sizeof(buf1)];
+
+    /* memczero(..., ..., 0) is a noop. */
+    memcpy(buf2, buf1, sizeof(buf1));
+    memczero(buf1, sizeof(buf1), 0);
+    CHECK(memcmp(buf1, buf2, sizeof(buf1)) == 0);
+
+    /* memczero(..., ..., 1) zeros the buffer. */
+    memset(buf2, 0, sizeof(buf2));
+    memczero(buf1, sizeof(buf1) , 1);
+    CHECK(memcmp(buf1, buf2, sizeof(buf1)) == 0);
+}
+
 int main(int argc, char **argv) {
     unsigned char seed16[16] = {0};
     unsigned char run32[32] = {0};
@@ -5315,6 +5330,9 @@ int main(int argc, char **argv) {
     run_schnorr_tests();
 #endif
 
+    /* util tests */
+    run_memczero_test();
+
     secp256k1_rand256(run32);
     printf("random run = %02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x\n", run32[0], run32[1], run32[2], run32[3], run32[4], run32[5], run32[6], run32[7], run32[8], run32[9], run32[10], run32[11], run32[12], run32[13], run32[14], run32[15]);
 

src/secp256k1/src/util.h

@@ -160,13 +160,16 @@ static SECP256K1_INLINE void *manual_alloc(void** prealloc_ptr, size_t alloc_siz
 SECP256K1_GNUC_EXT typedef unsigned __int128 uint128_t;
 #endif
 
-/* Zero memory if flag == 1. Constant time. */
+/* Zero memory if flag == 1. Flag must be 0 or 1. Constant time. */
 static SECP256K1_INLINE void memczero(void *s, size_t len, int flag) {
-    unsigned char *p;
-    unsigned char mask = -(unsigned char)flag;
-    p = (unsigned char *)s;
+    unsigned char *p = (unsigned char *)s;
+    /* Access flag with a volatile-qualified lvalue.
+       This prevents clang from figuring out (after inlining) that flag can
+       take only be 0 or 1, which leads to variable time code. */
+    volatile int vflag = flag;
+    unsigned char mask = -(unsigned char) vflag;
     while (len) {
-        *p ^= *p & mask;
+        *p &= ~mask;
         p++;
         len--;
     }