Skip to content

utils ct: replace black_box with a volatile optimization barrier in ct_eq_bytes and friends - #131

Open
ounsworth wants to merge 1 commit into
bcgit:release/0.1.3alphafrom
ounsworth:feature/ct_eq
Open

utils ct: replace black_box with a volatile optimization barrier in ct_eq_bytes and friends#131
ounsworth wants to merge 1 commit into
bcgit:release/0.1.3alphafrom
ounsworth:feature/ct_eq

Conversation

@ounsworth

Copy link
Copy Markdown
Contributor

Survey of other libraries (from their current upstream sources): libsodium's sodium_memcmp uses a volatile accumulator and volatile input pointers, and subtle's Choice::from is an #[inline(never)] read_volatile, so both place the barrier inside the loop per byte. constant_time_eq and RustCrypto's cmov chunk into machine words and apply an inline-asm barrier (or cmov/csel) per word, which is the shape adopted here. Graviola writes the whole byte loop in inline asm on x86_64 and aarch64. OpenSSL's CRYPTO_memcmp reads through volatile pointers only, and BoringSSL's uses no barrier at all.

Assembly was inspected by Claude from release builds on x86_64, i686, thumbv7em (Cortex-M4), riscv32imac, wasm32, msp430 (16-bit) and avr-none (8-bit): in every case the loop is load, xor, or, one native-width store to a stack slot and one reload, with a byte-wise tail, no bcmp/memcmp call and no data-dependent branch, and the final == 0 tests the volatile-loaded value.

Integration tests now sweep every length from 0 to 40 with a single bit flipped at every position, covering the word and tail paths on both sides of every boundary for 2, 4 and 8-byte words.

Co-Authored-By: Claude Fable 5

Closes #128

…t_eq_bytes and friends

Survey of other libraries (from their current upstream sources): libsodium's
`sodium_memcmp` uses a `volatile` accumulator and volatile input pointers,
and subtle's `Choice::from` is an `#[inline(never)]` `read_volatile`, so both
place the barrier inside the loop per byte. `constant_time_eq` and
RustCrypto's `cmov` chunk into machine words and apply an inline-asm barrier
(or `cmov`/`csel`) per word, which is the shape adopted here. Graviola writes
the whole byte loop in inline asm on x86_64 and aarch64. OpenSSL's
`CRYPTO_memcmp` reads through volatile pointers only, and BoringSSL's uses no
barrier at all.

Assembly was inspected by Claude from release builds on x86_64, i686, thumbv7em
(Cortex-M4), riscv32imac, wasm32, msp430 (16-bit) and avr-none (8-bit): in
every case the loop is load, xor, or, one native-width store to a stack slot
and one reload, with a byte-wise tail, no `bcmp`/`memcmp` call and no
data-dependent branch, and the final `== 0` tests the volatile-loaded value.

Integration tests now sweep every length from 0 to 40 with a single bit flipped at every
position, covering the word and tail paths on both sides of every boundary
for 2, 4 and 8-byte words.

Co-Authored-By: Claude Fable 5
@dghgit

dghgit commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Analysis courtesy of Opus 5. Some of these errors are extremely odd if this PR was generated using Fable - the LLMs do lend more weight to comments/text than code by default which could explain the mutant issue, but I'm not sure about the rest.

1. A killable mutant survives: |^ in volatile_or_assign (crypto/utils/src/ct.rs:605)

cargo mutants over the four changed functions gives 22 mutants, 20 caught, 2 missed. This one is not an OR/XOR equivalence of the kind we normally accept in crypto code — with ^ the accumulator cancels, so two differences that produce the same word-relative diff make unequal inputs compare equal. For example, flipping bit 0 of byte 0 and bit 0 of byte 8 gives acc = 1 ^ 1 = 0, and ct_eq_bytes returns true.

The new boundary sweep only ever flips a single bit at a time, so it cannot see this. Adding one case with two differences exactly one word apart (and, ideally, one pair within the byte-wise tail) closes the gap and kills the mutant.

2. The other missed mutant is genuinely equivalent

|^ at crypto/utils/src/ct.rs:685 in conditional_copy_bytes. mask is all-ones or all-zeros, so exactly one of a[i] & mask and b[i] & !mask is non-zero and | is identical to ^. Worth recording in the PR so nobody chases it later.

3. The release-notes bullet has three errors in two lines

* Refactored constant-time ct_equals_bytes () to use an optimization barrier based on unsafe read_volatile /
  write_volatile insead of core::mem::black_box.
  • ct_equals_bytes() is not a function in this repo — the name is ct_eq_bytes.
  • inseadinstead.
  • core::mem::black_boxcore::hint::black_box (which is what the code actually replaced).

There is also a trailing space at the end of the second line.

4. is_in_list still carries the question this PR just answered

crypto/utils/src/ct.rs:121 and :262 (the i64 and i32 impls) still read:

// Research question: is this actually constant-time?
// A clever compiler might turn this into a short-circuiting loop.
// A quick google search shows that rust doesn't have the ability to annotate specific code blocks
// as no-optimize; the only option is to insert direct assembly.

That is precisely the saturating-accumulator early exit described in this PR's new banner comment, and volatile_or_assign is the answer to it. As merged, ct.rs would run two different constant-time disciplines side by side, with the older one still flagged as an open question and the last sentence ("the only option is to insert direct assembly") now plainly contradicted by this PR's own argument.

Either extend the barrier to those two loops — which needs a per-width analogue, since the accumulator there is i64/i32 rather than usize — or, at minimum, replace the comment with a pointer to the new barrier plus a follow-up issue.

5. Minor wording

  • crypto/utils/src/ct.rs:582: "The cor operation that implements the optimization barrier".
  • Both new doc comments say "See the module comment above", but the block they refer to is a plain // banner rather than a //! module doc, so the reference does not resolve for anyone reading the rendered rustdoc.

6. The performance claim is unmeasured

"As a performance optimization, we compare one machine-word at a time" — crypto/utils has no benches/ directory and none is added here, so the 8x reduction in barrier round-trips is asserted rather than shown. Not a blocker, but if the word chunking is being justified on performance grounds it is cheap to back it up with a criterion bench.

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