Skip to content

module-lattice: annotate integer_division_remainder_used allows - #374

Open
vickyone0 wants to merge 2 commits into
RustCrypto:masterfrom
vickyone0:annotate-integer-division-allows
Open

module-lattice: annotate integer_division_remainder_used allows#374
vickyone0 wants to merge 2 commits into
RustCrypto:masterfrom
vickyone0:annotate-integer-division-allows

Conversation

@vickyone0

Copy link
Copy Markdown

Completes @tarcieri suggested pass from #25 — annotates the remaining allow(clippy::integer_division_remainder_used) sites with reason.

module-lattice/src/algebra.rs: BARRETT_MULTIPLIER is a const computed from Field::Q, so the division is const-evaluated and emits no runtime instruction — annotated reason = "constant".
module-lattice/tests/encode.rs (2 sites): annotated reason = "tests", matching the convention in the sibling test files.

All tests pass (including Wycheproof and ACVP vectors); cargo clippy --all-targets --all-features is clean on a fresh build.

@vickyone0

Copy link
Copy Markdown
Author

Following up with the verification @tarcieri and @bifurcation both asked for.

small_reduce (module-lattice/src/algebra.rs): checked the generated assembly for the masked comparison (x >= Q) across x86-64 and aarch64, at opt-level 0 and 3, with overflow-checks isolated as its own variable (this workspace has no [profile.release] override, so overflow-checks default off in release).

  • x86-64, O3, overflow-checks=off: cmp + cmovb — comparison feeds a conditional move, no jump.
  • x86-64, O0, overflow-checks=off: same shape via setae/sub/and, still zero jumps.
  • aarch64, O3, overflow-checks=off: cmp + csel — ARM's direct equivalent of cmov.
  • With overflow-checks on (Rust's dev-profile default), there's an additional jb to panic_const_sub_overflow, guarding the bare -. That branch is dead in practice — cx is always 0 or Q, so x - cx can't underflow — and it disappears entirely with overflow-checks off, so it's a debug-build artifact rather than a property of the algorithm. Switching x - (Self::Q & mask) to x.wrapping_sub(...) would remove it in debug builds too, making the branchless property hold regardless of profile. Happy to send that as a one-line follow-up if it's wanted.

Encode::<U1>::decode (the function the original report names): resolves to the generic Polynomial<F>::decodebyte_decode::<F, D> with D = U1. The only comparison in byte_decode is if D::USIZE == 12, gating the small_reduce call for the 12-bit case. D::USIZE is a typenum compile-time constant, so for D = U1 this is 1 == 12 — false at compile time, and the branch is eliminated entirely for this monomorphization. Confirmed the symbol exists in the compiled ml-kem binary (byte_decode<BaseField, UInt<UTerm, B1>>). What's left is a fixed 8-iteration bit-extraction loop with no data-dependent branch or trip count.

No secret-dependent branches found in either function across the configurations checked. Glad to open a small follow-up PR for the wrapping_sub hardening, or check another target/opt-level if there's one you'd want covered.

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.

1 participant