Merge pull request #479 from pq-code-package/remove_todo

Resolve or remove various small TODOs
pq-code-package · Dec 3, 2024 · b5f9627 · b5f9627 · oqs-bot · Dec 3, 2024
2 parents 425bc20 + 5cdbeea
commit b5f9627
Show file tree

Hide file tree

Showing 11 changed files with 53 additions and 50 deletions.
diff --git a/cbmc/proofs/poly_compress_du/Makefile b/cbmc/proofs/poly_compress_du/Makefile
@@ -19,12 +19,16 @@ PROOF_SOURCES += $(PROOFDIR)/$(HARNESS_FILE).c
 PROJECT_SOURCES += $(SRCDIR)/mlkem/poly.c
 
 CHECK_FUNCTION_CONTRACTS=$(MLKEM_NAMESPACE)poly_compress_du
+USE_FUNCTION_CONTRACTS =
+
+# TODO: We should be calling scalar_decompress_xxx by contract here,
+# but it does not seem to work yet because they are marked as static inline.
 # For K = 2 or 3, the code calls scalar_compress_d10, so
-ifeq ($(MLKEM_K),4)
-USE_FUNCTION_CONTRACTS = scalar_compress_d11
-else
-USE_FUNCTION_CONTRACTS = scalar_compress_d10
-endif
+# ifeq ($(MLKEM_K),4)
+# USE_FUNCTION_CONTRACTS = scalar_compress_d11
+# else
+# USE_FUNCTION_CONTRACTS = scalar_compress_d10
+# endif
 
 APPLY_LOOP_CONTRACTS=on
 USE_DYNAMIC_FRAMES=1

diff --git a/cbmc/proofs/poly_compress_dv/Makefile b/cbmc/proofs/poly_compress_dv/Makefile
@@ -19,11 +19,15 @@ PROOF_SOURCES += $(PROOFDIR)/$(HARNESS_FILE).c
 PROJECT_SOURCES += $(SRCDIR)/mlkem/poly.c
 
 CHECK_FUNCTION_CONTRACTS=$(MLKEM_NAMESPACE)poly_compress_dv
-ifeq ($(MLKEM_K),4)
-USE_FUNCTION_CONTRACTS = scalar_compress_d5
-else
-USE_FUNCTION_CONTRACTS = scalar_compress_d4
-endif
+
+USE_FUNCTION_CONTRACTS =
+# TODO: We should be calling scalar_decompress_xxx by contract here,
+# but it does not seem to work yet because they are marked as static inline.
+# ifeq ($(MLKEM_K),4)
+# USE_FUNCTION_CONTRACTS = scalar_compress_d5
+# else
+# USE_FUNCTION_CONTRACTS = scalar_compress_d4
+# endif
 APPLY_LOOP_CONTRACTS=on
 USE_DYNAMIC_FRAMES=1
 

diff --git a/cbmc/proofs/poly_decompress_du/Makefile b/cbmc/proofs/poly_decompress_du/Makefile
@@ -20,12 +20,15 @@ PROJECT_SOURCES += $(SRCDIR)/mlkem/poly.c
 
 CHECK_FUNCTION_CONTRACTS=$(MLKEM_NAMESPACE)poly_decompress_du
 
+USE_FUNCTION_CONTRACTS =
+# TODO: We should be calling scalar_decompress_xxx by contract here,
+# but it does not seem to work yet because they are marked as static inline.
 # For K = 2 or 3, the code calls scalar_decompress_d10, so
-ifeq ($(MLKEM_K),4)
-USE_FUNCTION_CONTRACTS = scalar_decompress_d11
-else
-USE_FUNCTION_CONTRACTS = scalar_decompress_d10
-endif
+# ifeq ($(MLKEM_K),4)
+# USE_FUNCTION_CONTRACTS = scalar_decompress_d11
+# else
+# USE_FUNCTION_CONTRACTS = scalar_decompress_d10
+# endif
 
 APPLY_LOOP_CONTRACTS=on
 USE_DYNAMIC_FRAMES=1

diff --git a/cbmc/proofs/poly_decompress_dv/Makefile b/cbmc/proofs/poly_decompress_dv/Makefile
@@ -20,12 +20,15 @@ PROJECT_SOURCES += $(SRCDIR)/mlkem/poly.c
 
 CHECK_FUNCTION_CONTRACTS=$(MLKEM_NAMESPACE)poly_decompress_dv
 
