pqc-thunderbird / libgcrypt

GNU General Public License v2.0

1 stars 0 forks source link

Falko's review of ML-DSA and SLH-DSA with AVX2 #48

Closed falko-strenzke closed 9 months ago

falko-strenzke commented 9 months ago

dilithium_avx2 review Falko

Comments to individual files

cipher/mldsa.c

remove comment /* TODO nbits not meaningful for mldsa */

[x] assignments made in

if (param->gamma1 == (1 << 17))
{
  param->polyz_packedbytes = 576;
}
else if (param->gamma1 == (1 << 19))
{
  param->polyz_packedbytes = 640;
}

and if-Block beneath it should better be done in the switch-block above it.

cipher/mldsa-packing.c

[x] should be replaced by memcpy:

for (i = 0; i < params->ctildebytes; ++i)
  sig[i] = c[i];

cipher/mldsa-symmetric.c / .h

[x] These files seem to implement general SHAKE256. This should be replaced by calls to libgcrypt's SHAKE.

Missing copyright headers

[x] Basically all new files seem to lack copyright headers.

sphincsplus_avx2 review Falko

Missing copyright headers

[x] Basically all new files seem to lack copyright headers.

TJ-91 commented 9 months ago

Addressed comments for ML-DSA in 7ccee618456231596d062a5b29abd4ca7cc0258b:

Additionally to the comments I noticed that I needed to take into account the "--disable-avx2-support" flag (can be passed in the ./configure call) and added it.

Two comments from my side:

Regarding cipher/mldsa-symmetric.c, it doesn't implement SHAKE256 and uses the functionality from libgcrypt via gcry_md_hd_t.
For the headers I didn't include a statement regarding the contents of the file as you did in mlkem for example "/* mlkem.c - API functions for ML-KEM".