search

bitcoin-core / secp256k1

Optimized C library for EC operations on curve secp256k1
MIT License
2.02k stars 977 forks source link

msan: use of uninitialized value in secp256k1_scalar_mul_shift_var #1511

Closed fanquake closed 3 months ago

fanquake commented 3 months ago

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

Ubuntu clang version 17.0.6 (5build1) (x86_64)
./autogen.sh
./configure CFLAGS="-fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls" CC=clang
Build Options:
  with external callbacks = no
  with benchmarks         = yes
  with tests              = yes
  with ctime tests        = yes
  with coverage           = no
  with examples           = no
  module ecdh             = yes
  module recovery         = no
  module extrakeys        = yes
  module schnorrsig       = yes
  module ellswift         = yes

  asm                     = x86_64
  ecmult window size      = 15
  ecmult gen prec. bits   = 4

  valgrind                = yes
  CC                      = clang
  CPPFLAGS                = 
  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 
  CFLAGS                  = -fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls
  LDFLAGS                 = 
make check -j17
<snip>
cat test-suite.log 
==============================================
   libsecp256k1 0.4.2-dev: ./test-suite.log
==============================================

# TOTAL: 3
# PASS:  2
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: tests
===========

test count = 64
random seed = 799c333349f126d5ee67e7ac48fa9dc8
==3962699==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x556d1c78cb8d in secp256k1_scalar_mul_shift_var /root/secp256k1/src/scalar_4x64_impl.h:909:5
    #1 0x556d1c7894d8 in secp256k1_scalar_split_lambda /root/secp256k1/src/scalar_impl.h:162:5
    #2 0x556d1c793be0 in secp256k1_ecmult_strauss_wnaf /root/secp256k1/src/ecmult_impl.h:256:9
    #3 0x556d1c674a50 in secp256k1_ecmult /root/secp256k1/src/ecmult_impl.h:355:5
    #4 0x556d1c674a50 in secp256k1_ecdsa_sig_verify /root/secp256k1/src/ecdsa_impl.h:212:5
    #5 0x556d1c70daba in run_proper_context_tests /root/secp256k1/src/tests.c:460:5
    #6 0x556d1c6b4257 in main /root/secp256k1/src/tests.c:7554:5
    #7 0x7f0ab902a1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #8 0x7f0ab902a28a in __libc_start_main csu/../csu/libc-start.c:360:3
    #9 0x556d1c5dc2a4 in _start (/root/secp256k1/tests+0x322a4) (BuildId: c02b23f447c54427d446d0e1ed03077d0b65a6a6)

  Uninitialized value was stored to memory at
    #0 0x556d1c78cb86 in secp256k1_scalar_mul_shift_var /root/secp256k1/src/scalar_4x64_impl.h:909:38
    #1 0x556d1c7894d8 in secp256k1_scalar_split_lambda /root/secp256k1/src/scalar_impl.h:162:5
    #2 0x556d1c793be0 in secp256k1_ecmult_strauss_wnaf /root/secp256k1/src/ecmult_impl.h:256:9
    #3 0x556d1c674a50 in secp256k1_ecmult /root/secp256k1/src/ecmult_impl.h:355:5
    #4 0x556d1c674a50 in secp256k1_ecdsa_sig_verify /root/secp256k1/src/ecdsa_impl.h:212:5
    #5 0x556d1c70daba in run_proper_context_tests /root/secp256k1/src/tests.c:460:5
    #6 0x556d1c6b4257 in main /root/secp256k1/src/tests.c:7554:5
    #7 0x7f0ab902a1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #8 0x7f0ab902a28a in __libc_start_main csu/../csu/libc-start.c:360:3
    #9 0x556d1c5dc2a4 in _start (/root/secp256k1/tests+0x322a4) (BuildId: c02b23f447c54427d446d0e1ed03077d0b65a6a6)

  Uninitialized value was created by an allocation of 'l' in the stack frame
    #0 0x556d1c78c151 in secp256k1_scalar_mul_shift_var /root/secp256k1/src/scalar_4x64_impl.h:893:5

SUMMARY: MemorySanitizer: use-of-uninitialized-value /root/secp256k1/src/scalar_4x64_impl.h:909:5 in secp256k1_scalar_mul_shift_var
Exiting
FAIL tests (exit status: 1)

Related to https://github.com/bitcoin/bitcoin/pull/29742.

maflcko commented 3 months ago

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

real-or-random commented 3 months ago

The accessed value is created by asm in secp256k1_scalar_mul_512. https://github.com/bitcoin-core/secp256k1/pull/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 3 months ago

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 3 months ago

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 3 months ago

Closing as fixed now that #1512 is merged.