+USE_FUNCTION_CONTRACTS =
+# TODO: We should be calling scalar_decompress_xxx by contract here,
+# but it does not seem to work yet because they are marked as static inline.
 # For K = 2 or 3, the code calls scalar_decompress_d4, so
-ifeq ($(MLKEM_K),4)
-USE_FUNCTION_CONTRACTS = scalar_decompress_d5
-else
-USE_FUNCTION_CONTRACTS = scalar_decompress_d4
-endif
+# ifeq ($(MLKEM_K),4)
+# USE_FUNCTION_CONTRACTS = scalar_decompress_d5
+# else
+# USE_FUNCTION_CONTRACTS = scalar_decompress_d4
+# endif
 
 APPLY_LOOP_CONTRACTS=on
 USE_DYNAMIC_FRAMES=1

diff --git a/mlkem/indcpa.c b/mlkem/indcpa.c
@@ -61,9 +61,11 @@ static void unpack_pk(polyvec *pk, uint8_t seed[MLKEM_SYMBYTES],
   memcpy(seed, packedpk + MLKEM_POLYVECBYTES, MLKEM_SYMBYTES);
 
   /*
-   * TODO! pk must be subject to a "modulus check" at the top-level
-   * crypto_kem_enc_derand(). Once that's done, the reduction is no
-   * longer necessary here.
+   * TODO! We know from the modulus check that this will result in an
+   * unsigned canonical polynomial, but CBMC does not know it. We should
+   * weaken the specification of `unpack_pk()` and all depending functions
+   * to work with the weaker 4096-bound, so that the proofs go through
+   * without the need of this redundant call to polyvec_reduce().
    */
   polyvec_reduce(pk);
 }
@@ -291,13 +293,6 @@ void gen_matrix(polyvec *a, const uint8_t seed[MLKEM_SYMBYTES], int transposed)
     memcpy(seedxy[j], seed, MLKEM_SYMBYTES);
   }
 
