Implement SHA256 SIMD intrinsics on x86 #3752
Conversation
|
How do I properly convey that I'm sure side effects don't happen? Just |
|
Thanks for the PR! There are some other PRs we have to review before we get to this one, so unfortunately it may take a bit -- sorry for that. From a quick glance, it seems like this PR actually implements sha256 in Miri. Ideally I'd like to avoid being responsible for cryptographic code. Is there a small crate, akin to If we do carry a sha256 implementation, I would prefer one that was not written by ChatGPT, but by a human that actually understands sha256. Maybe there's a larger Rust crypto crate that we can copy an implementation from. |
|
@RalfJung there is the bitcoin_hashes crate which you're welcome to steal code from (it is CC0 licensed) which is written by professional human cryptographers. You probably don't want to depend on it because it does a bunch of other stuff (and you'll get complaints about having There is also the sha2 crate written by a different set of professional human cryptographers, which is MIT/Apache licensed. And you could use it as a direct dependency (there is a good chance it is already in your dep tree somewhere) since it's narrowly focused and maintained. Though again, it does more than SHA256 which is really the only thing you want. |
|
I'll also add, that while your aversion to carrying/maintaining crypto code is well-placed, hashes are a low-burden thing to maintain for a few reasons. By their nature you don't need to worry about constant-timeness so you can write them once and forget about the code forever (especially in something like Miri where you aren't going to want to go microoptimizing this). And also by their nature it's super hard to have subtle correctness bugs; pretty-much any bug will result in every single output being visibly totally wrong. Finally hash functions are pretty small self-contained pure algorithms. |
|
Thanks, those are helpful comments! |
No problem! There's no rush, we can run my fork for now.
Not really, just the three primitives that need to be used together with a bunch of other code to actually implement sha256. I also suspect it's not trivial to just pull out these from existing impls - at least I tried and the code looked so different that I couldn't do it. That's why I gave up and translated the Intel's psueudocode which literally describe how the specific instructions work.
Isn't Anyway, everything @apoelstra said is true. The code is constant-time and any single tiny mistake would just completely break it - as I experienced this first hand. It also happens to make debugging the code incredibly hard. (I had to put For reference this is what the actual sha256 implementation using SIMD looks like: https://github.com/rust-bitcoin/rust-bitcoin/blob/8804fa63b49dc5ee639e768b73650b46a5e0a8ab/hashes/src/sha256.rs#L494-L722 |
So the sha2 crate wouldn't be useful? That seems like the closest thing.
Yes, but that doesn't change the fact that Miri has a small set of maintainers, and we can't just ship code we all have no clue about -- that's not living up to our responsibility as maintainers. It's not about performance, it's about the fact that I can't check whether your code makes any sense and I don't have the time to become an expert in every problem domain that is covered by an Intel intrinsic. "Code was translated by Chat GPT" also raises some very big alarm bells, since it seems to indicate that you also didn't check whether the code makes any sense. At the very least I would expect a link to the Intel docs, and a statement by you as PR author that you are sure that the Chat GPT translation is correct. Someone from the Miri team will have to double-check this before we can land this (if we can't find an alternative to carrying this code ourselves), but it's not reasonable to ask us to be the first humans to check this. And of course the PR needs to come with some tests. |
|
We probably could handle emulation of cryptographic intrinsics on the RustCrypto side. For example, we have a similar request for AES: RustCrypto/block-ciphers#389 But, personally, I would prefer to do it in separate crates, since having an emulation of SHA2 hardware intrinsics in the @tarcieri WDYT? |
Looking at its code it does actually have the same primitives inside so copying them would be arguably better but they still need a bit of wiring code - the same there is at the beginning that just shuffles values in arrays.
OK, fair. How can we solve it then? The code should exactly (bug-for-bug) emulate the Intel's intrinsic and the intrinsic has documented pseudo code implementation should just comparing the implementation be enough? Or is there anything else I can do?
I did test it by running If this is not sufficient please suggest specific steps I could do and I'll do it.
That's absolutely something I wouldn't make you do, I'm sorry if it looked like I did. That's what I meant by "this code works". The disclaimer is about the style of the code. The things I'm unsure about is the
Of course, I first wanted to see if my approach makes sense so that I wouldn't need to refactor the test (running SHA256 is the ultimate test). I'm also not sure what kind of testing strategy would be most appropriate. I kinda wish to just run the real instructions on real hardware with random inputs and compare the results but I don't know if this is a viable solution. Or just bake in the real SHA256 and run it on a few inputs. Of course I can also grab a bunch of random vectors and bake them in. |
Sounds like a good idea.
Manually comparing Intel's pseudocode with the Miri code would be a way to get additional confidence on top of the testing.
We don't usually load things into arrays on the host side, we just process them element-by-element, but here that doesn't seem to make sense so what you did looks reasonable.
We should definitely have some test vectors directly checking the intrinsic, yes. If there are any corner cases, they should be covered here. How big is a (not performance-oriented) sha256 implementation on top of these intrinsics? If that's small enough to put in a test, then we can also have that in addition. |
|
OK, I'll look into it later, I have currently some urgent things to do. |
|
@rustbot author Regarding the decision about how to implement these intrinsics; @rust-lang/miri see the discussion above -- what do you think? |
rustbot
added
the
S-waiting-on-author
Kixunil
force-pushed
the
simd-sha256
branch
3 times, most recently
from
de228a3 to
9f737d0
Compare
|
@rustbot ready Funny how you've looked at it at around the same time I did. :) I've pushed an updated version that copies stuff from RustCrypto. I have addressed all your review comments including adding tests. The tests are somewhat large so LMK if I should remove anything. |
rustbot
added
S-waiting-on-review
|
Thanks! Regarding the tests, it seems I should have been more clear -- what I meant is tests in |
|
I felt like something is off... Anyway, it's moved now. |
I am similarly opposed to checking in any code that comes from ChatGPT. In addition to the issues with its accuracy and our ability to read the code we are checking in, it seems like the project has no regard for the licensing of its training set. As to reviewing the implementation, I also cannot vouch for its correctness. I might be able to, given some time to learn SHA256. That being said, I think there's a pretty low chance that the implementation here is subtly broken if it passes a few tests. The implementation has no branches and I don't see any edge cases, so just a handful of test cases should provide full coverage. |
This is no longer relevant, the implementation now copies
Since it's a copy of
It's pretty much impossible to write an incorrect sha256 since any mistake will produce a completely different output. It's the same property as for the input. |
src/shims/x86/sha.rs
Outdated
| add(v0, sigma0x4(sha256load(v0, v1))) | ||
| } | ||
|
|
||
| fn sha256msg2_epu32(v4: [u32; 4], v3: [u32; 4]) -> [u32; 4] { |
It'd be useful to be able to verify code implementing SHA256 using SIMD since such code is a bit more complicated and at some points requires use of pointers. Until now `miri` didn't support x86 SHA256 intrinsics. This commit implements them.
|
Addressed the nits. |
|
Looks good, thanks. :) @bors r+ |
|
☀️ Test successful - checks-actions |
|
I can't find any information about the release schedule. Will this be available in the next nightly or will I have to wait longer? |
|
It will be available once Miri get synced into the rustc repo. I usually do this each weekend, though this weekend I am traveling so it might happen later. |
Disclaimer: this is my first contribution to
miri's code. It's quite possible I'm missing something. This code works but may not be the cleanest/best possible.It'd be useful to be able to verify code implementing SHA256 using SIMD since such code is a bit more complicated and at some points requires use of pointers. Until now
mirididn't support x86 SHA256 intrinsics. This commit implements them.