Repository navigation
fix(tracer): reject MISC-MEM encodings that are not FENCE - #1902
Conversation
0xAndoroid
left a comment
There was a problem hiding this comment.
Verified on top of current main: merges clean, and the new tests plus rejects_invalid_encodings_with_exact_messages pass on the merged tree. Two comments, neither blocking.
Note: this reply was written by an AI after it reported the issue to me and I made the decision. If anything here is wrong, reply and I will review it myself.
Instruction::decode passed the word straight to FENCE::new with validation on, so its debug_assert fired on fence.i and on the reserved funct3 values instead of returning Err; with debug assertions off those words silently decoded as a plain FENCE.
…ge table Review follow-up: the funct3 = 001..111 rejections now live in rejects_invalid_encodings_with_exact_messages, and the dedicated test only asserts the fence positive case. The decoder and tracer comments now say that funct3 = 010 carries the Zicbom/Zicboz CBO instructions and only 011..111 is reserved.
178e7b0 to
79ecfae
Compare
|
Thanks for checking it on top of main. Rebased on current main (f830b54) and folded both in: the funct3 = 001..111 rejections are now entries in |
0xAndoroid
left a comment
There was a problem hiding this comment.
Re-review of 79ecfae: both earlier threads are addressed and resolved. One doc nit below, not blocking.
Note: this reply was written by an AI after it reported the issue to me and I made the decision. If anything here is wrong, reply and I will review it myself.
0x0ff0_000f is `fence iorw, iorw` (the bare `fence` mnemonic), not `fence rw, rw`, and FENCE is the only MISC-MEM instruction in RV64IMAC rather than the only encoding.
* tracer: reject LR encodings with a nonzero rs2 LR.W/LR.D require rs2 = 0, but both decoders matched LR on funct3/funct5 only. The tracer then handed the word to LRW::new/LRD::new, whose debug_assert panicked; release builds and the jolt-program decoder took it as a plain LR. Reject rs2 != 0 with an error in both, like #1902 does for MISC-MEM. Fixes #1957 * tracer: move LR rs2 tests into the decode table Drop the lrw.rs test module (the accept case was already covered by the encoding table), add the two rejection rows to decode_rejects_malformed_encodings_with_a_specific_reason, fix the AMO comment and use the rs2() helper in jolt-program.
Summary
MISC-MEM (opcode
0b0001111) is the only RV64 opcode whosefunct3neither decoder checks.tracer'sInstruction::decodepasses the word straight toFENCE::new(.., validate = true, ..), butFENCE'sMASK/MATCH(0x0000707f/0x0000000f) requirefunct3 == 0, so thedebug_assert_eq!insidedeclare_riscv_instr!'snewfires:decodepanics onfence.i(0x0000100f) and on the reservedfunct3 >= 2encodings instead of returning theErrits signature promises. Withdebug_assertionsoff the assertion disappears and those words silently decode as a plainFENCE.jolt-program's image decoder has the matching gap (0b0001111 => SourceInstructionKind::FENCE, nofunct3check), sodecode_text_sectionadmits reserved MISC-MEM words into the committed image as no-op FENCE rows, and the tracer then either aborts on them (debug) or no-ops them (release).FENCE.Ibelongs to Zifencei, which is outsideRV64IMAC_JOLT, andfunct3 >= 2is reserved. Every other arm of both decoders already rejects out of profile and reserved encodings with a typed error (decode_op_imm,decode_amo,decode_system, the branch/load/store tables, and the F/D opcodes0x07/0x27thatuncompress_rv64_instructionemits forC.FLD/C.FSD).tracer/src/instruction/csrrw.rsrecords the same contract for CSRs: decode must reject unsupported encodings with a typed error rather than panic.The reachable path is
Cpu::decode_and_cache, which callsInstruction::decodeon every fetch: a guest containingfence.i(hand writtenasm!("fence.i"), an icache sync helper, a__builtin___clear_cachelowering) aborts insideFENCE::newrather than surfacing a decode error.Changes
tracer/src/instruction/mod.rs: the MISC-MEM arm matches onfunct3and returnsErr("Invalid MISC-MEM funct3")for anything other thanFENCE, so no out of profile word reachesFENCE::new.crates/jolt-program/src/image/decode.rs: the same gate on the image decoding side, returningProgramError::MalformedImage("invalid MISC-MEM funct3").tracer/src/instruction/fence.rsasserts everyfunct3 != 0MISC-MEM word decodes to anErr(not a panic) and thatfence rw, rwstill decodes toFENCE;misc_mem_decodes_only_fencecovers the same range on the image decoder.Testing
cargo test -p jolt-program55 passed,--features image57 passed,cargo test -p tracer129 passed (the 12 failures are pre-existing in my environment: they shell out to thejoltCLI to build guests, and fail identically on unmodifiedmain, which is 127 passed / 12 failed).cargo clippy -p tracer --all-targets -- -D warningsandcargo clippy -p jolt-program --features image --all-targets -- -D warningsare clean;cargo fmt --checkis clean.Security Considerations
No proof soundness change: prover and verifier share one program image and one decoder, so the permissive decode did not let the two sides disagree. It tightens two robustness properties. The program image is the boundary deciding which
.textbyte patterns are legal RV64 for Jolt, and reserved MISC-MEM words were admitted as no-ops instead of rejected; andInstruction::decodeis a publicResultreturning API reached on every fetch, which aborted through an internal assertion in debug builds. Both now fail the same way, in the same place, in every build profile.Breaking Changes
Behavioral, not API: an ELF whose
.textcontainsfence.ior a reserved MISC-MEM encoding now fails image decoding withMalformedImage("invalid MISC-MEM funct3")instead of silently becoming a no-op in release builds (and panicking in debug builds). Zifencei is outsideRV64IMAC_JOLT, and no RISC-V ELF artifact in the tree contains such a word. If you would rather keep acceptingfence.ias an architectural no-op, mappingfunct3 = 001toFENCEexplicitly in both decoders is a one line change on top of this; the reserved values should be rejected either way.