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

Update to FIPS204 and rename Dilithium to ML-DSA #363

Merged
merged 6 commits into from
Oct 22, 2024
Merged

Conversation

mkannwischer
Copy link
Contributor

@mkannwischer mkannwischer commented Oct 15, 2024

  • PR changes testvectors
  • Tests pass in qemu
  • Testvectors pass in qemu
  • Tests pass on Nucleo-L4R5ZI
  • Testvectors pass on Nucleo-L4R5ZI
  • Updated Benchmarks
  • Updated Skiplist entries

@mkannwischer mkannwischer marked this pull request as ready for review October 15, 2024 10:22
@rpls
Copy link
Contributor

rpls commented Oct 21, 2024

I noticed compiler warnings for the m4stack versions which may actually be problematic, depending on how the compiler compiles the code:

crypto_sign/ml-dsa-44/m4fstack/sign.c: In function 'crypto_sign_verify_ctx':
crypto_sign/ml-dsa-44/m4fstack/sign.c:366:24: warning: initialization of 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint8_t (*)[192]' {aka 'unsigned char (*)[192]'} [-Wincompatible-pointer-types]
  366 |   uint8_t *w1_packed = &w1_packed_comp.w1_packed;
      |                        ^
crypto_sign/ml-dsa-44/m4fstack/sign.c:367:21: warning: initialization of 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint8_t (*)[768]' {aka 'unsigned char (*)[768]'} [-Wincompatible-pointer-types]
  367 |   uint8_t *wcomp  = &w1_packed_comp.wcomp;
      |                     ^
crypto_sign/ml-dsa-44/m4fstack/sign.c:373:20: warning: initialization of 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint8_t (*)[68]' {aka 'unsigned char (*)[68]'} [-Wincompatible-pointer-types]
  373 |   uint8_t *ccomp = &ccomp_mu.ccomp;
      |                    ^
crypto_sign/ml-dsa-44/m4fstack/sign.c:374:18: warning: initialization of 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint8_t (*)[64]' {aka 'unsigned char (*)[64]'} [-Wincompatible-pointer-types]
  374 |   uint8_t *mu  = &ccomp_mu.mu;
      |                  ^
crypto_sign/ml-dsa-44/m4fstack/sign.c:384:26: warning: initialization of 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint8_t (*)[80]' {aka 'unsigned char (*)[80]'} [-Wincompatible-pointer-types]
  384 |   uint8_t *hint_ones   = &shake_hint.hint_ones;
      |                          ^
crypto_sign/ml-dsa-44/m4fstack/sign.c:386:26: warning: initialization of 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint8_t (*)[32]' {aka 'unsigned char (*)[32]'} [-Wincompatible-pointer-types]
  386 |   uint8_t *c2          = &shake_hint.c2;

I guess it works because it takes the address of an element of the packed structures, but from the C semantics, it takes the address of an array. Which might be the address of a pointer. But what the code means is probably "address of the first element", i.e., uint8_t *c2 = &shake_hint.c2[0]; with the added [0]. Right now, the compiler seems to correctly guess the intention, but it might not in the future and result in REALLY weird bugs. Unless that actually is the defined behavior of C when taking the address of an array (not sure about this).

@mkannwischer
Copy link
Contributor Author

I noticed compiler warnings for the m4stack versions which may actually be problematic, depending on how the compiler compiles the code:

crypto_sign/ml-dsa-44/m4fstack/sign.c: In function 'crypto_sign_verify_ctx':
crypto_sign/ml-dsa-44/m4fstack/sign.c:366:24: warning: initialization of 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint8_t (*)[192]' {aka 'unsigned char (*)[192]'} [-Wincompatible-pointer-types]
  366 |   uint8_t *w1_packed = &w1_packed_comp.w1_packed;
      |                        ^
crypto_sign/ml-dsa-44/m4fstack/sign.c:367:21: warning: initialization of 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint8_t (*)[768]' {aka 'unsigned char (*)[768]'} [-Wincompatible-pointer-types]
  367 |   uint8_t *wcomp  = &w1_packed_comp.wcomp;
      |                     ^
crypto_sign/ml-dsa-44/m4fstack/sign.c:373:20: warning: initialization of 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint8_t (*)[68]' {aka 'unsigned char (*)[68]'} [-Wincompatible-pointer-types]
  373 |   uint8_t *ccomp = &ccomp_mu.ccomp;
      |                    ^
crypto_sign/ml-dsa-44/m4fstack/sign.c:374:18: warning: initialization of 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint8_t (*)[64]' {aka 'unsigned char (*)[64]'} [-Wincompatible-pointer-types]
  374 |   uint8_t *mu  = &ccomp_mu.mu;
      |                  ^
crypto_sign/ml-dsa-44/m4fstack/sign.c:384:26: warning: initialization of 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint8_t (*)[80]' {aka 'unsigned char (*)[80]'} [-Wincompatible-pointer-types]
  384 |   uint8_t *hint_ones   = &shake_hint.hint_ones;
      |                          ^
crypto_sign/ml-dsa-44/m4fstack/sign.c:386:26: warning: initialization of 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint8_t (*)[32]' {aka 'unsigned char (*)[32]'} [-Wincompatible-pointer-types]
  386 |   uint8_t *c2          = &shake_hint.c2;

I guess it works because it takes the address of an element of the packed structures, but from the C semantics, it takes the address of an array. Which might be the address of a pointer. But what the code means is probably "address of the first element", i.e., uint8_t *c2 = &shake_hint.c2[0]; with the added [0]. Right now, the compiler seems to correctly guess the intention, but it might not in the future and result in REALLY weird bugs. Unless that actually is the defined behavior of C when taking the address of an array (not sure about this).

Good catch - thank you! I've fixed the compiler warnings.
I also made two other changes (removing unpack_sk_stack and replacing csubq with caddq) - those may have a very slight performance impact, so I'll need to benchmark again tomorrow.

@mkannwischer
Copy link
Contributor Author

I'm not seeing any changes in the benchmarks beyond the usual ML-DSA fluctuations due to the rejection sampling.

@mkannwischer mkannwischer merged commit 68e1ca7 into master Oct 22, 2024
6 checks passed
@mkannwischer mkannwischer deleted the fips204 branch October 22, 2024 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants