Repository navigation
CBMC: Add functional spec for runtime bounds check - #1937
hanno-becker wants to merge 3 commits into
Conversation
ee8422c to
030f4c1
Compare
`mlk_debug_check_bounds_report()` scans an `int16_t` array and reports values outside exclusive bounds. Prove that its return value equals `array_bound(ptr, 0, len, lower_bound_exclusive + 1, upper_bound_exclusive)`. The loop invariant establishes the same claim for each processed prefix. The `+ 1` conversion assumes `lower_bound_exclusive < INT32_MAX`. Also check the public bounds wrapper and assertion helper for safety. Pass `file_len` through the debug APIs and require a nonempty fresh range ending in a null byte. Macros pass `sizeof(__FILE__)`; harnesses keep the inputs arbitrary. Use an explicit length because CBMC 6.11's string predicates do not work with the current dynamic-frame pass. CBMC's `vfprintf` model does not parse formats or inspect variadic arguments. These proofs therefore do not establish memory safety or output behavior of the real `fprintf`; they check only the surrounding C code under CBMC's library model. Declare `exit` in the shared data-model shims. Diagnostics, ordering, and exit behavior are unchanged. Signed-off-by: Hanno Becker <beckphan@amazon.co.uk>
030f4c1 to
083555b
Compare
CBMC Results (ML-KEM-512)
Full Results (201 configurations)
|
CBMC Results (ML-KEM-768)
Full Results (201 configurations)
|
CBMC Results (ML-KEM-1024)
Full Results (201 configurations)
|
083555b to
d948659
Compare
RVV debug bounds checks spill active vector lanes into scalar arrays before calling the generic checker. In-tree assertions use only the m1 adapter; the m2 helper and macros have no callers. The m1 adapter sizes its buffer to `MLKEM_N` but does not check `vl` before the vector store. Remove the m2 function, macros, and generated single-compilation-unit undefinitions. Assert `vl <= MLKEM_N` before `__riscv_vse16_v_i16m1`. Include `riscv_vector.h` in the debug-only header branch. Preserve the compilation-unit guard and give `MLKEM_DEBUG` priority over `CBMC`, as the scalar helpers do. The CBMC-only vector macros remain no-ops. Pass `file_len` through the remaining m1 adapter so calls to the scalar debug helpers preserve their bounded filename contracts. Its macro passes `sizeof(__FILE__)`. Contract lint finds proofs by directory name. The bounds reporter and wrapper previously shared `debug_check_bounds`, so the reporter had no matching proof directory and lint rejected its contract. Give `mlk_debug_check_bounds_report` its own harness and proof directory. Keep `debug_check_bounds` for the wrapper and use the reporter's contract there. Both proofs retain the existing LP64 and ILP32 configurations and CBMC library model. Keep `MLKEM_DEBUG` enabled in the scalar debug-helper proofs so they compile the runtime implementations rather than the assertion macros used by ordinary CBMC proofs. The debug proofs include native stdio headers. On Linux, Nix enables glibc's fortified `fprintf` wrapper, whose `__builtin_va_arg_pack` call has no CBMC model. The assertion and bounds-reporter proofs therefore fail when they take a diagnostic path. Disable `fortify` in the CBMC Nix shell to retain calls to the modeled libc functions. Keep hardening in other shells unchanged. CBMC's `fprintf` model does not inspect variadic arguments. These scalar proofs do not establish memory safety or output behavior of the real `fprintf`, or cover the RVV store intrinsic. Signed-off-by: Hanno Becker <beckphan@amazon.co.uk>
d948659 to
51d5701
Compare
The README excludes all debug code from the CBMC safety claim, even though dedicated proofs now cover the scalar helpers. Remove that exclusion from both coverage statements. State in `SOUNDNESS.md` that the proofs use libc models which do not parse `fprintf` formats or inspect variadic arguments, and therefore do not establish safety or output behavior of real formatted diagnostics. Signed-off-by: Hanno Becker <beckphan@amazon.co.uk>
mkannwischer
left a comment
There was a problem hiding this comment.
Thanks @hanno-becker. Are you sure that the value added by this PR justifies the added complexity? A few things definitely have to be addressed before we can merge it.
| * Spell out C-string validity using a length because CBMC 6.11 ignores | ||
| * __CPROVER_is_cstring and ignores __CPROVER_is_zero_string without string | ||
| * abstraction. String abstraction is incompatible with the dynamic-frame | ||
| * contract pass used by these proofs. |
There was a problem hiding this comment.
Should this be a TODO with a corresponding issue? If this is unlikely to be fixed in 6.12 maybe trim down the comment as I can see this going stale very quickly otherwise.
| # Fortified glibc stdio wrappers use __builtin_va_arg_pack, which | ||
| # CBMC does not model. Keep the calls to the modeled libc functions. | ||
| hardeningDisable = [ "fortify" ]; |
There was a problem hiding this comment.
I'm not very happy about this introduction of complexity.
This also means that it will only work in the cbmc development shell and will fail in the default shell. That definitely needs to be fixed.
| USE_DYNAMIC_FRAMES = 1 | ||
|
|
||
| # Supported CBMC configurations | ||
| CBMC_SUPPORTED_CONFIGS = LP64:z3 ILP32:z3 |
There was a problem hiding this comment.
This proof fails for me with
make clean && CBMC_DM=ILP32 make result
That likely means this never runs in CI which is bad.
| int valid = 1; | ||
| unsigned i; | ||
|
|
||
| (void)file_len; |
There was a problem hiding this comment.
What's this file_len argument for? it seems it's never used anyway?
|
@mkannwischer I think your question is fair, yes. Until we find a more convincing solution, we can close the PR. |
No description provided.