Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cancun): mcopy check offsets #252

Merged
merged 5 commits into from
May 29, 2024
Merged

fix(cancun): mcopy check offsets #252

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0dafed9
fix: check invalid offsets
Nashtare May 27, 2024
8db1c1d
Update per Will's comment
Nashtare May 27, 2024
8a33b88
Merge branch 'feat/cancun' into cancun/mcopy_check_offsets
Nashtare May 28, 2024
76483fb
Minor
Nashtare May 29, 2024
a9f095a
fix(cancun): revamp `MCOPY` with overlapping (#254)
Nashtare May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions evm_arithmetization/src/cpu/kernel/asm/memory/metadata.asm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 12 additions & 6 deletions evm_arithmetization/src/cpu/kernel/asm/memory/syscalls.asm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -142,7 +142,7 @@ wcopy_large_offset:
%build_address
%jump(memset)

wcopy_after:
global wcopy_after:
// stack: kexit_info
EXIT_KERNEL

Expand Down Expand Up @@ -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
Nashtare marked this conversation as resolved.
Show resolved Hide resolved
DUP3
%ensure_offset_in_range
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not directly related to this PR, but I noticed we call %update_mem_bytes only on dest_offset+size below.
My understanding of the EIP, especially the line

If length > 0 and (src + length or dst + length) is beyond the current memory length, the memory is extended with respective gas cost applied.

would imply we also need to call it with offset+size.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm actually I think this would also be handling the error in that particular txn case. Will update the PR to have it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think there may be issues with the current mcopy_with_overlap. I don't think the current impl is valid if SRC < DST and DST - SRC < min(32, size). To alleviate it without too much overhead, I guess we could simply have a %memcpy_bytes_reversed macro that processes the copy backwards, starting from the last offset to read / write from and decrementing both DST, SRC and size.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would also like confirmation on this one! (Other MCOPY PR got merged into this branch)


%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
Expand Down Expand Up @@ -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.

Expand Down