ML-KEM refactor

[dkostic]

Issues:

CryptoAlg-2614

Description of changes:

Previous to this change, ML-KEM was implemented such that
the code for a parameter set was selected by defining the correct
C pre-processor flag (for example, if you want to compile the code
for ML-KEM 512 parameter set you would #define KYBER_K 2).
The consequence of this was that we had to compile the code
three times for three ML-KEM parameter sets. We do this by adding
a C file for each parameter set where we first define the corresponding
KYBER_K value and then include all the ML-KEM C files. Besides being
an awkward way to handle multiple parameter sets, this will not work
for the FIPS module where we bundle all C files inside bcm.c and
compile it as a single compilation unit.

In this change we refactor ML-KEM by parametrizing all functions that
depend on values that are specific to a parameter set, i.e., that directly
or indirectly depend on the value of KYBER_K. To do this, in params.h
we define a structure that holds those ML-KEM parameters and functions
that initialize a given structure with values corresponding to a parameter
set. This structure can then be passed to every function that requires it.
For example, polyvec_ntt function was previously implemented like this:

void polyvec_ntt(polyvec *r) {
  unsigned int i;
  for(i=0;i<KYBER_K;i++)
    poly_ntt(&r->vec[i]);
}

We now change that to:

void polyvec_ntt(ml_kem_params *params, polyvec *r) {
  unsigned int i;
  for(i=0;i<params->k;i++)
    poly_ntt(&r->vec[i]);
}

Of course, now we have to change all callers of polyvec_ntt to also
have ml_kem_params as an input argument, and then callers of the
caller, etc. These changes bubble up to the highest level API defined
in kem.h where we now have:

int crypto_kem_keypair(ml_kem_params *params, ...);
int crypto_kem_enc(ml_kem_params * params, ...);
int crypto_kem_dec(ml_kem_params * params, ...);
int crypto_kem_keypair_derand(ml_kem_params *params, ...);
int crypto_kem_enc_derand(ml_kem_params * params, ...);

Then we can easily implement these functions for specific parameter sets,
which can be seen in ml_kem.c file. For example:

int ml_kem_512_ipd_keypair(...) {
  ml_kem_params params;
  ml_kem_512_params_init(&params);
  return ml_kem_keypair_ref(&params, ...);
}
int ml_kem_768_ipd_keypair(...) {
  ml_kem_params params;
  ml_kem_768_params_init(&params);
  return ml_kem_keypair_ref(&params, ...);
}

Call-outs:

Point out areas that need special attention or support during the review process. Discuss architecture or design changes.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.32%. Comparing base (79d5d16) to head (9c591df).
Report is 18 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1763      +/-   ##
==========================================
- Coverage   78.44%   78.32%   -0.13%     
==========================================
  Files         580      581       +1     
  Lines       96780    97176     +396     
  Branches    13863    13930      +67     
==========================================
+ Hits        75921    76113     +192     
- Misses      20243    20440     +197     
- Partials      616      623       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sgmenda-aws]

lgtm with minor changes

[sgmenda-aws]

This else is risky because release builds may remove the above assert so this code path may be reached by code with eta1 != 3. Might be worth replacing with else if (params->eta1 == 3) and do a separate else that is a no-op.

[WillChilds-Klein]

^ or move the assert to that else clause.

[sgmenda-aws]

Wouldn't the assert still be a no-op in release builds?

[sgmenda-aws]

Isn't seed always KYBER_SYMBYTES = 32 bytes long, irrespective of parameter set? And we use exactly KYBER_SYMBYTES bytes of seed, so seems worth keeping the constraint.

[sgmenda-aws]

Same comment as above for seed.

[sgmenda-aws]

poly_getnoise_* assumes coins is KYBER_SYMBYTES, so might be worth keeping the constraint.

[sgmenda-aws]

release builds may ignore this assert, leading to unexpected data for the below conditionals.

[WillChilds-Klein]

nit: this code would be a bit more legible if we did e.g. #define K_MLKEM_1024 352 for the 3 security levels in params.h

[WillChilds-Klein]

code changes look good, no blockers. per sanketh's earlier comments, can we keep parameter-invariant buffers as byte arrays rather than pointers?

NOTE: my review consisted of validating that refactored code remained unchanged and that null dereferences aren't possible given current initialization logic. I assumed the correctness of the existing implementation.

[WillChilds-Klein]

+1 to sanketh's earlier comment -- seed size is invariant over parameters, right? why can't we define this as an array rather than a pointer?

[dkostic]

yes, will fix in #1775