Missing early clobber #766

issue ehoffman2 opened this issue on July 14, 2020
  1. ehoffman2 commented at 5:42 PM on July 14, 2020: none

    In scalar_4x64_impl.h, function secp256k1_scalar_reduce_512. The first asm block is missing early clobber for m0~m2.

    The input %rsi is used after writing to m0, m1 and m2. Any of those write can thus re-use %rsi (which will most likely result in segfault).

    Also, field_5x52_asm_impl.h is missing similar early clobbers for tmp1~tmp3

  2. real-or-random commented at 10:53 PM on July 14, 2020: contributor

    My understanding of the GCC ASM extensions is very limited but I think you're right. Would you be willing to open a PR?

  3. ehoffman2 commented at 2:30 AM on July 15, 2020: none

    Actually, I was not using the code as is, so I don't have the setup to test the issue in the current code itself. I actually found this bug because I was re-using code from the library in a small home project, where I have field elements not using nails (packed in 32 bytes, like the scalars). I do this in my project because I mainly want to use the GPU to do some computations and it help using packed binary representation (I use secp256k1 code mainly as a 'helper', the bulk of the computation will be GPU).

    So, I had to modify the field code to look like the scalar code, but using modulo P instead of modulo N. Most code mainly just involved using P instead of N as a constant, but I had to re-write the asm for the 512-bit reduction (much simpler as you only have one 64-bit single-precision multiplication). However, at some point, I hit the issue with the clobber list as it was defined in scalar code (segfault). Looking at asm output, I saw that the compiler did put the 'extract to m2' to %rsi, and since %rsi was used as indexed memory source for the next instruction, this caused segfault. Just defining m0~m2 output list as early-clobber (since the %rsi input is not 'consumed' before those outputs are set) fixed the issue.

  4. real-or-random commented at 2:34 PM on July 15, 2020: contributor

    Actually, I was not using the code as is, so I don't have the setup to test the issue in the current code itself.

    Well, neither do I (or anyone else here). We haven't seen bugs due to this but I guess then we were just lucky that compilers got it right arbitrarily.

    I do this in my project because I mainly want to use the GPU to do some computations and it help using packed binary representation (I use secp256k1 code mainly as a 'helper', the bulk of the computation will be GPU).

    Can I ask what your project is about? I'm just curious, it's not related to this issue.

  5. sipa referenced this in commit c80d79d94e on May 12, 2023
  6. sipa referenced this in commit 0c729ba70d on May 12, 2023
  7. jonasnick closed this on May 12, 2023

  8. real-or-random referenced this in commit 4e362c628e on May 12, 2023
  9. jonasnick referenced this in commit 56a5d41429 on May 14, 2023
  10. dderjoel referenced this in commit 258b7dd652 on May 23, 2023
  11. dderjoel referenced this in commit ae4d0bcd14 on May 23, 2023
  12. dderjoel referenced this in commit f5151a35b6 on Jun 21, 2023
  13. Fabcien referenced this in commit 566ac8d9eb on Apr 28, 2026
  14. Fabcien referenced this in commit b930c91e15 on Apr 28, 2026

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:52 UTC