-  /*
-   * TODO: All loops in this function should be unrolled for decent
-   * performance.
-   * Either add suitable pragmas, or split gen_matrix according to MLKEM_K
-   * and unroll by hand.
-   */
-
   for (i = 0; i < (MLKEM_K * MLKEM_K / KECCAK_WAY) * KECCAK_WAY;
        i += KECCAK_WAY)
   {

diff --git a/mlkem/native/arith_native.h b/mlkem/native/arith_native.h
@@ -240,7 +240,7 @@ static INLINE void poly_frombytes_native(poly *a,
  *
  * Return -1 if the native implementation does not support the input lengths.
  * Otherwise, returns non-negative number of sampled 16-bit integers (at most
- *len).
+ * len).
  **************************************************/
 static INLINE int rej_uniform_native(int16_t *r, unsigned int len,
                                      const uint8_t *buf, unsigned int buflen);

diff --git a/mlkem/native/x86_64/arith_native_x86_64.h b/mlkem/native/x86_64/arith_native_x86_64.h
@@ -20,7 +20,6 @@
   ((12 * MLKEM_N / 8 * (1 << 12) / MLKEM_Q + SHAKE128_RATE) / SHAKE128_RATE)
 #define REJ_UNIFORM_AVX_BUFLEN (REJ_UNIFORM_AVX_NBLOCKS * SHAKE128_RATE)
 
-/* TODO: Document buffer constraints */
 #define rej_uniform_avx2 MLKEM_NAMESPACE(rej_uniform_avx2)
 unsigned int rej_uniform_avx2(int16_t *r, const uint8_t *buf);
 

diff --git a/mlkem/native/x86_64/profiles/default.h b/mlkem/native/x86_64/profiles/default.h
@@ -74,13 +74,7 @@ static INLINE void poly_mulcache_compute_native(poly_mulcache *x, const poly *y)
 {
   /* AVX2 backend does not use mulcache */
   ((void)y);
-
-  /*
-   * TODO! The mulcache is subject to the absolute bound < q
-   * This needs to be dropped if the mulcache is not present.
-   * Until that's done, memset to 0 to avoid failure.
-   */
-  memset(x, 0, sizeof(poly_mulcache));
+  ((void)x);
 }
 
 static INLINE void polyvec_basemul_acc_montgomery_cached_native(

diff --git a/mlkem/poly.c b/mlkem/poly.c
@@ -456,6 +456,8 @@ void poly_basemul_montgomery_cached(poly *r, const poly *a, const poly *b,
                                     const poly_mulcache *b_cache)
 {
   int i;
+  POLY_BOUND(b_cache, MLKEM_Q);
+
   for (i = 0; i < MLKEM_N / 4; i++)
   __loop__(
     assigns(i, object_whole(r))
@@ -559,6 +561,8 @@ void poly_mulcache_compute(poly_mulcache *x, const poly *a)
 void poly_mulcache_compute(poly_mulcache *x, const poly *a)
 {
   poly_mulcache_compute_native(x, a);
-  POLY_BOUND(x, MLKEM_Q);
+  /* Omitting POLY_BOUND(x, MLKEM_Q) since native implementations may
+   * decide not to use a mulcache. Note that the C backend implementation
+   * of poly_basemul_montgomery_cached() does still include the check. */
 }
 #endif /* MLKEM_USE_NATIVE_POLY_MULCACHE_COMPUTE */
diff --git a/mlkem/poly.h b/mlkem/poly.h
@@ -192,9 +192,7 @@ __contract__(
 #pragma CPROVER check push
 #pragma CPROVER check disable "unsigned-overflow"
 #endif
-/* TODO: do the same for the other static inline functions */
-STATIC_INLINE_TESTABLE
-uint32_t scalar_compress_d10(uint16_t u)
+static INLINE uint32_t scalar_compress_d10(uint16_t u)
 __contract__(
   requires(u <= MLKEM_Q - 1)
   ensures(return_value < (1u << 10))
@@ -244,8 +242,7 @@ __contract__(
 #pragma CPROVER check push
 #pragma CPROVER check disable "unsigned-overflow"
 #endif
-STATIC_INLINE_TESTABLE
-uint32_t scalar_compress_d11(uint16_t u)
+static INLINE uint32_t scalar_compress_d11(uint16_t u)
 __contract__(
   requires(u <= MLKEM_Q - 1)
   ensures(return_value < (1u << 11))
@@ -270,8 +267,7 @@ __contract__(
  * Arguments: - u: Unsigned canonical modulus modulo 16
  *                 to be decompressed.
  ************************************************************/
-STATIC_INLINE_TESTABLE
-uint16_t scalar_decompress_d11(uint32_t u)
+static INLINE uint16_t scalar_decompress_d11(uint32_t u)
 __contract__(
   requires(0 <= u && u < 2048)
   ensures(return_value <= (MLKEM_Q - 1))
@@ -295,8 +291,7 @@ __contract__(
  *
  * Arguments: c: signed coefficient to be converted
  ************************************************************/
-STATIC_INLINE_TESTABLE
-uint16_t scalar_signed_to_unsigned_q(int16_t c)
+static INLINE uint16_t scalar_signed_to_unsigned_q(int16_t c)
 __contract__(
   requires(c >= -(MLKEM_Q - 1) && c <= (MLKEM_Q - 1))
   ensures(return_value >= 0 && return_value <= (MLKEM_Q - 1))

diff --git a/mlkem/polyvec.c b/mlkem/polyvec.c
@@ -128,7 +128,9 @@ void polyvec_basemul_acc_montgomery_cached(poly *r, const polyvec *a,
 {
   POLYVEC_BOUND(a, MLKEM_Q);
   POLYVEC_BOUND(b, NTT_BOUND);
-  POLYVEC_BOUND(b_cache, MLKEM_Q);
+  /* Omitting POLYVEC_BOUND(b_cache, MLKEM_Q) since native implementations may
+   * decide not to use a mulcache. Note that the C backend implementation
+   * of poly_basemul_montgomery_cached() does still include the check. */
   polyvec_basemul_acc_montgomery_cached_native(r, a, b, b_cache);
 }
 #endif /* MLKEM_USE_NATIVE_POLYVEC_BASEMUL_ACC_MONTGOMERY_CACHED */
Benchmark suite	Current: `b5f9627`	Previous: `425bc20`	Ratio
`ML-KEM-512 keypair`	`29175` cycles	`29175` cycles	`1`
`ML-KEM-512 encaps`	`35852` cycles	`35852` cycles	`1`
`ML-KEM-512 decaps`	`46655` cycles	`46653` cycles	`1.00`
`ML-KEM-768 keypair`	`49173` cycles	`49174` cycles	`1.00`
`ML-KEM-768 encaps`	`55842` cycles	`55839` cycles	`1.00`
`ML-KEM-768 decaps`	`71009` cycles	`71008` cycles	`1.00`
`ML-KEM-1024 keypair`	`72211` cycles	`72211` cycles	`1`
`ML-KEM-1024 encaps`	`81518` cycles	`81519` cycles	`1.00`
`ML-KEM-1024 decaps`	`102031` cycles	`102032` cycles	`1.00`
Benchmark suite	Current: `b5f9627`	Previous: `425bc20`	Ratio
`ML-KEM-512 keypair`	`13867` cycles	`13864` cycles	`1.00`
`ML-KEM-512 encaps`	`18209` cycles	`18199` cycles	`1.00`
`ML-KEM-512 decaps`	`24114` cycles	`24094` cycles	`1.00`
`ML-KEM-768 keypair`	`22529` cycles	`22428` cycles	`1.00`
`ML-KEM-768 encaps`	`24606` cycles	`24506` cycles	`1.00`
`ML-KEM-768 decaps`	`32992` cycles	`32521` cycles	`1.01`
`ML-KEM-1024 keypair`	`32044` cycles	`32155` cycles	`1.00`
`ML-KEM-1024 encaps`	`35669` cycles	`35727` cycles	`1.00`
`ML-KEM-1024 decaps`	`47172` cycles	`47393` cycles	`1.00`
Benchmark suite	Current: `b5f9627`	Previous: `425bc20`	Ratio
`ML-KEM-512 keypair`	`20406` cycles	`20326` cycles	`1.00`
`ML-KEM-512 encaps`	`27162` cycles	`27129` cycles	`1.00`
`ML-KEM-512 decaps`	`36252` cycles	`36210` cycles	`1.00`
`ML-KEM-768 keypair`	`34879` cycles	`34834` cycles	`1.00`
`ML-KEM-768 encaps`	`38211` cycles	`38193` cycles	`1.00`
`ML-KEM-768 decaps`	`51431` cycles	`51360` cycles	`1.00`
`ML-KEM-1024 keypair`	`47922` cycles	`48074` cycles	`1.00`
`ML-KEM-1024 encaps`	`54120` cycles	`54249` cycles	`1.00`
`ML-KEM-1024 decaps`	`72069` cycles	`72137` cycles	`1.00`
Benchmark suite	Current: `b5f9627`	Previous: `425bc20`	Ratio
`ML-KEM-512 keypair`	`18125` cycles	`18152` cycles	`1.00`
`ML-KEM-512 encaps`	`23108` cycles	`23137` cycles	`1.00`
`ML-KEM-512 decaps`	`30437` cycles	`30396` cycles	`1.00`
`ML-KEM-768 keypair`	`31062` cycles	`31050` cycles	`1.00`
`ML-KEM-768 encaps`	`33988` cycles	`33990` cycles	`1.00`
`ML-KEM-768 decaps`	`44793` cycles	`44804` cycles	`1.00`
`ML-KEM-1024 keypair`	`44464` cycles	`44728` cycles	`0.99`
`ML-KEM-1024 encaps`	`49925` cycles	`50036` cycles	`1.00`
`ML-KEM-1024 decaps`	`64360` cycles	`64742` cycles	`0.99`
Benchmark suite	Current: `b5f9627`	Previous: `425bc20`	Ratio
`ML-KEM-512 keypair`	`15094` cycles	`15076` cycles	`1.00`
`ML-KEM-512 encaps`	`19754` cycles	`19754` cycles	`1`
`ML-KEM-512 decaps`	`26388` cycles	`26413` cycles	`1.00`
`ML-KEM-768 keypair`	`25558` cycles	`25586` cycles	`1.00`
`ML-KEM-768 encaps`	`28162` cycles	`28233` cycles	`1.00`
`ML-KEM-768 decaps`	`37988` cycles	`38203` cycles	`0.99`
`ML-KEM-1024 keypair`	`35553` cycles	`35635` cycles	`1.00`
`ML-KEM-1024 encaps`	`40680` cycles	`41255` cycles	`0.99`
`ML-KEM-1024 decaps`	`54473` cycles	`54608` cycles	`1.00`
Benchmark suite	Current: `b5f9627`	Previous: `425bc20`	Ratio
`ML-KEM-512 keypair`	`36267` cycles	`36182` cycles	`1.00`
`ML-KEM-512 encaps`	`46461` cycles	`46391` cycles	`1.00`
`ML-KEM-512 decaps`	`61881` cycles	`61856` cycles	`1.00`
`ML-KEM-768 keypair`	`58961` cycles	`59068` cycles	`1.00`
`ML-KEM-768 encaps`	`73061` cycles	`73046` cycles	`1.00`
`ML-KEM-768 decaps`	`91662` cycles	`91650` cycles	`1.00`
`ML-KEM-1024 keypair`	`88471` cycles	`88479` cycles	`1.00`
`ML-KEM-1024 encaps`	`109327` cycles	`109304` cycles	`1.00`
`ML-KEM-1024 decaps`	`133709` cycles	`133582` cycles	`1.00`
Benchmark suite	Current: `b5f9627`	Previous: `425bc20`	Ratio
`ML-KEM-512 keypair`	`19031` cycles	`19032` cycles	`1.00`
`ML-KEM-512 encaps`	`23843` cycles	`23843` cycles	`1`
`ML-KEM-512 decaps`	`31230` cycles	`31228` cycles	`1.00`
`ML-KEM-768 keypair`	`32332` cycles	`32332` cycles	`1`
`ML-KEM-768 encaps`	`36086` cycles	`36084` cycles	`1.00`
`ML-KEM-768 decaps`	`46491` cycles	`46492` cycles	`1.00`
`ML-KEM-1024 keypair`	`46964` cycles	`46970` cycles	`1.00`
`ML-KEM-1024 encaps`	`53086` cycles	`53090` cycles	`1.00`
`ML-KEM-1024 decaps`	`67401` cycles	`67403` cycles	`1.00`
Benchmark suite	Current: `b5f9627`	Previous: `425bc20`	Ratio
`ML-KEM-512 keypair`	`58031` cycles	`58051` cycles	`1.00`
`ML-KEM-512 encaps`	`65732` cycles	`65739` cycles	`1.00`
`ML-KEM-512 decaps`	`84332` cycles	`84356` cycles	`1.00`
`ML-KEM-768 keypair`	`98618` cycles	`98398` cycles	`1.00`
`ML-KEM-768 encaps`	`110516` cycles	`110291` cycles	`1.00`
`ML-KEM-768 decaps`	`137582` cycles	`136959` cycles	`1.00`
`ML-KEM-1024 keypair`	`150000` cycles	`149545` cycles	`1.00`
`ML-KEM-1024 encaps`	`166971` cycles	`166765` cycles	`1.00`
`ML-KEM-1024 decaps`	`203311` cycles	`203329` cycles	`1.00`
Benchmark suite	Current: `b5f9627`	Previous: `425bc20`	Ratio
`ML-KEM-512 keypair`	`29176` cycles	`29175` cycles	`1.00`
`ML-KEM-512 encaps`	`35859` cycles	`35860` cycles	`1.00`
`ML-KEM-512 decaps`	`46664` cycles	`46665` cycles	`1.00`
`ML-KEM-768 keypair`	`49198` cycles	`49225` cycles	`1.00`
`ML-KEM-768 encaps`	`55870` cycles	`55892` cycles	`1.00`
`ML-KEM-768 decaps`	`70985` cycles	`71026` cycles	`1.00`
`ML-KEM-1024 keypair`	`72174` cycles	`72173` cycles	`1.00`
`ML-KEM-1024 encaps`	`81621` cycles	`81620` cycles	`1.00`
`ML-KEM-1024 decaps`	`102067` cycles	`102066` cycles	`1.00`
Benchmark suite	Current: `b5f9627`	Previous: `425bc20`	Ratio
`ML-KEM-512 keypair`	`56697` cycles	`56710` cycles	`1.00`
`ML-KEM-512 encaps`	`71820` cycles	`71778` cycles	`1.00`
`ML-KEM-512 decaps`	`96361` cycles	`96333` cycles	`1.00`
`ML-KEM-768 keypair`	`91823` cycles	`91872` cycles	`1.00`
`ML-KEM-768 encaps`	`111402` cycles	`111635` cycles	`1.00`
`ML-KEM-768 decaps`	`144449` cycles	`144630` cycles	`1.00`
`ML-KEM-1024 keypair`	`134612` cycles	`134508` cycles	`1.00`
`ML-KEM-1024 encaps`	`160084` cycles	`159842` cycles	`1.00`
`ML-KEM-1024 decaps`	`201685` cycles	`201292` cycles	`1.00`