Use rdrand as entropy source on supported platforms #10377

pull sipa wants to merge 1 commits into bitcoin:master from sipa:hwrand changing 4 files +73 −0
  1. sipa commented at 10:14 pm on May 9, 2017: member
  2. sipa force-pushed on May 9, 2017
  3. in src/random.cpp:71 in 0cd9249c5b outdated 
    64@@ -65,6 +65,34 @@ static inline int64_t GetPerformanceCounter()
65 #endif
66 }
67 
68+#if !defined(_MSC_VER) && (defined(__x86_64__) || defined(__amd64__))
69+static bool RDRandSupported()
    theuni commented at 10:32 pm on May 9, 2017:

    I think it would be useful to have a generic cpuid util function for this, for easier support for (among other things) sha extensions in the future.

    We can always break it out later, though.

  4. in src/random.cpp:81 in 0cd9249c5b outdated 
    70+{
71+    uint32_t ax,bx,cx,dx,func=1;
72+    __asm__ volatile ("cpuid": "=a" (ax), "=b" (bx), "=c" (cx), "=d" (dx) : "a" (func));
73+    return (cx & 0x40000000);
74+}
75+static const bool fRDRand = RDRandSupported();
    theuni commented at 10:41 pm on May 9, 2017:
    Any reason not to stick this inside GetHWRand()?
    sipa commented at 10:52 pm on May 9, 2017:
    I believe that C++11 specifies something about the order of initialization of statics defined inside functions, which results in automatically emitted synchronization calls. Perhaps that’s not the case for const variables, though.
  5. sipa force-pushed on May 9, 2017
  6. sipa force-pushed on May 10, 2017
  7. fanquake added the label Utils and libraries on May 10, 2017
  8. in src/random.cpp:73 in a8cd5996a9 outdated 
    64@@ -65,6 +65,40 @@ static inline int64_t GetPerformanceCounter()
