This should ensure that kernel spectre mitigations are enabled if available when the wallet is enabled and disabled if possible when there is no wallet for improved performance.
I also added a -mitigatespectre config switch to override the default behavior.
I'm not entirely sure if this specific spectre mitigation should be automatically enabled but it appears the prctl interface will be used for newer more important process level spectre mitigations as well.
900 | + SpectreMitigation(true); 901 | + } else { 902 | + SpectreMitigation(false); 903 | + } 904 | + } 905 | +
You could do
bool mitigatespectre_default = !gArgs.GetBoolArg("-disablewallet", false) && DEFAULT_MITIGATE_SPECTE;
SpectreMitigation(gArgs.GetBoolArg("-mitigatespectre", mitigatespectre_default));
yeah, that's def much cleaner
66 | @@ -67,6 +67,31 @@ 67 | 68 | #ifdef HAVE_SYS_PRCTL_H 69 | #include <sys/prctl.h> 70 | +// Set spectre prctl defines so that we can build on older kernels
This doesn't seem safe. What if a kernel has number 52 with a different meaning?
AFAIK the linux kernel API is supposed to be stable for these types of calls so that should not change.
Should make it clear that this is for linux only. Are there other operating systems with PRCTL?
I think some emulate the interface.
IRIX has its own prctl(), and there's no guarantee someone won't fork Linux incompatibly either - it's open source, after all.
Anyone building from source should have recent enough headers to match their actual kernel anyway. Let's just make sure the gitian builds have it working, and rely on the system for source builds...?
Irix is an abandoned platform, I think the only other non-linux OS's that have prctl are BSD's but they seem to be just for compatibility with linux so their prctl numbers should always match. Regardless as long as those values are present in the kernel header the ones I hard coded should not get used. I didn't want to have it build this feature based on the presence of the defines in prctl.h since that would complicate the testing and logic since some of the options may not be present. For example I think PR_SPEC_FORCE_DISABLE was added after PR_SPEC_DISABLE.
If linux were to change the prctl number for these it would break on binary builds regardless, but that sounds quite unlikely with the current linux userspace API stability policy.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
368 | @@ -369,6 +369,7 @@ void SetupServerArgs() 369 | "(default: 0 = disable pruning blocks, 1 = allow manual pruning via RPC, >=%u = automatically prune block files to stay under the specified target size in MiB)", MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024), false, OptionsCategory::OPTIONS); 370 | gArgs.AddArg("-reindex", "Rebuild chain state and block index from the blk*.dat files on disk", false, OptionsCategory::OPTIONS); 371 | gArgs.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks. When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead.", false, OptionsCategory::OPTIONS); 372 | + gArgs.AddArg("-mitigatespectre", strprintf("Attempt to enable per process spectre mitigations (default: %u, defaults to disabled if -disablewallet is set)", DEFAULT_MITIGATE_SPECTRE), true, OptionsCategory::OPTIONS);
The default-exception language here doesn't match elsewhere.
Maybe something like:
gArgs.AddArg("-mitigatespectre", strprintf("Attempt to enable per process spectre mitigations (default: %u%s)", DEFAULT_MITIGATE_SPECTRE, (
#ifdef ENABLE_WALLET
DEFAULT_MITIGATE_SPECTRE ? "0 or 1 if -disablewallet set" : "0"
#else
strprintf("%u", DEFAULT_MITIGATE_SPECTRE)
#endif
)), true, OptionsCategory::OPTIONS);
Alphabetic ordering seems to happen here, so move this entry up to below -minimumchainwork
357 | @@ -358,6 +358,13 @@ void SetupServerArgs() 358 | gArgs.AddArg("-minimumchainwork=<hex>", strprintf("Minimum work assumed to exist on a valid chain in hex (default: %s, testnet: %s)", defaultChainParams->GetConsensus().nMinimumChainWork.GetHex(), testnetChainParams->GetConsensus().nMinimumChainWork.GetHex()), true, OptionsCategory::OPTIONS); 359 | gArgs.AddArg("-par=<n>", strprintf("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)", 360 | -GetNumCores(), MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS), false, OptionsCategory::OPTIONS); 361 | + gArgs.AddArg("-mitigatespectre", strprintf("Attempt to enable per process spectre mitigations (default: %u%s)", DEFAULT_MITIGATE_SPECTRE, ( 362 | +#ifdef ENABLE_WALLET 363 | + DEFAULT_MITIGATE_SPECTRE ? " or 0 if -disablewallet set" : "0"
This will turn into 00 if !DEFAULT_MITIGATE_SPECTE, I think? Probably want end to go : "".
debug help shows with wallet enabled:
-mitigatespectre
Attempt to enable per process spectre mitigations (default: 1 or 0 if
-disablewallet set)
If you set DEFAULT_MITIGATE_SPECTRE to false it will say 00 I think.
yeah, it only works because DEFAULT_MITIGATE_SPECTRE is controlled by ENABLE_WALLET
I don't think it should rely on that.
And it does give the following output for -disable-wallet (at configure level) so it's broken regardless:
-mitigatespectre
Attempt to enable per process spectre mitigations (default: 00)
Perhaps
gArgs.AddArg("-mitigatespectre", strprintf("Attempt to enable per process spectre mitigations (default: %s)", (
#ifdef ENABLE_WALLET
DEFAULT_MITIGATE_SPECTRE ? "1 or 0 if -disablewallet set" : "0"
#else
std::to_string(DEFAULT_MITIGATE_SPECTRE)
#endif
357 | @@ -358,6 +358,13 @@ void SetupServerArgs() 358 | gArgs.AddArg("-minimumchainwork=<hex>", strprintf("Minimum work assumed to exist on a valid chain in hex (default: %s, testnet: %s)", defaultChainParams->GetConsensus().nMinimumChainWork.GetHex(), testnetChainParams->GetConsensus().nMinimumChainWork.GetHex()), true, OptionsCategory::OPTIONS); 359 | gArgs.AddArg("-par=<n>", strprintf("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)", 360 | -GetNumCores(), MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS), false, OptionsCategory::OPTIONS); 361 | + gArgs.AddArg("-mitigatespectre", strprintf("Attempt to enable per process spectre mitigations (default: %u%s)", DEFAULT_MITIGATE_SPECTRE, ( 362 | +#ifdef ENABLE_WALLET 363 | + DEFAULT_MITIGATE_SPECTRE ? " or 0 if -disablewallet set" : "0" 364 | +#else 365 | + strprintf("%u", DEFAULT_MITIGATE_SPECTRE)
Am I misreading this code completely? This looks like it will print out DEFAULT_MITIGATE_SPECTRE twice.
DEFAULT_MITIGATE_SPECTRE is somewhat redundant in DEFAULT_MITIGATE_SPECTRE ? " or 0 if -disablewallet set" : "0" since DEFAULT_MITIGATE_SPECTRE is controlled by ENABLE_WALLET, doesn't look like it would be printed twice.
The first 0 comes from %u in the initial format line and DEFAULT_MITIGATE_SPECTRE. The second 0 comes from
strprintf("%u", DEFAULT_MITIGATE_SPECTRE);
in the else case on line 365.
ah, removed that line now
1210 | @@ -1186,6 +1211,44 @@ void RenameThread(const char* name) 1211 | #endif 1212 | } 1213 | 1214 | +void SpectreMitigation(bool enable) 1215 | +{ 1216 | +#if defined(HAVE_SYS_PRCTL_H)
Please add a common prefix to these log messages to be able to quickly grep for it. E.g. SpectreMitigation: using %s: and __func__. It also makes it seem less to come out of the blue for users.
39 | @@ -40,6 +40,11 @@ int64_t GetStartupTime(); 40 | 41 | extern const char * const BITCOIN_CONF_FILENAME; 42 | extern const char * const BITCOIN_PID_FILENAME; 43 | +#ifdef ENABLE_WALLET
Please don't add #ifdef ENABLE_WALLET in the utility code! We've only recently painstakingly removed #ifdef ENABLE_WALLET stuff from the server library, for example.
Also I think it probably should be switched based on the run-time -disablewallet flag as well?
Or maybe: always enable it, don't make it depend on the wallet at all? What would be the drawback to that?
Should I move that to somewhere else?
What is the performance slowdown in practice? I think this needs benchmarking, if it's neglible over say, a full sync, I don't think it makes sense to do this kind of switching at all.
Perhaps this should be a ParameterInteraction thing using SoftSetArg in the wallet logic?
357 | @@ -358,6 +358,11 @@ void SetupServerArgs() 358 | gArgs.AddArg("-minimumchainwork=<hex>", strprintf("Minimum work assumed to exist on a valid chain in hex (default: %s, testnet: %s)", defaultChainParams->GetConsensus().nMinimumChainWork.GetHex(), testnetChainParams->GetConsensus().nMinimumChainWork.GetHex()), true, OptionsCategory::OPTIONS); 359 | gArgs.AddArg("-par=<n>", strprintf("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)", 360 | -GetNumCores(), MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS), false, OptionsCategory::OPTIONS); 361 | + gArgs.AddArg("-mitigatespectre", strprintf("Attempt to enable per process spectre mitigations (default: %u%s)", DEFAULT_MITIGATE_SPECTRE, (
Might want to add that this works around a CPU vulnerability, for users that don't know what spectre is.
Hmm, I wonder if it should be future-proofed by making it applicable to any other vulns we could mitigate later.
I think there will soon be multiple spectre related prctl controls such as this.
Concept ACK
Concept ACK.
I'm not sure this should be switched on ENABLE_WALLET. Knowing nothing else I'd say we should enable this always when possible (and not disabled using the flag).
Taking a step back from the "should we switch this based on wallet" or "should we take the (unknown) performance hit for the whole process". I think it's important here to define what the threat scenario is that this is protecting against. This also needs to be documented to the user.
As I understand it, Spectre mitigation is especially meant for browsers and such and other software that runs untrusted code in a sandbox. Code running in a sandbox could use spectre to learn things and gain access that it's not supposed to have.
I don't think this is the case for bitcoind at all—yes, very pedantically the consensus scripts could be seen as that, but I would be extremely surprised if there's any chance of that with the lack of loops and such. So, then, what is left?
Spectre can allow unrelated processes to get at private data of other processes. Whether these specific mitigations help with that scenario or not, however, I am unsure.
We shouldn't take this by default without characterizing the performance implications, esp if we're not precisely sure about what it's protecting against. If the performance impact is negligible then perhaps we can ignore the minutia of what it's protecting.
Lack of comprehensive mlock (e.g. rpc/QT result in decryption keys laying around in memory) probably and the lack of process isolation for secret keys are probably much more serious limitations to our current behaviour than anything spectre... but also can't just be improved with a ctl. OTOH, fixing them doesn't have a risk of causing a large sync performance loss that push people onto far less secure software.
Spectre can allow unrelated processes to get at private data of other processes. Whether these specific mitigations help with that scenario or not, however, I am unsure.
I think we need to be sure about that, as far as possible.
I have a fairly good grasp of how vulnerabilities and exploitation work in general, but this Spectre stuff is abracadabra to me. I can't imagine how a PoC exploit for this would look.
If we're unsure if and how this will help, just flipping some process flags seems security theater, or a charm to keep the evil hacker spirits away.
It's probably easier to tell if it has a meaningfully negative performance impact... understanding could be replaced with emailing the author(s) of the kernel functionality and asking if they think it would be useful for us.
@gmaxwell I sent an email to the authors of the spectre prctl kernel patch asking them about how it should be used.
<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase
I'm going to close this. Discussion seems to have stalled. It's not entirely clear when or how the mitigations should be applied. There would also need to be some effort put into figuring out the performance implications.
I have a fairly good grasp of how vulnerabilities and exploitation work in general, but this Spectre stuff is abracadabra to me. I can't imagine how a PoC exploit for this would look.
I agree with laanwj in that I'd be interested in seeing some sort of proof of concept attack for bitcoind.
@jameshilliard did the authors of the spectre prctl patch ever get back to your with more information?