Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rv32 sha512sigDX wrong order #128

Closed
marcfedorow opened this issue Sep 20, 2021 · 3 comments
Closed

rv32 sha512sigDX wrong order #128

marcfedorow opened this issue Sep 20, 2021 · 3 comments
Labels
public-review For all comments related to the public review process

Comments

@marcfedorow
Copy link

Note to software developers says:

The  entire  Sigma0  transform  for  SHA2-512  may  be  computed  on  RV32  using  the  following
instruction sequence:
sha512sig0l    t0, a0, a1
sha512sig0h    t1, a0, a1

This is not true. Actually sigma0 transform for SHA2-512 may be computed on RV32 using the following

sha512sig0l    t0, a0, a1
sha512sig0h    t1, a1, a0

It is obviously a typo whilst this instruction is ror-based (as sha512sum0r).

@marcfedorow
Copy link
Author

Also note that OpenSSL marks Sum functions as Sigma, and Sigma functions as sigma.
https://github.com/openssl/openssl/blob/master/crypto/sha/sha512.c
It is probably better to use OpenSSL's naming, but I am not sure.

@ben-marshall
Copy link
Member

ben-marshall commented Sep 20, 2021

Nice catch! Thank you, I will update this in the spec when I have some time on Wednesday.

You're absolutely right about how it was a typo, I must have copied the example and forgotten to change the registers.

@ben-marshall ben-marshall added the public-review For all comments related to the public review process label Sep 20, 2021
ben-marshall added a commit that referenced this issue Sep 22, 2021
- Register ordering of examples was wrong. See Issue #128

 On branch master
 Your branch is up-to-date with 'origin/master'.

 Changes to be committed:
	modified:   insns/sha512sig0h.adoc
	modified:   insns/sha512sig0l.adoc
	modified:   insns/sha512sig1h.adoc
	modified:   insns/sha512sig1l.adoc
@ben-marshall
Copy link
Member

Hi @marcfedorow - I've updated the spec in 1b66e0a - it turns out several instructions had the same typo. Thanks again for spotting this, I'll leave the issue open in case others spot it to avoid duplicates.
Cheers,
Ben

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
public-review For all comments related to the public review process
Projects
None yet
Development

No branches or pull requests

3 participants