msan: notate variable assignments from assembly code #1496

1. 
   theuni commented at 4:45 pm on February 21, 2024: contributor

   msan isn’t smart enough to see that these are set without some help.

   This was pointed out here: #1169 (comment)

   With this commit, msan output is clean even with x86 asm turned on.

2. real-or-random approved
3. 
   real-or-random commented at 5:08 pm on February 21, 2024: contributor

   utACK 8f237f6ff13f5654b1ca7d31481ca96e58fc7d02

   Oh, that’s nice. I run into this regularly, but I didn’t think of this simple solution.

4. real-or-random added the label assurance on Feb 21, 2024
5. 
   fanquake commented at 5:27 pm on February 21, 2024: member

   Nice. Concept ACK. I’ll run this through out MSAN CI while using asm.
6. 
   hebasto commented at 8:55 pm on February 21, 2024: member

   Concept ACK.

   Is still there any specific reason to keep ASM=no in the CI?

   FWIW, I’ve removed it everywhere in the .github/workflows/ci.yml and the CI passed.

7. 
   real-or-random commented at 11:58 am on February 22, 2024: contributor

   Is still there any specific reason to keep ASM=no in the CI?

   Yes, I think we want to test the non-ASM code even on x86_64. But you’re right in principle. We should probably enable ASM in some more tasks. (Reworking the matrix something we should do anyway. It’s one of the few boxes we haven’t ticked in #1392 … :/ )

8. hebasto approved
9. 
   hebasto commented at 4:02 pm on February 22, 2024: member

   ACK 8f237f6ff13f5654b1ca7d31481ca96e58fc7d02, tested on Ubuntu 23.10. Removing any of added macros triggers “MemorySanitizer: use-of-uninitialized-value” warning in the setup mentioned in the PR description.
10. 
    real-or-random commented at 6:29 pm on February 22, 2024: contributor

    @theuni Do you think it’s worth making these conditional under __has_feature(memory_sanitizer) (or cleaner, a macro SECP256K1_CHECKMEM_MSAN to be added to checkmem.h) to make sure that this workaround applies only to MSan, and we never suppress positives omitted by Valgrind?
11. 
    theuni commented at 7:53 pm on February 22, 2024: contributor

    @real-or-random I didn’t understand your question at first but I think maybe I know what you’re asking now. I’ll try to rephrase:

    Because notation is necessary for msan but not valgrind (I don’t know if that’s true, I’m assuming based on your question), there should be a macro that is used only to notate for msan without affecting valgrind’s detection? If that’s right, sure, I can hook up a SECP256K1_CHECKMEM_MSAN. I’m unsure if other parts of the code would also benefit from one but not the other, but presumably if so we’d want to switch them over too.

    edit: Like this, I think? (untested)

     0diff --git a/src/checkmem.h b/src/checkmem.h
     1index f2169de..68db9a7 100644
     2--- a/src/checkmem.h
     3+++ b/src/checkmem.h
     4@@ -48,11 +48,16 @@
     5 #    define SECP256K1_CHECKMEM_ENABLED 1
     6 #    define SECP256K1_CHECKMEM_UNDEFINE(p, len) __msan_allocated_memory((p), (len))
     7 #    define SECP256K1_CHECKMEM_DEFINE(p, len) __msan_unpoison((p), (len))
     8+#    define SECP256K1_CHECKMEM_MSAN(p, len) __msan_unpoison((p), (len))
     9 #    define SECP256K1_CHECKMEM_CHECK(p, len) __msan_check_mem_is_initialized((p), (len))
    10 #    define SECP256K1_CHECKMEM_RUNNING() (1)
    11 #  endif
    12 #endif
    13
    14+#ifndef SECP256K1_CHECKMEM_MSAN
    15+#  define SECP256K1_CHECKMEM_MSAN(p, len) SECP256K1_CHECKMEM_NOOP((p), (len))
    16+#endif
    17+
    18 /* If valgrind integration is desired (through the VALGRIND define), implement the
    19  * SECP256K1_CHECKMEM_* macros using valgrind. */
    20 #if !defined SECP256K1_CHECKMEM_ENABLED
    

    Logically, wrapping it in an #ifndef VALGRIND would make sense too, but I didn’t because of the comment above it:

    0/* If compiling under msan, map the SECP256K1_CHECKMEM_* functionality to msan.
    1 * Choose this preferentially, even when VALGRIND is defined, as msan-compiled
    2 * binaries can't be run under valgrind anyway. */
    

    Nit: I think SECP256K1_CHECKMEM_MSAN_DEFINE would be a better name so the other macros could get the same treatment in the future if necessary.

12. 
    real-or-random commented at 5:02 pm on February 23, 2024: contributor

    Yeah, sorry, my comment was not very detailed. :)

    Because notation is necessary for msan but not valgrind (I don’t know if that’s true, I’m assuming based on your question),

    Indeed. MSan’s analysis needs instrumentation from the compiler, and it’s not surprising that the compiler is not clever enough to understand our ASM and instrument the binary accordingly. Valgrind’s analysis looks at the final (uninstrumented) binary only, and thus doesn’t care at all if the code has been generated from C or asm or whatever.

    there should be a macro that is used only to notate for msan without affecting valgrind’s detection? If that’s right, sure, I can hook up a SECP256K1_CHECKMEM_MSAN.

    That’s what I had in mind, yes.

    I’m unsure if other parts of the code would also benefit from one but not the other, but presumably if so we’d want to switch them over too.

    I grepped for SECP256K1_CHECKMEM_DEFINE, and it turns out that all current uses of the macro are related to constant-time testing. In these cases, we’ll actually need it for both MSan and Valgrind. So no, no need to change any existing uses.

    Nit: I think SECP256K1_CHECKMEM_MSAN_DEFINE would be a better name so the other macros could get the same treatment in the future if necessary.

    Yeah, totally. Sorry, what I actually had in mind was that SECP256K1_CHECKMEM_MSAN (or perhaps better: ``SECP256K1_CHECKMEM_USES_MSAN`) is macro that indicates that we use MSan, and then we could do this after the asm code.

    0#if SECP256K1_CHECKMEM_USES_MSAN
    1    SECP256K1_CHECKMEM_DEFINE(...)
    2    ...
    3#endif
    

    And there could also be a SECP256K1_CHECKMEM_USES_VALGRIND macro.

    I think that’s a tiny tiny bit simpler in a possible future with more than two backends, but really, either approach is fine in the end. And I agree, if we follow your approach, then SECP256K1_CHECKMEM_MSAN_DEFINE is a better name for sure.

13. msan: Add SECP256K1_CHECKMEM_MSAN_DEFINE which applies to memory sanitizer and not valgrind e7ea32e30a
14. msan: notate variable assignments from assembly code

    msan isn't smart enough to see that these are set without some help.

    31ba404944
15. theuni force-pushed on Feb 23, 2024
16. 
    theuni commented at 5:31 pm on February 23, 2024: contributor

    Since you’re ok with either approach, I pushed up the one I already had ready :)
17. real-or-random approved
18. 
    real-or-random commented at 10:46 am on February 25, 2024: contributor

    utACK 31ba40494428dcbf2eb5eb6f2328eca91b0b0746
19. hebasto approved
20. 
    hebasto commented at 1:00 pm on February 27, 2024: member

    re-ACK 31ba40494428dcbf2eb5eb6f2328eca91b0b0746.

    Just leaving here a reminder to reconsider disabling assembly in the CI jobs.

21. real-or-random merged this on Feb 27, 2024
22. real-or-random closed this on Feb 27, 2024

    ---

23. achow101 referenced this in commit b525369f8e on Mar 25, 2024
24. 
    fanquake commented at 6:47 pm on March 26, 2024: member

    Have a followup to this here: https://github.com/bitcoin/bitcoin/pull/29742.

Contributors
theuni real-or-random fanquake hebasto

Labels
assurance