Revert new black_box optimization barrier #107
Conversation
This reverts commit 6410953.
|
There is also an alternative solution: Back when I proposed #54, assembly directives were still unstable, requiring a nightly rust compiler. However, assembly directives are stable now on multiple platforms [https://doc.rust-lang.org/reference/inline-assembly.html]. As far as I know, this is still a robust method to break any optimizations on the values passing through the assembly directive, and this is also what I suppose we can now also replace the Alternatively, we can leave the code like this, but we start periodically running a test on every new nightly compiler version to see if |
|
There's a PR to I'd suggest gating something like that under an The other concern there is ASM is only stable for x86/x86_64, ARM, and RISC-V and therefore needs to be gated appropriately with fallback to the previous implementation for other target CPU architectures. |
There was a problem hiding this comment.
Thanks @dsprenkels!
Originally the black_box function(s, there was an experiment at creating a handwritten wat/wasm version) was written in inline assembly, just a volatile NOP to convince LLVM to do nothing to the input. I wonder if maybe now that asm! is more stabilised, if we might want to opportunistically return to doing that on architectures which support it?
| cargo test --no-default-features --features "std const-generics" && | ||
| cargo test --no-default-features --features "std i128 core_hint_black_box" && | ||
| cargo test --no-default-features --features "std i128 core_hint_black_box const-generics" | ||
| cargo test --no-default-features --features "std i128 const-generics" |
There was a problem hiding this comment.
Oh also I guess one thing since we added the const-generics feature it wasn't being tested in the github actions workflow, so I'll add that back in.
That's one option. Another is to use predication instructions like CMOV. We've been working on that in the @RustCrypto The pattern can be used to implement e.g. We've discussed opening a PR to allow Using CMOV/CSEL inline assembly helps guarantee that not only will LLVM not rewrite the code, but that the resulting code will be executed by the CPU in constant-time. Edit: I guess I already opened an issue for this: #101 |
We currently use the `core_hint_black_box` feature from `subtle`, which uses a particular [optimization barrier](https://github.com/dalek-cryptography/subtle/blob/6b6a81ad9a6a00c0b42c327eaf4b2f785774377e/src/lib.rs#L245-L250). However, the standard library [documentation](https://doc.rust-lang.org/std/hint/fn.black_box.html) cautions against the use of `std::hint::black_box` for cryptographic use. This has led `subtle` to [remove it](dalek-cryptography/subtle#107) in an upcoming release, at which point the feature will do nothing. This PR takes the proactive step of removing the feature. There is still an [optimization barrier](https://github.com/dalek-cryptography/subtle/blob/6b6a81ad9a6a00c0b42c327eaf4b2f785774377e/src/lib.rs#L227-L243) in place, which will become the default after the feature is deprecated.
Description --- Removes the `core_hint_black_box` feature from `subtle`. Motivation and Context --- We currently use the `core_hint_black_box` feature from `subtle`, which uses a particular [optimization barrier](https://github.com/dalek-cryptography/subtle/blob/6b6a81ad9a6a00c0b42c327eaf4b2f785774377e/src/lib.rs#L245-L250). However, the standard library [documentation](https://doc.rust-lang.org/std/hint/fn.black_box.html) cautions against the use of `std::hint::black_box` for cryptographic use. This has led `subtle` to [remove it](dalek-cryptography/subtle#107) in an upcoming release, at which point the feature will do nothing. This PR takes the proactive step of removing the feature. There is still an [optimization barrier](https://github.com/dalek-cryptography/subtle/blob/6b6a81ad9a6a00c0b42c327eaf4b2f785774377e/src/lib.rs#L227-L243) in place, which will become the default after the feature is deprecated. How Has This Been Tested? --- Existing tests pass. What process can a PR reviewer use to test or verify this change? --- Check the claims made in the PR about the feature behavior.
In #96 (comment), @tarcieri noticed that recently the rustdoc documentation has added a big notice that
core::hint::black_boxshould not be relied upon for correctness, especially for security or cryptographic purposes. It seems wise to revert that change. :(Review suggestions: