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

map to canonical #369

Merged
merged 1 commit into from
Nov 28, 2024
Merged

map to canonical #369

merged 1 commit into from
Nov 28, 2024

Conversation

vincentvbh
Copy link
Contributor

This PR fixes the testvector mismatch introduced in #244.

A bit more history.
In the PR #128, there was the same issue. But this was identified and fixed before merging to pqm4. The problem is that the element 3329 should be mapped to 0 while packing to the public key array. In pull/128, there are two reductions: one in the NTT and one in the poly_tobytes. Due to the stack optimizations that are not described in the specification of Kyber/ML-KEM, the output of NTT should be mapped to [0, q) while packing the elements to the public key array. This remains until pull/244 removed the reduction in the NTT (and counted as a contribution of their paper).

I encountered mismatched testvectors while generating with my own chacha20 for the randombytes and iterating with 5 iterations for all the parameter sets of ml-kem. The mismatched testvectors occurred only on the 5th iteration of ml-kem768.
I don't know what's the plan of the randombytes, so I only applied minimal changes to the files under crypto_kem.
Assembly optimized subroutines and detailed documentations will appear in the future.

  • 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
Copy link
Contributor

mkannwischer commented Nov 28, 2024

Thanks @vincentvbh. I confirm the mismatching testvectors with the current version (if you increase NTESTS - for ml-kem-768, just changing it from 2 to 3 is enough).

Let me benchmark this and merge it later today. We should really run more testvector iterations.
Tracking progress:

  • 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

This acts as a reminder that one should not be using code from pqm4 in production. If someone wants to use it in production, please contact me and we can possibly bring it into shape together (maybe within https://github.com/pq-code-package/mlkem-c-embedded).

@mkannwischer mkannwischer changed the base branch from master to fix-ml-kem November 28, 2024 05:16
@mkannwischer
Copy link
Contributor

I don't have permission to push to this fork - merging into local branch first.

@mkannwischer mkannwischer merged commit 4a98037 into mupq:fix-ml-kem Nov 28, 2024
6 checks passed
mkannwischer added a commit that referenced this pull request Nov 28, 2024
Continuation of 'map to canonical' #369
mkannwischer added a commit to mupq/pqm7 that referenced this pull request Dec 6, 2024
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