Fix CPUID subleaf iteration

sipa commented at 11:00 pm on November 19, 2019: member

This fixes #17523.

The code to determine which CPUID subleaves to explore was incorrect in #17270. The new code here is based on Intel’s reference documentation for CPUID (a document called “Intel® Processor Identification and the CPUID Instruction - Application Note 485”, which I cannot actually find on their own website).

Fix CPUID subleaf iteration ba2c5fe147

fanquake added the label Utils/log/libs on Nov 19, 2019

mb300sd commented at 11:12 pm on November 19, 2019: contributor

Tested. Changes fixed the issue for me.

sipa added the label Bug on Nov 19, 2019

fanquake commented at 11:21 pm on November 19, 2019: member

Intel® Processor Identification and the CPUID Instruction - Application Note 485

in src/randomenv.cpp:202 in ba2c5fe147 outdated

197@@ -198,12 +198,23 @@ void AddAllCPUID(CSHA512& hasher)
198     AddCPUID(hasher, 0, 0, ax, bx, cx, dx); // Returns max leaf in ax
199     uint32_t max = ax;
200     for (uint32_t leaf = 1; leaf <= max; ++leaf) {
201+        uint32_t maxsub = 0;
202         for (uint32_t subleaf = 0;; ++subleaf) {

mzumsande commented at 0:08 am on November 20, 2019:

I am not an expert on CPUID, but how about an emergency break after some number of subleaves - or was it a deliberate decision to construct the inner loop in a way that it can possibly run infinitely when certain assumptions are wrong, or might be in the future?

sipa commented at 0:21 am on November 20, 2019:

I guess the question is whether we want to have any chance of catching weird systems that return non-compliant CPUID values. If we add an upper bound, we’ll probably never ever see a bug report like this one.

mzumsande commented at 0:59 am on November 20, 2019:

True, I am not sure if catching those justifies the inconvenience for affected users who might not be able to run bitcoin on their system without waiting for a patch (considering that CPUID is just one of many static entropy sources).

laanwj commented at 8:22 am on November 20, 2019:

I think this depends on the chance that there actually CPUs out there, suitable for running nodes, that are not compliant with this.

I feel slightly uneasy with potentially infinite loops over hardware registers like this, OTOH I understand @sipa’s point.

I guess the question is whether we want to have any chance of catching weird systems that return non-compliant CPUID values.

It could log a message when it hits the upper bound. And if you make it a high enough value you’d still get reports, just that bitcoind startup is slow instead of doesn’t start at all. (which is probably preferable :smile:)

laanwj commented at 8:23 am on November 20, 2019:

It’s only a uint32_t. How long does a full wrap-around take?

sipa commented at 6:56 pm on November 20, 2019:

@laanwj A full 32-bit wrap-around would mean SHA512 hashing 96 GiB of data. Too long, I’m sure.

I’ve added a commit to limit the number of iterations.

jonatack commented at 8:09 am on November 20, 2019: member

Tested ba2c5fe147 by building, running tests and bitcoind. I was unaffected by the issue but running bitcoind appears unaffected by this change.

0$ uname -a
1Linux purity 4.19.0-5-amd64 [#1](/bitcoin-bitcoin/1/) SMP Debian 4.19.37-5+deb10u2 (2019-08-08) x86_64 GNU/Linux

laanwj commented at 8:27 am on November 20, 2019: member

Concept ACK. Thanks @mb300sd for catching this before a release.

fanquake added this to the milestone 0.20.0 on Nov 20, 2019

Put bounds on the number of CPUID leaves explored f93fc61c65

laanwj commented at 3:53 pm on November 22, 2019: member

ACK f93fc61c65d605eae2d3e2c98bdd30ae587fcdab

jonatack commented at 4:47 pm on November 22, 2019: member

ACK f93fc61c65d605eae2d3e2c98bdd30ae587fcdab code review, tested rebased on current master bb862d7 with Debian 4.19 x86_64

in src/randomenv.cpp:211 in ba2c5fe147 outdated

210+                if ((ax & 0x1f) == 0) break;
211+            } else if (leaf == 7) {
212+                if (subleaf == 0) maxsub = ax;
213+                if (subleaf == maxsub) break;
214+            } else if (leaf == 11) {
215+                if ((cx & 0xff00) == 0) break;

mzumsande commented at 9:55 pm on November 23, 2019:

Got slightly confused here because the intel pdf linked above recommends eax=0 and ebx=0 as break condition for leaf 11 (p.40), but checking cx & 0xff00 should be equivalent according to this.

mzumsande commented at 10:04 pm on November 23, 2019: member

ACK f93fc61, reviewed code and compared with the intel doc, tested on an AMD and an Intel processor.

laanwj referenced this in commit 2eeacdfe44 on Nov 24, 2019

laanwj merged this on Nov 24, 2019

laanwj closed this on Nov 24, 2019

sidhujag referenced this in commit 7f69c42e6f on Nov 24, 2019

deadalnix referenced this in commit bef4ef2954 on May 26, 2020

sidhujag referenced this in commit c14e5d9cf9 on Nov 10, 2020

random-zebra referenced this in commit 93f43f0f81 on Apr 14, 2021

DrahtBot locked this on Dec 16, 2021

Fix CPUID subleaf iteration #17527