From 0dafed9a7c8beeffb8b70201fbdb41b108bb8ade Mon Sep 17 00:00:00 2001 From: Robin Salen Date: Mon, 27 May 2024 12:54:54 -0400 Subject: [PATCH 1/4] fix: check invalid offsets --- .../src/cpu/kernel/asm/memory/metadata.asm | 9 +++++++++ .../src/cpu/kernel/asm/memory/syscalls.asm | 18 ++++++++++++------ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/evm_arithmetization/src/cpu/kernel/asm/memory/metadata.asm b/evm_arithmetization/src/cpu/kernel/asm/memory/metadata.asm index a00c57028..4ac78b143 100644 --- a/evm_arithmetization/src/cpu/kernel/asm/memory/metadata.asm +++ b/evm_arithmetization/src/cpu/kernel/asm/memory/metadata.asm @@ -433,6 +433,15 @@ zero_hash: // stack: (empty) %endmacro +// Faults if the given offset is "out of range", i.e. does not fit in a single memory limb. +%macro ensure_offset_in_range + // stack: offset + %gt_const(0xffffffff) + // stack: is_overflow + %jumpi(fault_exception) + // stack: (empty) +%endmacro + // Convenience macro for checking if the current context is static. // Called before state-changing opcodes. %macro check_static diff --git a/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm b/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm index 97607d191..661b580f7 100644 --- a/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm +++ b/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm @@ -112,7 +112,7 @@ calldataload_large_offset: codecopy_within_bounds: // stack: total_size, segment, src_ctx, kexit_info, dest_offset, offset, size POP -wcopy_within_bounds: +global wcopy_within_bounds: // TODO: rework address creation to have less stack manipulation overhead // stack: segment, src_ctx, kexit_info, dest_offset, offset, size GET_CONTEXT @@ -123,17 +123,17 @@ wcopy_within_bounds: // stack: DST, SRC, size, wcopy_after, kexit_info %jump(memcpy_bytes) -wcopy_empty: +global wcopy_empty: // stack: Gverylow, kexit_info, dest_offset, offset, size %charge_gas %stack (kexit_info, dest_offset, offset, size) -> (kexit_info) EXIT_KERNEL -codecopy_large_offset: +global codecopy_large_offset: // stack: total_size, src_ctx, kexit_info, dest_offset, offset, size %pop2 -wcopy_large_offset: +global wcopy_large_offset: // offset is larger than the size of the {CALLDATA,CODE,RETURNDATA}. So we just have to write zeros. // stack: kexit_info, dest_offset, offset, size GET_CONTEXT @@ -142,7 +142,7 @@ wcopy_large_offset: %build_address %jump(memset) -wcopy_after: +global wcopy_after: // stack: kexit_info EXIT_KERNEL @@ -225,6 +225,12 @@ global sys_mcopy: // stack: kexit_info, dest_offset, offset, size %wcopy_charge_gas + // stack: kexit_info, dest_offset, offset, size + DUP2 + %ensure_offset_in_range + DUP3 + %ensure_offset_in_range + %stack (kexit_info, dest_offset, offset, size) -> (dest_offset, size, kexit_info, dest_offset, offset, size) %add_or_fault // stack: expanded_num_bytes, kexit_info, dest_offset, offset, size, kexit_info @@ -255,7 +261,7 @@ global sys_mcopy: // stack: segment, context, kexit_info, dest_offset, offset, size %jump(wcopy_within_bounds) -mcopy_with_overlap: +global mcopy_with_overlap: // We do have an overlap between the SRC and DST ranges. We will first copy the overlapping segment // (i.e. end of the copy portion), then copy the remaining (i.e. beginning) portion. From 8db1c1d065d02531adcdf4a6fc1d225434833aa7 Mon Sep 17 00:00:00 2001 From: Robin Salen Date: Mon, 27 May 2024 15:11:45 -0400 Subject: [PATCH 2/4] Update per Will's comment --- .../src/cpu/kernel/asm/memory/metadata.asm | 9 --------- .../src/cpu/kernel/asm/memory/syscalls.asm | 11 +++++------ 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/evm_arithmetization/src/cpu/kernel/asm/memory/metadata.asm b/evm_arithmetization/src/cpu/kernel/asm/memory/metadata.asm index 4ac78b143..a00c57028 100644 --- a/evm_arithmetization/src/cpu/kernel/asm/memory/metadata.asm +++ b/evm_arithmetization/src/cpu/kernel/asm/memory/metadata.asm @@ -433,15 +433,6 @@ zero_hash: // stack: (empty) %endmacro -// Faults if the given offset is "out of range", i.e. does not fit in a single memory limb. -%macro ensure_offset_in_range - // stack: offset - %gt_const(0xffffffff) - // stack: is_overflow - %jumpi(fault_exception) - // stack: (empty) -%endmacro - // Convenience macro for checking if the current context is static. // Called before state-changing opcodes. %macro check_static diff --git a/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm b/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm index 661b580f7..d14bfc441 100644 --- a/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm +++ b/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm @@ -225,18 +225,17 @@ global sys_mcopy: // stack: kexit_info, dest_offset, offset, size %wcopy_charge_gas - // stack: kexit_info, dest_offset, offset, size - DUP2 - %ensure_offset_in_range - DUP3 - %ensure_offset_in_range - %stack (kexit_info, dest_offset, offset, size) -> (dest_offset, size, kexit_info, dest_offset, offset, size) %add_or_fault // stack: expanded_num_bytes, kexit_info, dest_offset, offset, size, kexit_info DUP1 %ensure_reasonable_offset %update_mem_bytes + %stack (kexit_info, dest_offset, offset, size) -> (offset, size, kexit_info, dest_offset, offset, size) + %add_or_fault + DUP1 %ensure_reasonable_offset + %update_mem_bytes + // stack: kexit_info, dest_offset, offset, size DUP3 DUP3 EQ // stack: dest_offset = offset, kexit_info, dest_offset, offset, size From 76483fb1170ca9fd110b8cd106cf9cd12b27c3d8 Mon Sep 17 00:00:00 2001 From: Robin Salen Date: Wed, 29 May 2024 09:13:51 -0400 Subject: [PATCH 3/4] Minor --- .../src/cpu/kernel/asm/memory/syscalls.asm | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm b/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm index d14bfc441..4c90850ca 100644 --- a/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm +++ b/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm @@ -112,7 +112,7 @@ calldataload_large_offset: codecopy_within_bounds: // stack: total_size, segment, src_ctx, kexit_info, dest_offset, offset, size POP -global wcopy_within_bounds: +wcopy_within_bounds: // TODO: rework address creation to have less stack manipulation overhead // stack: segment, src_ctx, kexit_info, dest_offset, offset, size GET_CONTEXT @@ -123,17 +123,17 @@ global wcopy_within_bounds: // stack: DST, SRC, size, wcopy_after, kexit_info %jump(memcpy_bytes) -global wcopy_empty: +wcopy_empty: // stack: Gverylow, kexit_info, dest_offset, offset, size %charge_gas %stack (kexit_info, dest_offset, offset, size) -> (kexit_info) EXIT_KERNEL -global codecopy_large_offset: +codecopy_large_offset: // stack: total_size, src_ctx, kexit_info, dest_offset, offset, size %pop2 -global wcopy_large_offset: +wcopy_large_offset: // offset is larger than the size of the {CALLDATA,CODE,RETURNDATA}. So we just have to write zeros. // stack: kexit_info, dest_offset, offset, size GET_CONTEXT @@ -142,7 +142,7 @@ global wcopy_large_offset: %build_address %jump(memset) -global wcopy_after: +wcopy_after: // stack: kexit_info EXIT_KERNEL @@ -260,7 +260,7 @@ global sys_mcopy: // stack: segment, context, kexit_info, dest_offset, offset, size %jump(wcopy_within_bounds) -global mcopy_with_overlap: +mcopy_with_overlap: // We do have an overlap between the SRC and DST ranges. We will first copy the overlapping segment // (i.e. end of the copy portion), then copy the remaining (i.e. beginning) portion. From a9f095abda6d463f4e26898805f58abc340050fc Mon Sep 17 00:00:00 2001 From: Robin Salen <30937548+Nashtare@users.noreply.github.com> Date: Wed, 29 May 2024 11:54:28 -0400 Subject: [PATCH 4/4] fix(cancun): revamp `MCOPY` with overlapping (#254) --- .../src/cpu/kernel/asm/memory/memcpy.asm | 53 +++++++- .../src/cpu/kernel/asm/memory/syscalls.asm | 75 +++++++----- .../src/cpu/kernel/tests/mcopy.rs | 114 ++++++++++++++++++ .../src/cpu/kernel/tests/mod.rs | 1 + 4 files changed, 212 insertions(+), 31 deletions(-) create mode 100644 evm_arithmetization/src/cpu/kernel/tests/mcopy.rs diff --git a/evm_arithmetization/src/cpu/kernel/asm/memory/memcpy.asm b/evm_arithmetization/src/cpu/kernel/asm/memory/memcpy.asm index a7819bf6e..09f686965 100644 --- a/evm_arithmetization/src/cpu/kernel/asm/memory/memcpy.asm +++ b/evm_arithmetization/src/cpu/kernel/asm/memory/memcpy.asm @@ -45,7 +45,7 @@ global memcpy_bytes: %lt_const(0x21) // stack: count <= 32, DST, SRC, count, retdest %jumpi(memcpy_bytes_finish) - + // We will pack 32 bytes into a U256 from the source, and then unpack it at the destination. // Copy the next chunk of bytes. // stack: DST, SRC, count, retdest @@ -104,3 +104,54 @@ memcpy_finish: %jump(memcpy_bytes) %%after: %endmacro + +// Similar logic to memcpy_bytes, but proceeding the sequence in the backwards direction. +// Note that this is slightly heavier than the regular `memcpy_bytes`. +global memcpy_bytes_backwards: + // stack: DST, SRC, count, retdest + + // Handle small case + DUP3 + // stack: count, DST, SRC, count, retdest + %lt_const(0x21) + // stack: count <= 32, DST, SRC, count, retdest + %jumpi(memcpy_bytes_finish) + + // We will pack 32 bytes into a U256 from the source, and then unpack it at the destination. + // Copy the next chunk of bytes. + // stack: DST, SRC, count, retdest + PUSH 0x20 + DUP3 + // stack: SRC, 32, DST, SRC, count, retdest + MLOAD_32BYTES + // stack: value, DST, SRC, count, retdest + SWAP1 + // stack: DST, value, SRC, count, retdest + MSTORE_32BYTES_32 + // stack: DST'', SRC, count, retdest + + // Decrement count by 32. + SWAP2 + %sub_const(0x20) + SWAP2 + + // Decrement DST'' by 32 (from `MSTORE_32BYTES_32` increment) + min(32, count') for the next chunk. + // Decrement SRC by min(32, count'). + // stack: DST'', SRC, count', retdest + DUP3 PUSH 0x20 %min + // stack: min(32, count'), DST'', SRC, count', retdest + DUP1 %add_const(0x20) + // stack: 32 + min(32, count'), min(32, count'), DST'', SRC, count', retdest + SWAP3 SUB + // stack: SRC' = SRC-min(32, count'), DST'', 32 + min(32, count'), count', retdest + SWAP2 SWAP1 SUB + // stack: DST' = DST''-(32+min(32, count')), SRC', count', retdest + + // Continue the loop. + %jump(memcpy_bytes_backwards) + +%macro memcpy_bytes_backwards + %stack (dst, src, count) -> (dst, src, count, %%after) + %jump(memcpy_bytes_backwards) +%%after: +%endmacro diff --git a/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm b/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm index 4c90850ca..de04c111e 100644 --- a/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm +++ b/evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm @@ -244,46 +244,61 @@ global sys_mcopy: // stack: kexit_info, dest_offset, offset, size GET_CONTEXT PUSH @SEGMENT_MAIN_MEMORY - DUP6 DUP6 ADD - // stack: offset + size, segment, context, kexit_info, dest_offset, offset, size - DUP5 LT - // stack: dest_offset < offset + size, segment, context, kexit_info, dest_offset, offset, size - DUP6 DUP6 GT - // stack: dest_offset > offset, dest_offset < offset + size, segment, context, kexit_info, dest_offset, offset, size + + DUP5 DUP5 LT + // stack: dest_offset < offset, kexit_info, dest_offset, offset, size + %jumpi(wcopy_within_bounds) + + // stack: segment, context, kexit_info, dest_offset, offset, size + DUP6 PUSH 32 %min + // stack: shift=min(size, 32), segment, context, kexit_info, dest_offset, offset, size + DUP6 DUP8 ADD + // stack: offset + size, shift, segment, context, kexit_info, dest_offset, offset, size + DUP6 LT + // stack: dest_offset < offset + size, shift, segment, context, kexit_info, dest_offset, offset, size + DUP2 + // stack: shift, dest_offset < offset + size, shift, segment, context, kexit_info, dest_offset, offset, size + DUP9 GT + // stack: size > shift, dest_offset < offset + size, shift, segment, context, kexit_info, dest_offset, offset, size MUL // AND - // stack: (dest_offset > offset) && (dest_offset < offset + size), segment, context, kexit_info, dest_offset, offset, size + // stack: (size > shift) && (dest_offset < offset + size), shift, segment, context, kexit_info, dest_offset, offset, size - // If both conditions are satisfied, that means we will get an overlap, in which case we need to process the copy - // in two chunks to prevent overwriting memory data before reading it. + // If the conditions `size > shift` and `dest_offset < offset + size` are satisfied, that means + // we will get an overlap that will overwrite some SRC data. In that case, we will proceed to the + // memcpy in the backwards direction to never overwrite the SRC section before it has been read. %jumpi(mcopy_with_overlap) - // stack: segment, context, kexit_info, dest_offset, offset, size + // Otherwise, we either have `SRC` < `DST`, or a small enough `size` that a single loop of + // `memcpy_bytes` suffices and does not risk to overwrite `SRC` data before being read. + // stack: shift, segment, context, kexit_info, dest_offset, offset, size + POP %jump(wcopy_within_bounds) mcopy_with_overlap: - // We do have an overlap between the SRC and DST ranges. We will first copy the overlapping segment - // (i.e. end of the copy portion), then copy the remaining (i.e. beginning) portion. - - // stack: segment, context, kexit_info, dest_offset, offset, size - DUP5 DUP5 SUB - // stack: remaining_size = dest_offset - offset, segment, context, kexit_info, dest_offset, offset, size - DUP1 DUP8 - SUB // overlapping_size = size - remaining_size - // stack: overlapping_size, remaining_size, segment, context, kexit_info, dest_offset, offset, size - - // Shift the initial offsets to copy the overlapping segment first. - DUP2 DUP8 ADD - // stack: offset_first_copy, overlapping_size, remaining_size, segment, context, kexit_info, dest_offset, offset, size - DUP3 DUP8 ADD - // stack: dest_offset_first_copy, offset_first_copy, overlapping_size, remaining_size, segment, context, kexit_info, dest_offset, offset, size - - %stack (dest_offset_first_copy, offset_first_copy, overlapping_size, remaining_size, segment, context, kexit_info, dest_offset, offset, size) -> - (context, segment, offset_first_copy, segment, dest_offset_first_copy, context, overlapping_size, wcopy_within_bounds, segment, context, kexit_info, dest_offset, offset, remaining_size) + // We do have an overlap between the SRC and DST ranges. + // We will proceed to `memcpy` in the backwards direction to prevent overwriting unread SRC data. + // For this, we need to update `offset` and `dest_offset` to their final position, corresponding + // to `x + size - min(32, size)`. + + // stack: shift=min(size, 32), segment, context, kexit_info, dest_offset, offset, size + DUP1 + // stack: shift, shift, segment, context, kexit_info, dest_offset, offset, size + DUP8 DUP8 ADD + // stack: offset+size, shift, shift, segment, context, kexit_info, dest_offset, offset, size + SUB + // stack: offset'=offset+size-shift, shift, segment, context, kexit_info, dest_offset, offset, size + SWAP5 DUP8 ADD + // stack: dest_offset+size, shift, segment, context, kexit_info, offset', offset, size + SUB + // stack: dest_offset'=dest_offset+size-shift, segment, context, kexit_info, offset', offset, size + + %stack (next_dst_offset, segment, context, kexit_info, new_offset, offset, size) -> + (context, segment, new_offset, segment, next_dst_offset, context, size, wcopy_after, kexit_info) %build_address // SRC SWAP3 %build_address // DST - // stack: DST, SRC, overlapping_size, wcopy_within_bounds, segment, context, kexit_info, dest_offset, offset, remaining_size - %jump(memcpy_bytes) + // stack: DST, SRC, size, wcopy_after, kexit_info + %jump(memcpy_bytes_backwards) mcopy_empty: // kexit_info, dest_offset, offset, size diff --git a/evm_arithmetization/src/cpu/kernel/tests/mcopy.rs b/evm_arithmetization/src/cpu/kernel/tests/mcopy.rs new file mode 100644 index 000000000..07ca1bda8 --- /dev/null +++ b/evm_arithmetization/src/cpu/kernel/tests/mcopy.rs @@ -0,0 +1,114 @@ +use anyhow::Result; +use ethereum_types::U256; +use hex_literal::hex; +use itertools::Itertools; +use plonky2::field::goldilocks_field::GoldilocksField as F; + +use crate::cpu::kernel::constants::context_metadata::ContextMetadata; +use crate::cpu::kernel::interpreter::Interpreter; +use crate::memory::segments::Segment; +use crate::testing_utils::init_logger; + +fn test_mcopy( + dest_offset: usize, + offset: usize, + size: usize, + pre_memory: &[u8], + post_memory: &[u8], +) -> Result<()> { + init_logger(); + + let sys_mcopy = crate::cpu::kernel::aggregator::KERNEL.global_labels["sys_mcopy"]; + let kexit_info = U256::from(0xdeadbeefu32) + (U256::from(u64::from(true)) << 32); + let initial_stack = vec![size.into(), offset.into(), dest_offset.into(), kexit_info]; + + let mut interpreter: Interpreter = Interpreter::new(sys_mcopy, initial_stack); + interpreter.set_context_metadata_field( + 0, + ContextMetadata::GasLimit, + U256::from(1000000000000u64), + ); + + let pre_memory: Vec = pre_memory.iter().map(|&b| b.into()).collect_vec(); + let post_memory: Vec = post_memory.iter().map(|&b| b.into()).collect_vec(); + + interpreter.set_memory_segment(Segment::MainMemory, pre_memory); + interpreter.run()?; + + let main_memory_data = interpreter.get_memory_segment(Segment::MainMemory); + assert_eq!(&main_memory_data, &post_memory); + + Ok(()) +} + +#[test] +fn test_mcopy_0_32_32() { + let dest_offset = 0; + let offset = 32; + let size = 32; + let pre_memory = hex!("0000000000000000000000000000000000000000000000000000000000000000000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f"); + let post_memory = hex!("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f"); + + assert!(test_mcopy(dest_offset, offset, size, &pre_memory, &post_memory).is_ok()) +} + +#[test] +fn test_mcopy_0_0_32() { + let dest_offset = 0; + let offset = 0; + let size = 32; + let pre_memory = hex!("0101010101010101010101010101010101010101010101010101010101010101"); + let post_memory = hex!("0101010101010101010101010101010101010101010101010101010101010101"); + + assert!(test_mcopy(dest_offset, offset, size, &pre_memory, &post_memory).is_ok()) +} + +#[test] +fn test_mcopy_0_1_8() { + let dest_offset = 0; + let offset = 1; + let size = 8; + let pre_memory = hex!("0001020304050607080000000000000000000000000000000000000000000000"); + let post_memory = hex!("0102030405060708080000000000000000000000000000000000000000000000"); + + assert!(test_mcopy(dest_offset, offset, size, &pre_memory, &post_memory).is_ok()) +} + +#[test] +fn test_mcopy_1_0_8() { + let dest_offset = 1; + let offset = 0; + let size = 8; + let pre_memory = hex!("0001020304050607080000000000000000000000000000000000000000000000"); + let post_memory = hex!("0000010203040506070000000000000000000000000000000000000000000000"); + + assert!(test_mcopy(dest_offset, offset, size, &pre_memory, &post_memory).is_ok()) +} + +#[test] +fn test_mcopy_1_0_33() { + init_logger(); + let dest_offset = 1; + let offset = 0; + let size = 33; + let pre_memory = + hex!("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f2021222324252627"); + let post_memory = + hex!("00000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20222324252627"); + + assert!(test_mcopy(dest_offset, offset, size, &pre_memory, &post_memory).is_ok()) +} + +#[test] +fn test_mcopy_1_2_33() { + init_logger(); + let dest_offset = 1; + let offset = 2; + let size = 33; + let pre_memory = + hex!("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f202122232425262728"); + let post_memory = + hex!("0002030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20212222232425262728"); + + assert!(test_mcopy(dest_offset, offset, size, &pre_memory, &post_memory).is_ok()) +} diff --git a/evm_arithmetization/src/cpu/kernel/tests/mod.rs b/evm_arithmetization/src/cpu/kernel/tests/mod.rs index 8ce0e7604..c44eb8454 100644 --- a/evm_arithmetization/src/cpu/kernel/tests/mod.rs +++ b/evm_arithmetization/src/cpu/kernel/tests/mod.rs @@ -13,6 +13,7 @@ mod exp; mod hash; mod kernel_consistency; mod log; +mod mcopy; mod mpt; mod packing; mod receipt;