From a0f8806b546ddc14b062d9e61a3ffc2b3de17d64 Mon Sep 17 00:00:00 2001 From: YairVaknin-starkware <141148375+YairVaknin-starkware@users.noreply.github.com> Date: Mon, 27 Jan 2025 23:16:17 +0200 Subject: [PATCH] Minor `ret` decoding refactor (#1925) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Minor_ret_decoding_refactor * tests naming fix --------- Co-authored-by: Iñaki Garay --- CHANGELOG.md | 2 + vm/src/vm/decoding/decoder.rs | 151 ++++++++++++++++++++++------ vm/src/vm/runners/cairo_runner.rs | 2 +- vm/src/vm/security.rs | 158 +++++++++++++++++++++++++++--- 4 files changed, 269 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f401f4bea..4f40a256a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ #### Upcoming Changes +* refactor: Limit ret opcode decodeing to Cairo0's standards. [#1925](https://github.com/lambdaclass/cairo-vm/pull/1925) + #### [2.0.0-rc4] - 2025-01-23 * feat: implement `kzg` data availability hints [#1887](https://github.com/lambdaclass/cairo-vm/pull/1887) diff --git a/vm/src/vm/decoding/decoder.rs b/vm/src/vm/decoding/decoder.rs index 35a98e7812..8a018980f4 100644 --- a/vm/src/vm/decoding/decoder.rs +++ b/vm/src/vm/decoding/decoder.rs @@ -82,11 +82,11 @@ pub fn decode_instruction(encoded_instr: u64) -> Result return Err(VirtualMachineError::InvalidPcUpdate(pc_update_num)), }; - let res = match res_logic_num { - 0 if matches!(pc_update, PcUpdate::Jnz) => Res::Unconstrained, - 0 => Res::Op1, - 1 => Res::Add, - 2 => Res::Mul, + let res = match (res_logic_num, pc_update == PcUpdate::Jnz) { + (0, true) => Res::Unconstrained, + (0, false) => Res::Op1, + (1, false) => Res::Add, + (2, false) => Res::Mul, _ => return Err(VirtualMachineError::InvalidRes(res_logic_num)), }; @@ -98,17 +98,38 @@ pub fn decode_instruction(encoded_instr: u64) -> Result return Err(VirtualMachineError::InvalidOpcode(opcode_num)), }; - let ap_update = match ap_update_num { - 0 if matches!(opcode, Opcode::Call) => ApUpdate::Add2, - 0 => ApUpdate::Regular, - 1 => ApUpdate::Add, - 2 => ApUpdate::Add1, + let ap_update = match (ap_update_num, opcode == Opcode::Call) { + (0, true) => ApUpdate::Add2, + (0, false) => ApUpdate::Regular, + (1, false) => ApUpdate::Add, + (2, false) => ApUpdate::Add1, _ => return Err(VirtualMachineError::InvalidApUpdate(ap_update_num)), }; let fp_update = match opcode { - Opcode::Call => FpUpdate::APPlus2, - Opcode::Ret => FpUpdate::Dst, + Opcode::Call => { + if off0 != 0 + || off1 != 1 + || ap_update != ApUpdate::Add2 + || dst_register != Register::AP + || op0_register != Register::AP + { + return Err(VirtualMachineError::InvalidOpcode(opcode_num)); + }; + FpUpdate::APPlus2 + } + Opcode::Ret => { + if off0 != -2 + || off2 != -1 + || dst_register != Register::FP + || op1_addr != Op1Addr::FP + || res != Res::Op1 + || pc_update != PcUpdate::Jump + { + return Err(VirtualMachineError::InvalidOpcode(opcode_num)); + }; + FpUpdate::Dst + } _ => FpUpdate::Regular, }; @@ -199,56 +220,56 @@ mod decoder_test { #[test] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] - fn decode_flags_call_add_jmp_add_imm_fp_fp() { + fn decode_flags_nop_add_jmp_add_imm_fp_fp() { // 0| opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg // 15|14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0 - // | CALL| ADD| JUMP| ADD| IMM| FP| FP - // 0 0 0 1 0 1 0 0 1 0 1 0 0 1 1 1 - // 0001 0100 1010 0111 = 0x14A7; offx = 0 - let inst = decode_instruction(0x14A7800080008000).unwrap(); + // | NOp| ADD| JUMP| ADD| IMM| FP| FP + // 0 0 0 0 0 1 0 0 1 0 1 0 0 1 1 1 + // 0000 0100 1010 0111 = 0x04A7; offx = 0 + let inst = decode_instruction(0x04A7800080008000).unwrap(); assert_matches!(inst.dst_register, Register::FP); assert_matches!(inst.op0_register, Register::FP); assert_matches!(inst.op1_addr, Op1Addr::Imm); assert_matches!(inst.res, Res::Add); assert_matches!(inst.pc_update, PcUpdate::Jump); assert_matches!(inst.ap_update, ApUpdate::Add); - assert_matches!(inst.opcode, Opcode::Call); - assert_matches!(inst.fp_update, FpUpdate::APPlus2); + assert_matches!(inst.opcode, Opcode::NOp); + assert_matches!(inst.fp_update, FpUpdate::Regular); } #[test] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] - fn decode_flags_ret_add1_jmp_rel_mul_fp_ap_ap() { + fn decode_flags_nop_add1_jmp_rel_mul_fp_ap_ap() { // 0| opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg // 15|14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0 - // | RET| ADD1| JUMP_REL| MUL| FP| AP| AP - // 0 0 1 0 1 0 0 1 0 1 0 0 1 0 0 0 - // 0010 1001 0100 1000 = 0x2948; offx = 0 - let inst = decode_instruction(0x2948800080008000).unwrap(); + // | NOp| ADD1| JUMP_REL| MUL| FP| AP| AP + // 0 0 0 0 1 0 0 1 0 1 0 0 1 0 0 0 + // 0000 1001 0100 1000 = 0x0948; offx = 0 + let inst = decode_instruction(0x0948800080008000).unwrap(); assert_matches!(inst.dst_register, Register::AP); assert_matches!(inst.op0_register, Register::AP); assert_matches!(inst.op1_addr, Op1Addr::FP); assert_matches!(inst.res, Res::Mul); assert_matches!(inst.pc_update, PcUpdate::JumpRel); assert_matches!(inst.ap_update, ApUpdate::Add1); - assert_matches!(inst.opcode, Opcode::Ret); - assert_matches!(inst.fp_update, FpUpdate::Dst); + assert_matches!(inst.opcode, Opcode::NOp); + assert_matches!(inst.fp_update, FpUpdate::Regular); } #[test] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] - fn decode_flags_assrt_add_jnz_mul_ap_ap_ap() { + fn decode_flags_assrt_add_regular_mul_ap_ap_ap() { // 0| opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg // 15|14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0 - // |ASSRT_EQ| ADD| JNZ| MUL| AP| AP| AP - // 0 1 0 0 1 0 1 0 0 1 0 1 0 0 0 0 - // 0100 1010 0101 0000 = 0x4A50; offx = 0 - let inst = decode_instruction(0x4A50800080008000).unwrap(); + // |ASSRT_EQ| ADD| REGULAR| MUL| AP| AP| AP + // 0 1 0 0 1 0 0 0 0 1 0 1 0 0 0 0 + // 0100 1000 0101 0000 = 0x4850; offx = 0 + let inst = decode_instruction(0x4850800080008000).unwrap(); assert_matches!(inst.dst_register, Register::AP); assert_matches!(inst.op0_register, Register::AP); assert_matches!(inst.op1_addr, Op1Addr::AP); assert_matches!(inst.res, Res::Mul); - assert_matches!(inst.pc_update, PcUpdate::Jnz); + assert_matches!(inst.pc_update, PcUpdate::Regular); assert_matches!(inst.ap_update, ApUpdate::Add1); assert_matches!(inst.opcode, Opcode::AssertEq); assert_matches!(inst.fp_update, FpUpdate::Regular); @@ -305,4 +326,70 @@ mod decoder_test { assert_eq!(inst.off1, 0); assert_eq!(inst.off2, 1); } + + #[test] + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] + fn decode_ret_cairo_standard() { + // 0| opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg + // 15|14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0 + // | RET| REGULAR| JUMP| Op1| FP| FP| FP + // 0 0 1 0 0 0 0 0 1 0 0 0 1 0 1 1 + // 0010 0000 1000 1011 = 0x208b; off0 = -2, off1 = -1 + let inst = decode_instruction(0x208b7fff7fff7ffe).unwrap(); + assert_matches!(inst.opcode, Opcode::Ret); + assert_matches!(inst.off0, -2); + assert_matches!(inst.off1, -1); + assert_matches!(inst.dst_register, Register::FP); + assert_matches!(inst.op0_register, Register::FP); + assert_matches!(inst.op1_addr, Op1Addr::FP); + assert_matches!(inst.res, Res::Op1); + assert_matches!(inst.pc_update, PcUpdate::Jump); + assert_matches!(inst.ap_update, ApUpdate::Regular); + assert_matches!(inst.fp_update, FpUpdate::Dst); + } + + #[test] + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] + fn decode_call_cairo_standard() { + // 0| opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg + // 15|14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0 + // | CALL| Add2| JumpRel| Op1| FP| FP| FP + // 0 0 0 1 0 0 0 1 0 0 0 0 0 1 0 0 + // 0001 0001 0000 0100 = 0x208b; off0 = 0, off1 = 1 + let inst = decode_instruction(0x1104800180018000).unwrap(); + assert_matches!(inst.opcode, Opcode::Call); + assert_matches!(inst.off0, 0); + assert_matches!(inst.off1, 1); + assert_matches!(inst.dst_register, Register::AP); + assert_matches!(inst.op0_register, Register::AP); + assert_matches!(inst.op1_addr, Op1Addr::Imm); + assert_matches!(inst.res, Res::Op1); + assert_matches!(inst.pc_update, PcUpdate::JumpRel); + assert_matches!(inst.ap_update, ApUpdate::Add2); + assert_matches!(inst.fp_update, FpUpdate::APPlus2); + } + + #[test] + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] + fn decode_ret_opcode_error() { + // 0| opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg + // 15|14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0 + // | RET| REGULAR| JUMP| Op1| FP| FP| FP + // 0 0 1 0 0 0 0 0 1 0 0 0 1 0 1 1 + // 0010 0000 1000 1011 = 0x208b; off0 = -1, off1 = -1 + let error = decode_instruction(0x208b7fff7fff7fff); + assert_matches!(error, Err(VirtualMachineError::InvalidOpcode(2))); + } + + #[test] + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] + fn decode_call_opcode_error() { + // 0| opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg + // 15|14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0 + // | CALL| Add2| JumpRel| Op1| FP| FP| FP + // 0 0 0 1 0 0 0 1 0 0 0 0 0 1 0 0 + // 0001 0001 0000 0100 = 0x208b; off0 = 1, off1 = 1 + let error = decode_instruction(0x1104800180018001); + assert_matches!(error, Err(VirtualMachineError::InvalidOpcode(1))); + } } diff --git a/vm/src/vm/runners/cairo_runner.rs b/vm/src/vm/runners/cairo_runner.rs index 65209c35de..07ee837cde 100644 --- a/vm/src/vm/runners/cairo_runner.rs +++ b/vm/src/vm/runners/cairo_runner.rs @@ -154,7 +154,7 @@ pub struct CairoRunner { run_ended: bool, segments_finalized: bool, execution_public_memory: Option>, - runner_mode: RunnerMode, + pub(crate) runner_mode: RunnerMode, pub relocated_memory: Vec>, pub exec_scopes: ExecutionScopes, pub relocated_trace: Option>, diff --git a/vm/src/vm/security.rs b/vm/src/vm/security.rs index 2967a641bf..9914c36681 100644 --- a/vm/src/vm/security.rs +++ b/vm/src/vm/security.rs @@ -4,9 +4,10 @@ use num_traits::ToPrimitive; use super::{ errors::{runner_errors::RunnerError, vm_errors::VirtualMachineError}, - runners::cairo_runner::CairoRunner, + runners::cairo_runner::{CairoRunner, RunnerMode}, }; use crate::types::relocatable::MaybeRelocatable; +use crate::Felt252; /// Verify that the completed run in a runner is safe to be relocated and be /// used by other Cairo programs. @@ -79,6 +80,41 @@ pub fn verify_secure_runner( builtin.run_security_checks(&runner.vm)?; } + // Validate ret FP. + let initial_fp = runner.get_initial_fp().ok_or_else(|| { + VirtualMachineError::Other(anyhow::anyhow!( + "Failed to retrieve the initial_fp: it is None. \ + The initial_fp field should be initialized after running the entry point." + )) + })?; + let ret_fp_addr = (initial_fp - 2).map_err(VirtualMachineError::Math)?; + let ret_fp = runner.vm.get_maybe(&ret_fp_addr).ok_or_else(|| { + VirtualMachineError::Other(anyhow::anyhow!( + "Ret FP address is not in memory: {ret_fp_addr}" + )) + })?; + let final_fp = runner.vm.get_fp(); + match ret_fp { + MaybeRelocatable::RelocatableValue(value) => { + if runner.runner_mode == RunnerMode::ProofModeCanonical && value != final_fp { + return Err(VirtualMachineError::Other(anyhow::anyhow!( + "Return FP is not equal to final FP: ret_f={ret_fp}, final_fp={final_fp}" + ))); + } + if runner.runner_mode == RunnerMode::ExecutionMode && value.offset != final_fp.offset { + return Err(VirtualMachineError::Other(anyhow::anyhow!( + "Return FP offset is not equal to final FP offset: ret_f={ret_fp}, final_fp={final_fp}" + ))); + } + } + MaybeRelocatable::Int(value) => { + if Felt252::from(final_fp.offset) != value { + return Err(VirtualMachineError::Other(anyhow::anyhow!( + "Return FP felt value is not equal to final FP offset: ret_fp={ret_fp}, final_fp={final_fp}" + ))); + } + } + } Ok(()) } @@ -115,9 +151,13 @@ mod test { fn verify_secure_runner_empty_memory() { let program = program!(main = Some(0),); let mut runner = cairo_runner!(program); - runner.initialize(false).unwrap(); - runner.vm.segments.compute_effective_sizes(); + // runner.vm.segments.compute_effective_sizes(); + let mut hint_processor = BuiltinHintProcessor::new_empty(); + runner.end_run(false, false, &mut hint_processor).unwrap(); + // At the end of the run, the ret_fp should be the base of the new ret_fp segment we added + // to the stack at the start of the run. + runner.vm.run_context.fp = 0; assert_matches!(verify_secure_runner(&runner, true, None), Ok(())); } @@ -145,10 +185,12 @@ mod test { let mut runner = cairo_runner!(program); runner.initialize(false).unwrap(); - - runner.vm.segments = segments![((0, 0), 100)]; + // We insert (1, 0) for ret_fp segment. + runner.vm.segments = segments![((0, 0), 100), ((1, 0), 0)]; runner.vm.segments.segment_used_sizes = Some(vec![1]); - + // At the end of the run, the ret_fp should be the base of the new ret_fp segment we added + // to the stack at the start of the run. + runner.vm.run_context.fp = 0; assert_matches!(verify_secure_runner(&runner, true, Some(1)), Ok(())); } @@ -179,8 +221,11 @@ mod test { let mut hint_processor = BuiltinHintProcessor::new_empty(); runner.end_run(false, false, &mut hint_processor).unwrap(); runner.vm.builtin_runners[0].set_stop_ptr(1); - - runner.vm.segments.memory = memory![((2, 0), 1)]; + // Adding ((1, 1), (3, 0)) to the memory segment to simulate the ret_fp_segment. + runner.vm.segments.memory = memory![((2, 0), 1), ((1, 1), (3, 0))]; + // At the end of the run, the ret_fp should be the base of the new ret_fp segment we added + // to the stack at the start of the run. + runner.vm.run_context.fp = 0; runner.vm.segments.segment_used_sizes = Some(vec![0, 0, 1, 0]); assert_matches!(verify_secure_runner(&runner, true, None), Ok(())); @@ -202,13 +247,18 @@ mod test { let mut runner = cairo_runner!(program); runner.initialize(false).unwrap(); + // We insert (1, 0) for ret_fp segment. runner.vm.segments.memory = memory![ ((0, 0), (1, 0)), ((0, 1), (2, 1)), ((0, 2), (3, 2)), - ((0, 3), (4, 3)) + ((0, 3), (4, 3)), + ((1, 0), 0) ]; runner.vm.segments.segment_used_sizes = Some(vec![5, 1, 2, 3, 4]); + // At the end of the run, the ret_fp should be the base of the new ret_fp segment we added + // to the stack at the start of the run. + runner.vm.run_context.fp = 0; assert_matches!(verify_secure_runner(&runner, true, None), Ok(())); } @@ -228,14 +278,19 @@ mod test { let mut runner = cairo_runner!(program); + // We insert (1, 0) for ret_fp segment. runner.initialize(false).unwrap(); runner.vm.segments.memory = memory![ ((0, 1), (1, 0)), ((0, 2), (2, 1)), ((0, 3), (3, 2)), - ((-1, 0), (1, 2)) + ((-1, 0), (1, 2)), + ((1, 0), 0) ]; runner.vm.segments.segment_used_sizes = Some(vec![5, 1, 2, 3, 4]); + // At the end of the run, the ret_fp should be the base of the new ret_fp segment we added + // to the stack at the start of the run. + runner.vm.run_context.fp = 0; assert_matches!(verify_secure_runner(&runner, true, None), Ok(())); } @@ -256,12 +311,14 @@ mod test { let mut runner = cairo_runner!(program); runner.initialize(false).unwrap(); + // We insert (1, 0) for ret_fp segment. runner.vm.segments.memory = memory![ ((0, 0), (1, 0)), ((0, 1), (2, 1)), ((0, 2), (-3, 2)), ((0, 3), (4, 3)), - ((-1, 0), (1, 2)) + ((-1, 0), (1, 2)), + ((1, 0), 0) ]; runner.vm.segments.segment_used_sizes = Some(vec![5, 1, 2, 3, 4]); @@ -272,4 +329,83 @@ mod test { )) if *bx == relocatable!(-3, 2) ); } + + #[test] + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] + fn verify_secure_runner_missing_initial_fp_error() { + let program = program!(main = Some(0),); + let mut runner = cairo_runner!(program); + // init program base to avoid other errors. + runner.program_base = Some(runner.vm.add_memory_segment()); + + assert_matches!( + verify_secure_runner(&runner, true, None), + Err(VirtualMachineError::Other(ref err)) if err.to_string().contains("Failed to retrieve the initial_fp: it is None") + ); + } + + #[test] + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] + fn verify_secure_runner_ret_fp_address_not_in_memory() { + let program = program!(main = Some(0),); + let mut runner = cairo_runner!(program); + runner.initialize(false).unwrap(); + // simulate empty memory. + runner.vm.segments.memory = crate::vm::vm_memory::memory::Memory::new(); + assert_matches!( + verify_secure_runner(&runner, true, None), + Err(VirtualMachineError::Other(ref err)) + if err.to_string().contains("Ret FP address is not in memory") + ); + } + + #[test] + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] + fn verify_secure_runner_return_fp_not_equal_final_fp_proof_mode() { + let program = program!(main = Some(0),); + let mut runner = cairo_runner!(program); + runner.initialize(false).unwrap(); + + // Set the runner mode to ProofModeCanonical, so we expect + // the return FP to be equal to final_fp. + runner.runner_mode = RunnerMode::ProofModeCanonical; + + assert_matches!( + verify_secure_runner(&runner, true, None), + Err(VirtualMachineError::Other(ref err)) + if err.to_string().contains("Return FP is not equal to final FP") + ); + } + + #[test] + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] + fn verify_secure_runner_return_fp_offset_not_equal_final_fp_offset_execution_mode() { + let program = program!(main = Some(0),); + let mut runner = cairo_runner!(program); + runner.initialize(false).unwrap(); + + // ExecutionMode only requires offset equality, not the entire relocatable. + assert_matches!( + verify_secure_runner(&runner, true, None), + Err(VirtualMachineError::Other(ref err)) + if err.to_string().contains("Return FP offset is not equal to final FP offset") + ); + } + + #[test] + #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] + fn verify_secure_runner_return_fp_felt_not_equal_final_fp_offse() { + let program = program!(main = Some(0),); + let mut runner = cairo_runner!(program); + runner.initialize(false).unwrap(); + // Insert Felt(0) as the return FP. + runner.vm.segments.memory = memory![((1, 0), 0)]; + + // ExecutionMode only requires offset equality, not the entire relocatable. + assert_matches!( + verify_secure_runner(&runner, true, None), + Err(VirtualMachineError::Other(ref err)) + if err.to_string().contains("Return FP felt value is not equal to final FP offset") + ); + } }