65 #endif
66 }
67 
68+#if !defined(_MSC_VER) && (defined(__x86_64__) || defined(__amd64__))
69+static bool RDRandSupported()
70+{
71+    uint32_t ax,bx,cx,dx,func=1;
72+    __asm__ volatile ("cpuid": "=a" (ax), "=b" (bx), "=c" (cx), "=d" (dx) : "a" (func));
73+    return (cx & 0x40000000);
    ryanofsky commented at 7:24 pm on May 10, 2017:
    Maybe note the name of the flag (rdrnd) in a comment.
  9. ryanofsky commented at 7:37 pm on May 10, 2017: member
    utACK a8cd5996a941cd0d9a4a315180dc041a0ad09f34
  10. sipa force-pushed on May 24, 2017
  11. sipa commented at 1:07 am on May 24, 2017: member
    Updated.
  12. ryanofsky commented at 9:17 pm on June 1, 2017: member
    utACK 7864abb0673e22e5d7c2a2f5ede6c142a09b8890. Changes since previous review were disabling for msvc, shuffling around some constants & local variables in RDRandSupported.
  13. gmaxwell approved
  14. gmaxwell commented at 10:03 am on June 6, 2017: contributor
    utACK
  15. gmaxwell commented at 10:04 am on June 6, 2017: contributor
    You might want to make some log entry note if fRDRand is set.
  16. laanwj commented at 10:04 am on June 6, 2017: member

    I have no hardware that supports this instruction, but the code changes look good to me. The rdrand output is only ever added to the state, through a cryptographic hash function, so there is no (realistic) scope for bad rdrand implementations to interfere with strong random number generation.

    Tested (that it doesn’t crash on CPUs without the instruction) ACK 7864abb

  17. laanwj commented at 10:10 am on June 6, 2017: member

    You might want to make some log entry note if fRDRand is set.

    Yes good idea - though this message should only be emitted once, and this cannot be done at the time fRDRand is initialized as this is before the initialization sequence.

  18. sipa force-pushed on Jun 6, 2017
  19. sipa commented at 7:38 pm on June 6, 2017: member

    Done, it will now print line like

    02017-06-06 19:31:12 Using RdRand as entropy source

    It’s a bit ugly as it gets printed before the version line.

  20. jonasschnelli commented at 7:47 pm on June 6, 2017: contributor
    Just my five cents: During discussions if RdRand should be added as entropy source for /dev/random on linux possible backdoor-entropy-control attacked where demonstrated. Not sure if this apply here: https://pastebin.com/A07q3nL3
  21. sipa commented at 7:52 pm on June 6, 2017: member
    @jonasschnelli That attack doesn’t apply here, we don’t XOR the RdRand output into the buffer; we combine it with the former state using SHA512.
  22. jonasschnelli commented at 2:11 pm on June 7, 2017: contributor

    Tested a bit. The log order seems a bit strange: **EDIT: ** saw that @sipa mentioned that already.

     02017-06-07 14:08:35 Shutdown: done
 12017-06-07 14:08:54 Using RdRand as entropy source
 22017-06-07 14:08:54 
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
222017-06-07 14:08:54 Bitcoin version v0.14.99.0-7dd5ec4
232017-06-07 14:08:54 InitParameterInteraction: parameter interaction: -whitelistforcerelay=1 -> setting -whitelistrelay=1
242017-06-07 14:08:54 Assuming ancestors of block 00000000000000000013176bf8d7dfeab4e1db31dc93bc311b436e82ab226b90 have valid signatures.
  23. TheBlueMatt commented at 4:04 pm on June 7, 2017: member

    Yea, can we move the CPUID check to a static check in GetHWRand (or via init)? Then we wouldnt LogPrintf pre-main().

    Also, would be nice to support i686 as well, since I believe we still ship binaries for that.

  24. in src/random.cpp:318 in fb854860cc outdated 
    294@@ -255,6 +295,11 @@ void GetStrongRandBytes(unsigned char* out, int num)
295     GetOSRand(buf);
296     hasher.Write(buf, 32);
297 
298+    // Third source: HW RNG, if available.
299+    if (GetHWRand(buf)) {
300+        hasher.Write(buf, 32);
    jonasschnelli commented at 6:13 am on June 8, 2017:
    I guess logging (Using RdRand as entropy source) at this point would make more sense.
    laanwj commented at 12:51 pm on June 14, 2017:
    the logging is now in void RDRandInit(), which is a better place, so I suppose this comment is outdated
  25. jonasschnelli commented at 6:15 am on June 8, 2017: contributor
    Testet ACK on Xeon E3-1275 (Skylake) fb854860ccbed5d8513a6251e6caeda250fde10a.
  26. sipa commented at 7:30 am on June 8, 2017: member
    @jonasschnelli We’re not going to log a line every time a random number is generated.
  27. jonasschnelli commented at 7:38 am on June 8, 2017: contributor

    @jonasschnelli We’re not going to log a line every time a random number is generated. Oh. Indeed. I stupidly had the assumption that this was only called once.

  28. sipa force-pushed on Jun 9, 2017
  29. sipa commented at 9:17 pm on June 9, 2017: member
    Rewritten using an initialization function, and added support for 32-bit x86 (which works on my i7 CPU).
  30. sipa force-pushed on Jun 9, 2017
  31. theuni commented at 7:27 pm on June 13, 2017: member

    @sipa Would you be opposed to adding a static bool g_random_init_done = false;, set by RDRandInit(), and assert it in GetHWRand()?

    I’m nervous that some future tool will forget to call RDRandInit() at startup.

    It would also obviate any startup order issues, in case we’re somehow using strong random numbers during init without realizing.

  32. sipa commented at 7:29 pm on June 13, 2017: member
    @theuni That would need to be atomic, as GetHWRand() may be called concurrently :(
  33. theuni commented at 7:49 pm on June 13, 2017: member
    I’m not sure how its access would be different from rdrand_supported?
  34. sipa commented at 7:51 pm on June 13, 2017: member

    @theuni rdrand_supported is only ever written before any read access to the variable (under the assumption that RDRandInit() is called before any GetHWRand()).

    Reading g_random_init_done while perhaps GetHWRand() is being called concurrently (a case it is trying to detect) would be undefined behaviour.

  35. Use rdrand as entropy source on supported platforms cb24c8539d
  36. sipa force-pushed on Jun 14, 2017
  37. sipa commented at 0:02 am on June 14, 2017: member
    @theuni I’ve added your suggestion as a relaxed atomic read.
  38. laanwj commented at 1:15 pm on June 14, 2017: member
    re-utACK cb24c85
  39. laanwj merged this on Jun 14, 2017
  40. laanwj closed this on Jun 14, 2017
  41. laanwj referenced this in commit b63be2c685 on Jun 14, 2017
  42. theuni commented at 6:38 pm on June 14, 2017: member
    post-merge utACK (excluding the asm bits that I’m not qualified to review). @sipa thanks. If the cost of that check is problematic, we could reduce it to an atomic_flag instead, and assert that it’s already set in GetHWRand. I assume it’s not worth bothering, though.
  43. sipa deleted the branch on Jun 23, 2017
  44. PastaPastaPasta referenced this in commit c235fd2877 on Jul 5, 2019
  45. PastaPastaPasta referenced this in commit 8d5e071327 on Jul 5, 2019
  46. PastaPastaPasta referenced this in commit 9d80fabe1f on Jul 6, 2019
  47. PastaPastaPasta referenced this in commit 603eb2940b on Jul 8, 2019
  48. PastaPastaPasta referenced this in commit eb188e6981 on Jul 9, 2019
  49. PastaPastaPasta referenced this in commit 0cb552a20b on Jul 9, 2019
  50. barrystyle referenced this in commit 0fdfad1ea3 on Jan 22, 2020
  51. DrahtBot locked this on Sep 8, 2021


sipa theuni ryanofsky gmaxwell laanwj jonasschnelli TheBlueMatt

Labels
Utils/log/libs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-04 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me