Skip to content

Commit 1f62283

Browse files
hanno-beckermkannwischer
authored andcommitted
CBMC: Document and enforce 4 GiB limit of forall/exists quantifiers
This commit ports pq-code-package/mlkem-native#1694. The forall/exists macros in cbmc.h declared the quantified variable as `unsigned`, which on 64-bit platforms is 32 bits wide. When applied to a size_t bound, this silently truncated the upper bound: the quantifier only ranged over indices up to UINT32_MAX rather than the full size_t range, leaving the predicate unchecked for any larger indices. mldsa-native does accept arbitrarily large buffers at the top-level API: the message, context, and pre-string passed into mld_sign / mld_sign_verify / mld_H / mld_shake256_absorb are bounded only by MLD_MAX_BUFFER_SIZE = SIZE_MAX >> 12 (~2^52 on 64-bit), and keccak_absorb walks them in a size_t loop. However, those buffers never feed into a quantified predicate -- they appear only as arguments to memory_no_alias / memory_slice and as scalar loop invariants. Hence, the restricted scope of `forall` has no impact on the CBMC safety guarantees for mldsa-native's top-level API. The only size_t-bounded quantifier in the entire tree is mld_ct_memcmp in ct.h, and it already requires len <= UINT16_MAX. For now, make the limitation explicit rather than widening the quantifier: - Explicitly cast the lower and upper bounds to `uint32_t`, so passing a wider bound triggers CBMC's conversion check rather than silently truncating. - Declare the quantified variable as `uint32_t` (was `unsigned`) as a robustness improvement towards the implementation-defined size of `unsigned`. Note: the upstream PR also tightened mlk_ct_cmov_zero's precondition to UINT32_MAX. mldsa-native has no equivalent function, so no analogous change is needed. We may still want to consider widening the quantifier variable as a follow-up, but this is a more intrusive change that prior experiments have proved to significantly impact proof performance. Signed-off-by: Hanno Becker <beckphan@amazon.co.uk>
1 parent 1b47ba6 commit 1f62283

1 file changed

Lines changed: 28 additions & 16 deletions

File tree

mldsa/src/cbmc.h

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -93,24 +93,36 @@
9393
* Quantifiers
9494
* Note that the range on qvar is _exclusive_ between qvar_lb .. qvar_ub
9595
* https://diffblue.github.io/cbmc/contracts-quantifiers.html
96+
*
97+
* The quantified variable is declared as uint32_t, so these macros
98+
* quantify only over indices in [0, UINT32_MAX). Bounds larger than
99+
* UINT32_MAX (4 GiB) are NOT supported: the explicit (uint32_t) casts
100+
* on the bounds will trigger CBMC's conversion check if a wider bound
101+
* (e.g. a size_t > UINT32_MAX) is passed.
102+
*
103+
* Quantifying over size_t (64-bit) was found to blow up SMT proof
104+
* times, so we deliberately keep the index width at 32 bits. Callers
105+
* dealing with size_t-typed buffers must add an explicit
106+
* requires(len <= UINT32_MAX)
107+
* precondition.
96108
*/
97109

98110
/*
99111
* Prevent clang-format from corrupting CBMC's special ==> operator
100112
*/
101113
/* clang-format off */
102-
#define forall(qvar, qvar_lb, qvar_ub, predicate) \
103-
__CPROVER_forall \
104-
{ \
105-
unsigned qvar; \
106-
((qvar_lb) <= (qvar) && (qvar) < (qvar_ub)) ==> (predicate) \
114+
#define forall(qvar, qvar_lb, qvar_ub, predicate) \
115+
__CPROVER_forall \
116+
{ \
117+
uint32_t qvar; \
118+
((uint32_t) (qvar_lb) <= (qvar) && (qvar) < (uint32_t) (qvar_ub)) ==> (predicate) \
107119
}
108120

109-
#define exists(qvar, qvar_lb, qvar_ub, predicate) \
110-
__CPROVER_exists \
111-
{ \
112-
unsigned qvar; \
113-
((qvar_lb) <= (qvar) && (qvar) < (qvar_ub)) && (predicate) \
121+
#define exists(qvar, qvar_lb, qvar_ub, predicate) \
122+
__CPROVER_exists \
123+
{ \
124+
uint32_t qvar; \
125+
((uint32_t) (qvar_lb) <= (qvar) && (qvar) < (uint32_t) (qvar_ub)) && (predicate) \
114126
}
115127
/* clang-format on */
116128

@@ -128,8 +140,8 @@
128140
value_lb, value_ub) \
129141
__CPROVER_forall \
130142
{ \
131-
unsigned qvar; \
132-
((qvar_lb) <= (qvar) && (qvar) < (qvar_ub)) ==> \
143+
uint32_t qvar; \
144+
((uint32_t) (qvar_lb) <= (qvar) && (qvar) < (uint32_t) (qvar_ub)) ==> \
133145
(((int)(value_lb) <= ((array_var)[(qvar)])) && \
134146
(((array_var)[(qvar)]) < (int)(value_ub))) \
135147
}
@@ -141,8 +153,8 @@
141153
#define array_unchanged_core(qvar, qvar_lb, qvar_ub, array_var) \
142154
__CPROVER_forall \
143155
{ \
144-
unsigned qvar; \
145-
((qvar_lb) <= (qvar) && (qvar) < (qvar_ub)) ==> \
156+
uint32_t qvar; \
157+
((uint32_t) (qvar_lb) <= (qvar) && (qvar) < (uint32_t) (qvar_ub)) ==> \
146158
((array_var)[(qvar)]) == (old(* (int32_t (*)[(qvar_ub)])(array_var)))[(qvar)] \
147159
}
148160

@@ -152,8 +164,8 @@
152164
#define array_unchanged_u64_core(qvar, qvar_lb, qvar_ub, array_var) \
153165
__CPROVER_forall \
154166
{ \
155-
unsigned qvar; \
156-
((qvar_lb) <= (qvar) && (qvar) < (qvar_ub)) ==> \
167+
uint32_t qvar; \
168+
((uint32_t) (qvar_lb) <= (qvar) && (qvar) < (uint32_t) (qvar_ub)) ==> \
157169
((array_var)[(qvar)]) == (old(* (uint64_t (*)[(qvar_ub)])(array_var)))[(qvar)] \
158170
}
159171

0 commit comments

Comments
 (0)