msan: use of uninitialized value in secp256k1_scalar_mul_shift

fanquake commented at 10:13 am on March 27, 2024: member

Building master (05bfab69aef3622f77f754cfb01220108a109c91) in the following way (same flags as we use in our MSAN CI), results in the following failure:

 0Ubuntu clang version 17.0.6 (5build1) (x86_64)
 1./autogen.sh
 2./configure CFLAGS="-fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls" CC=clang
 3Build Options:
 4  with external callbacks = no
 5  with benchmarks         = yes
 6  with tests              = yes
 7  with ctime tests        = yes
 8  with coverage           = no
 9  with examples           = no
10  module ecdh             = yes
11  module recovery         = no
12  module extrakeys        = yes
13  module schnorrsig       = yes
14  module ellswift         = yes
15
16  asm                     = x86_64
17  ecmult window size      = 15
18  ecmult gen prec. bits   = 4
19
20  valgrind                = yes
21  CC                      = clang
22  CPPFLAGS                = 
23  SECP_CFLAGS             = -O2  -std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef -Wno-overlength-strings -Wall -Wno-unused-function -Wextra -Wcast-align -Wconditional-uninitialized -Wreserved-identifier -fvisibility=hidden 
24  CFLAGS                  = -fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls
25  LDFLAGS                 = 
26make check -j17
27<snip>
28cat test-suite.log 
29==============================================
30   libsecp256k1 0.4.2-dev: ./test-suite.log
31==============================================
32
33# TOTAL: 3
34# PASS:  2
35# SKIP:  0
36# XFAIL: 0
37# FAIL:  1
38# XPASS: 0
39# ERROR: 0
40
41.. contents:: :depth: 2
42
43FAIL: tests
44===========
45
46test count = 64
47random seed = 799c333349f126d5ee67e7ac48fa9dc8
48==3962699==WARNING: MemorySanitizer: use-of-uninitialized-value
49    [#0](/bitcoin-core-secp256k1/0/) 0x556d1c78cb8d in secp256k1_scalar_mul_shift_var /root/secp256k1/src/scalar_4x64_impl.h:909:5
50    [#1](/bitcoin-core-secp256k1/1/) 0x556d1c7894d8 in secp256k1_scalar_split_lambda /root/secp256k1/src/scalar_impl.h:162:5
51    [#2](/bitcoin-core-secp256k1/2/) 0x556d1c793be0 in secp256k1_ecmult_strauss_wnaf /root/secp256k1/src/ecmult_impl.h:256:9
52    [#3](/bitcoin-core-secp256k1/3/) 0x556d1c674a50 in secp256k1_ecmult /root/secp256k1/src/ecmult_impl.h:355:5
53    [#4](/bitcoin-core-secp256k1/4/) 0x556d1c674a50 in secp256k1_ecdsa_sig_verify /root/secp256k1/src/ecdsa_impl.h:212:5
54    [#5](/bitcoin-core-secp256k1/5/) 0x556d1c70daba in run_proper_context_tests /root/secp256k1/src/tests.c:460:5
55    [#6](/bitcoin-core-secp256k1/6/) 0x556d1c6b4257 in main /root/secp256k1/src/tests.c:7554:5
56    [#7](/bitcoin-core-secp256k1/7/) 0x7f0ab902a1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
57    [#8](/bitcoin-core-secp256k1/8/) 0x7f0ab902a28a in __libc_start_main csu/../csu/libc-start.c:360:3
58    [#9](/bitcoin-core-secp256k1/9/) 0x556d1c5dc2a4 in _start (/root/secp256k1/tests+0x322a4) (BuildId: c02b23f447c54427d446d0e1ed03077d0b65a6a6)
59
60  Uninitialized value was stored to memory at
61    [#0](/bitcoin-core-secp256k1/0/) 0x556d1c78cb86 in secp256k1_scalar_mul_shift_var /root/secp256k1/src/scalar_4x64_impl.h:909:38
62    [#1](/bitcoin-core-secp256k1/1/) 0x556d1c7894d8 in secp256k1_scalar_split_lambda /root/secp256k1/src/scalar_impl.h:162:5
63    [#2](/bitcoin-core-secp256k1/2/) 0x556d1c793be0 in secp256k1_ecmult_strauss_wnaf /root/secp256k1/src/ecmult_impl.h:256:9
64    [#3](/bitcoin-core-secp256k1/3/) 0x556d1c674a50 in secp256k1_ecmult /root/secp256k1/src/ecmult_impl.h:355:5
65    [#4](/bitcoin-core-secp256k1/4/) 0x556d1c674a50 in secp256k1_ecdsa_sig_verify /root/secp256k1/src/ecdsa_impl.h:212:5
66    [#5](/bitcoin-core-secp256k1/5/) 0x556d1c70daba in run_proper_context_tests /root/secp256k1/src/tests.c:460:5
67    [#6](/bitcoin-core-secp256k1/6/) 0x556d1c6b4257 in main /root/secp256k1/src/tests.c:7554:5
68    [#7](/bitcoin-core-secp256k1/7/) 0x7f0ab902a1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
69    [#8](/bitcoin-core-secp256k1/8/) 0x7f0ab902a28a in __libc_start_main csu/../csu/libc-start.c:360:3
70    [#9](/bitcoin-core-secp256k1/9/) 0x556d1c5dc2a4 in _start (/root/secp256k1/tests+0x322a4) (BuildId: c02b23f447c54427d446d0e1ed03077d0b65a6a6)
71
72  Uninitialized value was created by an allocation of 'l' in the stack frame
73    [#0](/bitcoin-core-secp256k1/0/) 0x556d1c78c151 in secp256k1_scalar_mul_shift_var /root/secp256k1/src/scalar_4x64_impl.h:893:5
74
75SUMMARY: MemorySanitizer: use-of-uninitialized-value /root/secp256k1/src/scalar_4x64_impl.h:909:5 in secp256k1_scalar_mul_shift_var
76Exiting
77FAIL tests (exit status: 1)

maflcko commented at 10:18 am on March 27, 2024: none

I guess it does not happen in valgrind, dropping -fsanitize=memory first in the CFLAGS?

real-or-random commented at 11:47 am on March 27, 2024: contributor

The accessed value is created by asm in secp256k1_scalar_mul_512. #1496 added annotations to secp256k1_scalar_reduce_512, but not to secp256k1_scalar_mul_512. So we’ll simply need to add those. I’m not sure why this wasn’t noticed in #1496, i.e., why MSAN was happy without the annotation. (Perhaps differences in clang versions etc?) cc @theuni

theuni commented at 1:28 pm on March 27, 2024: contributor

Huh, yeah, I’m not sure either. I never bumped into this with my testing. Will have a look and try to PR a fix.

theuni commented at 3:03 pm on March 27, 2024: contributor

It took me a while to reproduce… indeed clang-15 does not complain, but clang-17 does. Seeing as it detected something that clang-15 missed, but smarter tracking could potentially understand vars set in asm, it’s hard to say if newer clang is smarter or dumber here :p

Either way, I agree with @real-or-random that this needs annotations. Will PR a fix.

fanquake commented at 8:34 am on April 4, 2024: member

Closing as fixed now that #1512 is merged.

fanquake closed this on Apr 4, 2024

msan: use of uninitialized value in secp256k1_scalar_mul_shift_var #1511