Model one-operand imul as a signed multiply in the assembly checker (scrutineer #2514) - #2415
Draft
JasonGross wants to merge 1 commit into
Draft
Model one-operand imul as a signed multiply in the assembly checker (scrutineer #2514)#2415JasonGross wants to merge 1 commit into
JasonGross wants to merge 1 commit into
Conversation
Fixes scrutineer security finding #2514. The one-operand form of `imul r/m` (rdx:rax := rax * src, or ax := al * src for 8-bit operands) shared a single branch with `mul` in both the symbolic executor (src/Assembly/Symbolic.v, SymexNormalInstruction) and the concrete semantics (src/Assembly/WithBedrock/Semantics.v, DenoteNormalInstruction). Both computed the *unsigned* full product. On real x86-64 hardware `mul` is unsigned but one-operand `imul` is signed (Intel SDM Vol. 2A, IMUL), so the high word written to rdx (ah for 8-bit) differs whenever an operand has its top bit set; the low word agrees. For example, with rax = 0xffffffff00000001 (the top limb of the P-256 prime) and rcx = 3, `imul rcx` leaves rdx = 0xffffffffffffffff on hardware, but the model computed 0x2. Consequently the equivalence checker (`--hints-file`) could certify assembly that uses `imul rcx` where the reference computation needs the unsigned high word from `mul rcx` / `mulx`, i.e. accept non-equivalent assembly. The two- and three-operand forms of `imul` only write the low half and were already correct. The fix splits the shared branch: - `mul` keeps its previous (unsigned) behaviour unchanged. - One-operand `imul` now computes the high word as `keep s ((Z.signed s rax * Z.signed s src) >> s)` in the concrete semantics, and in the symbolic executor as `shr s (mulZ (signed rax) (signed src)) s` where `signed x` is spelled with existing operators as `addZ (add s x (2^(s-1))) (-(2^(s-1)))`, which is exactly `Z.signed s x` unfolded. The low word is still the (unsigned) `mulZ` product, which agrees with the signed one after truncation. Flags are havocked as before. The 8-bit form (ah:al) is handled by the same code. - The symbolic high-word computation lives in a new definition `SignedMulHigh` rather than inline in `SymexNormalInstruction`, with its own `same_reg_some_of_success` / `same_mem_addressed_of_success` instances in EquivalenceProofs.v (derived by typeclass resolution, like `Symeval`). Inlining it made the generic tactics of `SymexNormalInstruction_reg_same`/`_mem_same` blow up (they are exponential in the number of binds of a branch), and nesting the whole expression in a single `Symeval` made the `Qed` of `SymexNornalInstruction_R` take over an hour; with the separate definition all three files compile in about the same time as before. - SymbolicProofs.v unfolds `SignedMulHigh` alongside `SymexNormalInstruction` and gains a small case for the new branch relating the unfolded sign extension to `Z.signed`. A new test file src/Assembly/WithBedrock/SemanticsTests.v pins down the hardware values of `mul`/`imul` (64- and 8-bit, top bit set or clear) for both the concrete semantics and the symbolic executor; the imul cases fail against the previous model. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Rbn2gww3MGhvrh52fNjpD
JasonGross
force-pushed
the
fable/fix-2514-imul-signed
branch
from
September 2, 2026 03:51
5f3781b to
0297f97
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes scrutineer security finding #2514 (CWE-682): One-operand
imulis modelled as an unsigned multiply in both the symbolic executor and the concrete semantics, so itsrdxhigh word is wrong whenever an operand has its top bit set.The bug
src/Assembly/Symbolic.v(SymexNormalInstruction) andsrc/Assembly/WithBedrock/Semantics.v(DenoteNormalInstruction) handled the one-operand forms ofmulandimulin one shared branch,| (Syntax.mul | imul), [src2] =>, computing the unsigned full product and writing its high word tordx(ahfor 8-bit operands). On x86-64,mul r/m64is unsigned but one-operandimul r/m64is signed (Intel SDM Vol. 2A, IMUL). The low words agree; the high words differ whenever an operand has bit 63 set. E.g. withrax = 0xffffffff00000001(top limb of the P-256 prime) andrcx = 3, hardwareimul rcxgivesrdx = 0xffffffffffffffffwhile both models gave0x2.So the
--hints-fileequivalence checker accepted assembly usingimul rcxwhere the reference computation needs the unsigned high word (mul/mulx), i.e. it certified non-equivalent assembly. The two- and three-operandimulforms only write the low half and were already correct.The fix
The shared branch is split:
mulkeeps exactly its previous (unsigned) definition in both models.imul:Z.land (Z.shiftr (Z.signed s v1 * Z.signed s v2) s) (Z.ones s); the low word is stillv1 * v2(equal to the signed product after truncation);shr s (mulZ (signed a) (signed b)) swheresigned x := addZ (add s x (2^(s-1))) (-(2^(s-1))), which isZ.signed s xunfolded, expressed with the existing operators (no newopconstructor, so no changes to the rewriting passes or their proofs). This lives in a new definitionSignedMulHigh(next toSymexNormalInstruction) rather than inline; see below for why. The low word is the unsignedmulZproduct as before. Flags are havocked as before. The 8-bit form (ah:al) goes through the same code.SymbolicProofs.v:SymexNornalInstruction_RunfoldsSignedMulHightogether withSymexNormalInstructionand gains a four-line case relating the unfolded sign extension toZ.signed.EquivalenceProofs.v:same_reg_some_of_success/same_mem_addressed_of_successinstances forSignedMulHigh, derived bytypeclasses eautoexactly like theSymevalones, andTypeclasses Opaque SignedMulHigh.src/Assembly/WithBedrock/SemanticsTests.vpins down the hardware values ofmul/imul(64-bit and 8-bit; top bit set/clear; low and high words; untouched upper bytes for the 8-bit form) for both the concrete semantics and the symbolic executor (symbolic execution frominit_symbolic_state, theninterp_exprwith concrete values for therax/rcxsymbols). Theimulcases fail against the previous model (checked by compiling the test against the previously installed library:Unable to unify "Some 18446744073709551615" with "Some 2").Alternatives considered: adding a dedicated signed-multiply / sign-extension
op(rejected: everyopenumeration,Showinstance, and rewriting-pass proof would need a case, for an instruction the shipped corpus never uses); rejecting one-operandimuloutright (rejected: the correct semantics is expressible with existing operators and the proofs go through). Two formulations were tried and rejected on proof-performance grounds: nesting the whole high-word expression in a singleSymevalpre-expression inline in the branch made theQedofSymexNornalInstruction_Rtake over an hour in the kernel (the tactics themselves ran in seconds); splitting it into severalSymeval/Appbinds inline instead made the generic tactic behindSymexNormalInstruction_reg_same/_mem_sameinEquivalenceProofs.vrun for over 40 minutes (it is exponential in the number of binds in a branch; the unmodified file takes 50 s). Factoring the computation intoSignedMulHigh, which those proofs treat as a unit likeSymeval, brings every file back to its usual compile time.Verification
All compiled with the overlay recipe (
coqc -q -R src Crypto ...against the installedcoq-fiat-crypto-with-bedrock, withsrc/Assembly/WithBedrock/Semantics.vcompiled locally from HEAD since the installed copy differs):src/Assembly/WithBedrock/Semantics.v,src/Assembly/Symbolic.v(43 s),src/Assembly/Equivalence.v,src/Assembly/WithBedrock/SymbolicProofs.v(47 s; unmodified master: 41 s),src/Assembly/EquivalenceProofs.v(52 s; unmodified master: 50 s),src/Assembly/WithBedrock/Proofs.v(2 min),src/Assembly/WithBedrock/SemanticsTests.v(2 s): all compile.Binary chain
BoundsPipeline.v→PushButtonSynthesis/*.v→CLI.v→StandaloneOCamlMain.v→ExtractionOCaml/word_by_word_montgomery.v→ocamlfind ocamlopt: builds.End-to-end demo with that binary: took
fiat-amd64/fiat_p256_mul/seed0000000015492029_ratio16518.asmand rewrote its firstmulx r11, r10, [rax+0x10](whoserdxis reloaded immediately and whose flags are dead) asmov rcx, rdx; mov rax, [rsi+0x10]; mov r11, [rcx+0x10]; {mul|imul} r11; mov r10, rax; mov r11, rdx; mov rax, rcx, then ranword_by_word_montgomery p256 64 '2^256 - 2^224 + 2^192 + 2^96 - 1' mul --hints-file <file>:mul r11variantimul r11variantfiat_crypto word-by-word-montgomeryword_by_word_montgomeryEquivalence checking error: Unable to unify ...)Shipped CryptOpt corpus:
grepfinds no one-operandimulinfiat-amd64/(only the three-operand form andmulx). Re-ran thegentest.pyinvocations for all word-by-word Montgomery curves with the final rebuilt binary: p224, p256, secp256k1_montgomery, p384, p434 (× mul, square; 300 hints files in 10 invocations) all verify (exit 0; the p434 invocations take ~70-80 min each).Not verified: the full
make(CI will do it), the Haskell/JS extraction targets, and anything outside the cone listed above.🤖 Generated with Claude Code
https://claude.ai/code/session_016Rbn2gww3MGhvrh52fNjpD