From 768347fde6d97b9bb61e1beeabcdd80d73b0b7cc Mon Sep 17 00:00:00 2001 From: Xuejie Xiao Date: Fri, 15 Dec 2023 03:14:49 +0000 Subject: [PATCH 1/2] chore: Add useful information to certain Error variants This commit revisits Error type with additional information that would help debugging, such as addresses to access in memory errors, and segment vaddr in Elf errors. To preserve the required information, one additional assembly line is required in each WRITE operation of x64 implementation. The arm64 code does not suffer from any additional assembly code, in fact, we also took the liberty to reduce a few unnecessary arm64 assembly lines. --- definitions/src/asm.rs | 6 +- definitions/src/generate_asm_constants.rs | 4 + src/decoder.rs | 3 +- src/elf.rs | 14 ++-- src/error.rs | 39 ++++++---- src/instructions/common.rs | 7 +- src/machine/asm/cdefinitions_generated.h | 17 ++-- src/machine/asm/execute_aarch64.S | 34 ++++---- src/machine/asm/execute_x64.S | 25 +++--- src/machine/asm/mod.rs | 94 +++++++++++++++++------ src/memory/flat.rs | 39 +++++----- src/memory/mod.rs | 8 +- src/memory/sparse.rs | 27 ++++--- src/memory/wxorx.rs | 28 ++++--- src/snapshot2.rs | 10 ++- tests/test_a_extension.rs | 4 +- tests/test_asm.rs | 40 +++++++--- tests/test_dy_memory.rs | 29 +++++-- tests/test_misc.rs | 45 ++++++++--- tests/test_mop.rs | 86 +++++++++++++++++---- tests/test_versions.rs | 15 +++- 21 files changed, 393 insertions(+), 181 deletions(-) diff --git a/definitions/src/asm.rs b/definitions/src/asm.rs index ed5402e7..bc3285e2 100644 --- a/definitions/src/asm.rs +++ b/definitions/src/asm.rs @@ -76,9 +76,6 @@ pub struct InvokeData { pub fixed_trace_mask: u64, } -// Although the memory here is an array, but when it is created, -// its size is allocated through memory_size, and its maximum length RISCV_MAX_MEMORY -// is used in the structure declaration. #[repr(C)] pub struct AsmCoreMachine { pub registers: [u64; RISCV_GENERAL_REGISTER_NUMBER], @@ -94,6 +91,8 @@ pub struct AsmCoreMachine { pub isa: u8, pub version: u32, + pub error_arg0: u64, + pub memory_size: u64, pub frames_size: u64, pub flags_size: u64, @@ -149,6 +148,7 @@ impl AsmCoreMachine { } machine.chaos_seed = 0; machine.reset_signal = 0; + machine.error_arg0 = 0; machine.version = version; machine.isa = isa; diff --git a/definitions/src/generate_asm_constants.rs b/definitions/src/generate_asm_constants.rs index eefe822f..6f9a6922 100644 --- a/definitions/src/generate_asm_constants.rs +++ b/definitions/src/generate_asm_constants.rs @@ -159,6 +159,10 @@ fn main() { "#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_LOAD_RESERVATION_ADDRESS {}", (&m.load_reservation_address as *const u64 as usize) - m_address ); + println!( + "#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_ERROR_ARG0 {}", + (&m.error_arg0 as *const u64 as usize) - m_address + ); println!( "#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_MEMORY_SIZE {}", (&m.memory_size as *const u64 as usize) - m_address diff --git a/src/decoder.rs b/src/decoder.rs index 967a5aa1..47763928 100644 --- a/src/decoder.rs +++ b/src/decoder.rs @@ -1,6 +1,7 @@ use ckb_vm_definitions::instructions::{self as insts}; use ckb_vm_definitions::registers::{RA, ZERO}; +use crate::error::OutOfBoundKind; use crate::instructions::{ a, b, extract_opcode, i, instruction_length, m, rvc, set_instruction_length_n, Instruction, InstructionFactory, Itype, R4type, R5type, Register, Rtype, Utype, @@ -99,7 +100,7 @@ impl Decoder { // since we are using u64::MAX as the default key in the instruction cache, have to check out of bound // error first. if pc as usize >= memory.memory_size() { - return Err(Error::MemOutOfBound); + return Err(Error::MemOutOfBound(pc, OutOfBoundKind::Memory)); } let instruction_cache_key = { // according to RISC-V instruction encoding, the lowest bit in PC will always be zero diff --git a/src/elf.rs b/src/elf.rs index 971dd7c8..8d537492 100644 --- a/src/elf.rs +++ b/src/elf.rs @@ -12,15 +12,15 @@ pub use goblin_v023::elf::program_header::{PF_R, PF_W, PF_X, PT_LOAD}; pub use goblin_v023::elf::section_header::SHF_EXECINSTR; /// Converts goblin's ELF flags into RISC-V flags -pub fn convert_flags(p_flags: u32, allow_freeze_writable: bool) -> Result { +pub fn convert_flags(p_flags: u32, allow_freeze_writable: bool, vaddr: u64) -> Result { let readable = p_flags & PF_R != 0; let writable = p_flags & PF_W != 0; let executable = p_flags & PF_X != 0; if !readable { - return Err(Error::ElfSegmentUnreadable); + return Err(Error::ElfSegmentUnreadable(vaddr)); } if writable && executable { - return Err(Error::ElfSegmentWritableAndExecutable); + return Err(Error::ElfSegmentWritableAndExecutable(vaddr)); } if executable { Ok(FLAG_EXECUTABLE | FLAG_FREEZED) @@ -188,12 +188,16 @@ pub fn parse_elf(program: &Bytes, version: u32) -> Result slice_end || slice_end > program.len() as u64 { - return Err(Error::ElfSegmentAddrOrSizeError); + return Err(Error::ElfSegmentAddrOrSizeError(program_header.p_vaddr)); } actions.push(LoadingAction { addr: aligned_start, size, - flags: convert_flags(program_header.p_flags, version < VERSION1)?, + flags: convert_flags( + program_header.p_flags, + version < VERSION1, + program_header.p_vaddr, + )?, source: slice_start..slice_end, offset_from_addr: padding_start, }); diff --git a/src/error.rs b/src/error.rs index 7d8e7a21..c17605f2 100644 --- a/src/error.rs +++ b/src/error.rs @@ -10,12 +10,15 @@ pub enum Error { ElfBits, #[display(fmt = "elf error: {}", "_0")] ElfParseError(String), - #[display(fmt = "elf error: segment is unreadable")] - ElfSegmentUnreadable, - #[display(fmt = "elf error: segment is writable and executable")] - ElfSegmentWritableAndExecutable, - #[display(fmt = "elf error: segment addr or size is wrong")] - ElfSegmentAddrOrSizeError, + #[display(fmt = "elf error: segment is unreadable vaddr=0x{:x}", "_0")] + ElfSegmentUnreadable(u64), + #[display( + fmt = "elf error: segment is writable and executable vaddr=0x{:x}", + "_0" + )] + ElfSegmentWritableAndExecutable(u64), + #[display(fmt = "elf error: segment addr or size is wrong vaddr=0x{:x}", "_0")] + ElfSegmentAddrOrSizeError(u64), // External error type is for the debugging tool of CKB-VM, it should not be // used in this project. #[display(fmt = "external error: {}", "_0")] @@ -37,22 +40,26 @@ pub enum Error { kind: std::io::ErrorKind, data: String, }, - #[display(fmt = "memory error: out of bound")] - MemOutOfBound, + #[display(fmt = "memory error: out of bound addr=0x{:x}, kind={:?}", "_0", "_1")] + MemOutOfBound(u64, OutOfBoundKind), #[display(fmt = "memory error: out of stack")] MemOutOfStack, - #[display(fmt = "memory error: unaligned page access")] - MemPageUnalignedAccess, - #[display(fmt = "memory error: write on executable page")] - MemWriteOnExecutablePage, - #[display(fmt = "memory error: write on freezed page")] - MemWriteOnFreezedPage, + #[display(fmt = "memory error: unaligned page access addr=0x{:x}", "_0")] + MemPageUnalignedAccess(u64), + #[display(fmt = "memory error: write on executable page page_index={}", "_0")] + MemWriteOnExecutablePage(u64), + #[display(fmt = "memory error: write on freezed page page_index={}", "_0")] + MemWriteOnFreezedPage(u64), #[display(fmt = "pause")] Pause, #[display(fmt = "unexpected error")] Unexpected(String), - #[display(fmt = "unimplemented")] - Unimplemented, +} + +#[derive(Debug, PartialEq, Clone, Eq, Display)] +pub enum OutOfBoundKind { + Memory, + ExternalData, } impl std::error::Error for Error {} diff --git a/src/instructions/common.rs b/src/instructions/common.rs index 361a429a..98a6e1a6 100644 --- a/src/instructions/common.rs +++ b/src/instructions/common.rs @@ -1,3 +1,4 @@ +use super::super::error::OutOfBoundKind; use super::super::machine::Machine; use super::super::memory::Memory; use super::register::Register; @@ -88,9 +89,9 @@ fn check_load_boundary( ) -> Result<(), Error> { if version0 { let address = address.to_u64(); - let end = address.checked_add(bytes).ok_or(Error::MemOutOfBound)?; - if end == memory_size { - return Err(Error::MemOutOfBound); + let (end, overflow) = address.overflowing_add(bytes); + if overflow || end == memory_size { + return Err(Error::MemOutOfBound(end, OutOfBoundKind::Memory)); } } Ok(()) diff --git a/src/machine/asm/cdefinitions_generated.h b/src/machine/asm/cdefinitions_generated.h index ab57330c..3aa92b89 100644 --- a/src/machine/asm/cdefinitions_generated.h +++ b/src/machine/asm/cdefinitions_generated.h @@ -44,14 +44,15 @@ #define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_CHAOS_MODE 296 #define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_CHAOS_SEED 300 #define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_LOAD_RESERVATION_ADDRESS 304 -#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_MEMORY_SIZE 320 -#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_FRAMES_SIZE 328 -#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_FLAGS_SIZE 336 -#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_LAST_READ_FRAME 344 -#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_LAST_WRITE_PAGE 352 -#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_MEMORY_PTR 360 -#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_FLAGS_PTR 368 -#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_FRAMES_PTR 376 +#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_ERROR_ARG0 320 +#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_MEMORY_SIZE 328 +#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_FRAMES_SIZE 336 +#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_FLAGS_SIZE 344 +#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_LAST_READ_FRAME 352 +#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_LAST_WRITE_PAGE 360 +#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_MEMORY_PTR 368 +#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_FLAGS_PTR 376 +#define CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_FRAMES_PTR 384 #define CKB_VM_ASM_OP_UNLOADED 16 #define CKB_VM_ASM_OP_ADD 17 diff --git a/src/machine/asm/execute_aarch64.S b/src/machine/asm/execute_aarch64.S index 50952edf..c4b668c1 100644 --- a/src/machine/asm/execute_aarch64.S +++ b/src/machine/asm/execute_aarch64.S @@ -179,12 +179,12 @@ cmp TEMP1, TEMP2 SEP \ beq 2f SEP \ 3: \ - mov TEMP1, address_reg SEP \ + mov TEMP3, address_reg SEP \ ldr TEMP2, [MACHINE, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_MEMORY_SIZE] SEP \ - cmp TEMP1, TEMP2 SEP \ + cmp TEMP3, TEMP2 SEP \ bhs .exit_out_of_bound SEP \ - add TEMP1, TEMP1, length SEP \ - cmp TEMP1, TEMP2 SEP \ + add TEMP3, TEMP3, length SEP \ + cmp TEMP3, TEMP2 SEP \ bhs .exit_out_of_bound SEP \ _CHECK_READ_FRAMES(address_reg, length) @@ -200,23 +200,22 @@ cmp TEMP1, TEMP2 SEP \ beq 2f SEP \ 3: \ - mov TEMP1, address_reg SEP \ + mov TEMP3, address_reg SEP \ ldr TEMP2, [MACHINE, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_MEMORY_SIZE] SEP \ - cmp TEMP1, TEMP2 SEP \ + cmp TEMP3, TEMP2 SEP \ bhs .exit_out_of_bound SEP \ - add TEMP1, TEMP1, length SEP \ - cmp TEMP1, TEMP2 SEP \ + add TEMP3, TEMP3, length SEP \ + cmp TEMP3, TEMP2 SEP \ bhi .exit_out_of_bound SEP \ _CHECK_READ_FRAMES(address_reg, length) #define CHECK_WRITE(address_reg, length) \ - mov TEMP1, address_reg SEP \ - lsr TEMP1, TEMP1, CKB_VM_ASM_RISCV_PAGE_SHIFTS SEP \ + mov TEMP3, address_reg SEP \ + lsr TEMP1, TEMP3, CKB_VM_ASM_RISCV_PAGE_SHIFTS SEP \ ldr TEMP2, [MACHINE, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_LAST_WRITE_PAGE] SEP \ cmp TEMP1, TEMP2 SEP \ bne 3f SEP \ - mov TEMP2, address_reg SEP \ - add TEMP2, TEMP2, length SEP \ + add TEMP2, TEMP3, length SEP \ lsr TEMP2, TEMP2, CKB_VM_ASM_RISCV_PAGE_SHIFTS SEP \ cmp TEMP1, TEMP2 SEP \ beq 2f SEP \ @@ -248,12 +247,11 @@ CALL_INITED_MEMORY SEP \ POSTCALL SEP \ 1: \ - mov TEMP1, TEMP2 SEP \ - add TEMP1, TEMP1, 1 SEP \ + add TEMP1, TEMP2, 1 SEP \ lsl TEMP1, TEMP1, CKB_VM_ASM_RISCV_PAGE_SHIFTS SEP \ - mov TEMP2, address_reg SEP \ - add TEMP2, TEMP2, length SEP \ - cmp TEMP2, TEMP1 SEP \ + mov TEMP3, address_reg SEP \ + add TEMP3, TEMP3, length SEP \ + cmp TEMP3, TEMP1 SEP \ bls 2f SEP \ lsr TEMP1, TEMP1, CKB_VM_ASM_RISCV_PAGE_SHIFTS SEP \ ldr TEMP2, [MACHINE, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_FLAGS_SIZE] SEP \ @@ -1935,9 +1933,11 @@ ckb_vm_x64_execute: mov x0, CKB_VM_ASM_RET_CYCLES_OVERFLOW b .exit .exit_out_of_bound: + str TEMP3, [MACHINE, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_ERROR_ARG0] mov x0, CKB_VM_ASM_RET_OUT_OF_BOUND b .exit .exit_invalid_permission: + str TEMP1, [MACHINE, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_ERROR_ARG0] mov x0, CKB_VM_ASM_RET_INVALID_PERMISSION b .exit .exit_pause: diff --git a/src/machine/asm/execute_x64.S b/src/machine/asm/execute_x64.S index 429b911d..37d5f354 100644 --- a/src/machine/asm/execute_x64.S +++ b/src/machine/asm/execute_x64.S @@ -231,11 +231,11 @@ cmp TEMP2, TEMP1; \ je 2f; \ 3: ; \ - movq address_reg, TEMP1; \ - cmp MEMORY_SIZE, TEMP1; \ + movq address_reg, TEMP3; \ + cmp MEMORY_SIZE, TEMP3; \ jae .exit_out_of_bound; \ - addq $length, TEMP1; \ - cmp MEMORY_SIZE, TEMP1; \ + addq $length, TEMP3; \ + cmp MEMORY_SIZE, TEMP3; \ jae .exit_out_of_bound; \ _CHECK_READ_FRAMES(address_reg, length) @@ -251,11 +251,11 @@ cmp TEMP2, TEMP1; \ je 2f; \ 3: ;\ - movq address_reg, TEMP1; \ - cmp MEMORY_SIZE, TEMP1; \ + movq address_reg, TEMP3; \ + cmp MEMORY_SIZE, TEMP3; \ jae .exit_out_of_bound; \ - addq $length, TEMP1; \ - cmp MEMORY_SIZE, TEMP1; \ + addq $length, TEMP3; \ + cmp MEMORY_SIZE, TEMP3; \ ja .exit_out_of_bound; \ _CHECK_READ_FRAMES(address_reg, length) @@ -271,6 +271,7 @@ cmp TEMP2, TEMP1; \ je 2f;\ 3:; \ + movq address_reg, TEMP3; \ movq TEMP1, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_LAST_WRITE_PAGE(MACHINE); \ movq CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_FLAGS_SIZE(MACHINE), TEMP2; \ cmp TEMP2, TEMP1; \ @@ -299,9 +300,9 @@ movq TEMP2, TEMP1; \ addq $1, TEMP1; \ shl $CKB_VM_ASM_RISCV_PAGE_SHIFTS, TEMP1; \ - movq address_reg, TEMP2; \ - addq $length, TEMP2; \ - cmp TEMP1, TEMP2; \ + movq address_reg, TEMP3; \ + addq $length, TEMP3; \ + cmp TEMP1, TEMP3; \ jbe 2f; \ shr $CKB_VM_ASM_RISCV_PAGE_SHIFTS, TEMP1; \ movq CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_FLAGS_SIZE(MACHINE), TEMP2; \ @@ -2396,6 +2397,7 @@ ckb_vm_x64_execute: NEXT_INST .p2align 3 .exit_out_of_bound: + mov TEMP3, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_ERROR_ARG0(MACHINE) mov $CKB_VM_ASM_RET_OUT_OF_BOUND, ARG_RETd jmp .exit .p2align 3 @@ -2408,6 +2410,7 @@ ckb_vm_x64_execute: jmp .exit .p2align 3 .exit_invalid_permission: + mov TEMP1, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_ERROR_ARG0(MACHINE) mov $CKB_VM_ASM_RET_INVALID_PERMISSION, ARG_RETd jmp .exit /* diff --git a/src/machine/asm/mod.rs b/src/machine/asm/mod.rs index c60eaaba..bd442262 100644 --- a/src/machine/asm/mod.rs +++ b/src/machine/asm/mod.rs @@ -17,6 +17,7 @@ use std::os::raw::c_uchar; use crate::{ decoder::{build_decoder, InstDecoder}, elf::ProgramMetadata, + error::OutOfBoundKind, instructions::execute_instruction, machine::{ asm::traces::{decode_fixed_trace, SimpleFixedTraceDecoder, TraceDecoder}, @@ -107,7 +108,7 @@ fn check_memory(machine: &mut AsmCoreMachine, page: u64) { fn check_permission(memory: &mut M, page: u64, flag: u8) -> Result<(), Error> { let page_flag = memory.fetch_flag(page)?; if (page_flag & FLAG_WXORX_BIT) != (flag & FLAG_WXORX_BIT) { - return Err(Error::MemWriteOnExecutablePage); + return Err(Error::MemWriteOnExecutablePage(page)); } Ok(()) } @@ -121,7 +122,7 @@ fn check_memory_writable( debug_assert!(size == 1 || size == 2 || size == 4 || size == 8); let page = addr >> RISCV_PAGE_SHIFTS; if page as usize >= machine.memory_pages() { - return Err(Error::MemOutOfBound); + return Err(Error::MemOutOfBound(addr, OutOfBoundKind::Memory)); } check_permission(machine, page, FLAG_WRITABLE)?; check_memory(machine, page); @@ -132,7 +133,10 @@ fn check_memory_writable( if page_offset + size > RISCV_PAGESIZE { let page = page + 1; if page as usize >= machine.memory_pages() { - return Err(Error::MemOutOfBound); + return Err(Error::MemOutOfBound( + addr + size as u64, + OutOfBoundKind::Memory, + )); } else { check_permission(machine, page, FLAG_WRITABLE)?; check_memory(machine, page); @@ -152,7 +156,7 @@ fn check_memory_executable( let page = addr >> RISCV_PAGE_SHIFTS; if page as usize >= machine.memory_pages() { - return Err(Error::MemOutOfBound); + return Err(Error::MemOutOfBound(addr, OutOfBoundKind::Memory)); } check_permission(machine, page, FLAG_EXECUTABLE)?; check_memory(machine, page); @@ -162,7 +166,10 @@ fn check_memory_executable( if page_offset + size > RISCV_PAGESIZE { let page = page + 1; if page as usize >= machine.memory_pages() { - return Err(Error::MemOutOfBound); + return Err(Error::MemOutOfBound( + addr + size as u64, + OutOfBoundKind::Memory, + )); } else { check_permission(machine, page, FLAG_EXECUTABLE)?; check_memory(machine, page); @@ -180,7 +187,7 @@ fn check_memory_inited( debug_assert!(size == 1 || size == 2 || size == 4 || size == 8); let page = addr >> RISCV_PAGE_SHIFTS; if page as usize >= machine.memory_pages() { - return Err(Error::MemOutOfBound); + return Err(Error::MemOutOfBound(addr, OutOfBoundKind::Memory)); } check_memory(machine, page); @@ -189,7 +196,10 @@ fn check_memory_inited( if page_offset + size > RISCV_PAGESIZE { let page = page + 1; if page as usize >= machine.memory_pages() { - return Err(Error::MemOutOfBound); + return Err(Error::MemOutOfBound( + addr + size as u64, + OutOfBoundKind::Memory, + )); } else { check_memory(machine, page); } @@ -369,17 +379,26 @@ impl Memory for Box { source: Option, offset_from_addr: u64, ) -> Result<(), Error> { - if round_page_down(addr) != addr || round_page_up(size) != size { - return Err(Error::MemPageUnalignedAccess); + if round_page_down(addr) != addr { + return Err(Error::MemPageUnalignedAccess(addr)); } - let memory_size = self.memory_size() as u64; - if addr > memory_size - || size > memory_size as u64 - || addr + size > memory_size as u64 - || offset_from_addr > size - { - return Err(Error::MemOutOfBound); + if round_page_up(size) != size { + return Err(Error::MemPageUnalignedAccess(addr + size)); } + + if addr > self.memory_size() as u64 { + return Err(Error::MemOutOfBound(addr, OutOfBoundKind::Memory)); + } + if size > self.memory_size() as u64 || addr + size > self.memory_size() as u64 { + return Err(Error::MemOutOfBound(addr + size, OutOfBoundKind::Memory)); + } + if offset_from_addr > size { + return Err(Error::MemOutOfBound( + offset_from_addr, + OutOfBoundKind::ExternalData, + )); + } + // We benchmarked the code piece here, using while loop this way is // actually faster than a for..in solution. The difference is roughly // 3% so we are keeping this version. @@ -387,7 +406,7 @@ impl Memory for Box { while current_addr < addr + size { let page = current_addr / RISCV_PAGESIZE as u64; if self.fetch_flag(page)? & FLAG_FREEZED != 0 { - return Err(Error::MemWriteOnFreezedPage); + return Err(Error::MemWriteOnFreezedPage(page)); } current_addr += RISCV_PAGESIZE as u64; } @@ -409,7 +428,10 @@ impl Memory for Box { let slice = self.cast_ptr_to_slice(self.flags_ptr, page as usize, 1); Ok(slice[0]) } else { - Err(Error::MemOutOfBound) + Err(Error::MemOutOfBound( + page << RISCV_PAGE_SHIFTS, + OutOfBoundKind::Memory, + )) } } @@ -421,7 +443,10 @@ impl Memory for Box { self.last_write_page = u64::max_value(); Ok(()) } else { - Err(Error::MemOutOfBound) + Err(Error::MemOutOfBound( + page << RISCV_PAGE_SHIFTS, + OutOfBoundKind::Memory, + )) } } @@ -433,7 +458,10 @@ impl Memory for Box { self.last_write_page = u64::max_value(); Ok(()) } else { - Err(Error::MemOutOfBound) + Err(Error::MemOutOfBound( + page << RISCV_PAGE_SHIFTS, + OutOfBoundKind::Memory, + )) } } @@ -682,8 +710,17 @@ impl AsmMachine { RET_DYNAMIC_JUMP => (), RET_MAX_CYCLES_EXCEEDED => return Err(Error::CyclesExceeded), RET_CYCLES_OVERFLOW => return Err(Error::CyclesOverflow), - RET_OUT_OF_BOUND => return Err(Error::MemOutOfBound), - RET_INVALID_PERMISSION => return Err(Error::MemWriteOnExecutablePage), + RET_OUT_OF_BOUND => { + return Err(Error::MemOutOfBound( + self.machine.inner.error_arg0, + OutOfBoundKind::Memory, + )) + } + RET_INVALID_PERMISSION => { + return Err(Error::MemWriteOnExecutablePage( + self.machine.inner.error_arg0, + )) + } RET_SLOWPATH => { let pc = *self.machine.pc() - 4; let instruction = decoder.decode(self.machine.memory_mut(), pc)?; @@ -716,8 +753,17 @@ impl AsmMachine { RET_ECALL => self.machine.ecall()?, RET_EBREAK => self.machine.ebreak()?, RET_MAX_CYCLES_EXCEEDED => return Err(Error::CyclesExceeded), - RET_OUT_OF_BOUND => return Err(Error::MemOutOfBound), - RET_INVALID_PERMISSION => return Err(Error::MemWriteOnExecutablePage), + RET_OUT_OF_BOUND => { + return Err(Error::MemOutOfBound( + self.machine.inner.error_arg0, + OutOfBoundKind::Memory, + )) + } + RET_INVALID_PERMISSION => { + return Err(Error::MemWriteOnExecutablePage( + self.machine.inner.error_arg0, + )) + } RET_SLOWPATH => { let pc = *self.machine.pc() - 4; let instruction = decoder.decode(self.machine.memory_mut(), pc)?; diff --git a/src/memory/flat.rs b/src/memory/flat.rs index a68a48c5..07160584 100644 --- a/src/memory/flat.rs +++ b/src/memory/flat.rs @@ -1,4 +1,6 @@ -use super::super::{Error, Register, DEFAULT_MEMORY_SIZE, RISCV_PAGESIZE}; +use super::super::{ + error::OutOfBoundKind, Error, Register, DEFAULT_MEMORY_SIZE, RISCV_PAGESIZE, RISCV_PAGE_SHIFTS, +}; use super::{check_no_overflow, fill_page_data, get_page_indices, memset, set_dirty, Memory}; use byteorder::{LittleEndian, ReadBytesExt, WriteBytesExt}; @@ -77,7 +79,10 @@ impl Memory for FlatMemory { if page < self.riscv_pages as u64 { Ok(self.flags[page as usize]) } else { - Err(Error::MemOutOfBound) + Err(Error::MemOutOfBound( + page << RISCV_PAGE_SHIFTS, + OutOfBoundKind::Memory, + )) } } @@ -86,7 +91,10 @@ impl Memory for FlatMemory { self.flags[page as usize] |= flag; Ok(()) } else { - Err(Error::MemOutOfBound) + Err(Error::MemOutOfBound( + page << RISCV_PAGE_SHIFTS, + OutOfBoundKind::Memory, + )) } } @@ -95,7 +103,10 @@ impl Memory for FlatMemory { self.flags[page as usize] &= !flag; Ok(()) } else { - Err(Error::MemOutOfBound) + Err(Error::MemOutOfBound( + page << RISCV_PAGE_SHIFTS, + OutOfBoundKind::Memory, + )) } } @@ -113,9 +124,7 @@ impl Memory for FlatMemory { fn load8(&mut self, addr: &Self::REG) -> Result { let addr = addr.to_u64(); - if addr.checked_add(1).ok_or(Error::MemOutOfBound)? > self.len() as u64 { - return Err(Error::MemOutOfBound); - } + check_no_overflow(addr, 1, self.memory_size as u64)?; let mut reader = Cursor::new(&self.data); reader.seek(SeekFrom::Start(addr as u64))?; let v = reader.read_u8()?; @@ -124,9 +133,7 @@ impl Memory for FlatMemory { fn load16(&mut self, addr: &Self::REG) -> Result { let addr = addr.to_u64(); - if addr.checked_add(2).ok_or(Error::MemOutOfBound)? > self.len() as u64 { - return Err(Error::MemOutOfBound); - } + check_no_overflow(addr, 2, self.memory_size as u64)?; let mut reader = Cursor::new(&self.data); reader.seek(SeekFrom::Start(addr as u64))?; // NOTE: Base RISC-V ISA is defined as a little-endian memory system. @@ -136,9 +143,7 @@ impl Memory for FlatMemory { fn load32(&mut self, addr: &Self::REG) -> Result { let addr = addr.to_u64(); - if addr.checked_add(4).ok_or(Error::MemOutOfBound)? > self.len() as u64 { - return Err(Error::MemOutOfBound); - } + check_no_overflow(addr, 4, self.memory_size as u64)?; let mut reader = Cursor::new(&self.data); reader.seek(SeekFrom::Start(addr as u64))?; // NOTE: Base RISC-V ISA is defined as a little-endian memory system. @@ -148,9 +153,7 @@ impl Memory for FlatMemory { fn load64(&mut self, addr: &Self::REG) -> Result { let addr = addr.to_u64(); - if addr.checked_add(8).ok_or(Error::MemOutOfBound)? > self.len() as u64 { - return Err(Error::MemOutOfBound); - } + check_no_overflow(addr, 8, self.memory_size as u64)?; let mut reader = Cursor::new(&self.data); reader.seek(SeekFrom::Start(addr as u64))?; // NOTE: Base RISC-V ISA is defined as a little-endian memory system. @@ -230,9 +233,7 @@ impl Memory for FlatMemory { if size == 0 { return Ok(Bytes::new()); } - if addr.checked_add(size).ok_or(Error::MemOutOfBound)? > self.memory_size() as u64 { - return Err(Error::MemOutOfBound); - } + check_no_overflow(addr, size, self.memory_size as u64)?; Ok(Bytes::from( self[addr as usize..(addr + size) as usize].to_vec(), )) diff --git a/src/memory/mod.rs b/src/memory/mod.rs index e53886fb..170b3d2e 100644 --- a/src/memory/mod.rs +++ b/src/memory/mod.rs @@ -1,5 +1,6 @@ use super::{ bits::{rounddown, roundup}, + error::OutOfBoundKind, Error, Register, RISCV_PAGESIZE, }; use bytes::Bytes; @@ -101,9 +102,12 @@ pub fn fill_page_data( } pub fn check_no_overflow(addr: u64, size: u64, memory_size: u64) -> Result<(), Error> { + if addr >= memory_size { + return Err(Error::MemOutOfBound(addr, OutOfBoundKind::Memory)); + } let (addr_end, overflowed) = addr.overflowing_add(size); if overflowed || addr_end > memory_size { - Err(Error::MemOutOfBound) + Err(Error::MemOutOfBound(addr_end, OutOfBoundKind::Memory)) } else { Ok(()) } @@ -125,7 +129,7 @@ pub fn check_permission( for page in page_indices.0..=page_indices.1 { let page_flag = memory.fetch_flag(page)?; if (page_flag & FLAG_WXORX_BIT) != (flag & FLAG_WXORX_BIT) { - return Err(Error::MemWriteOnExecutablePage); + return Err(Error::MemWriteOnExecutablePage(page)); } } Ok(()) diff --git a/src/memory/sparse.rs b/src/memory/sparse.rs index 89500216..09cf9178 100644 --- a/src/memory/sparse.rs +++ b/src/memory/sparse.rs @@ -1,5 +1,7 @@ -use super::super::{Error, Register, DEFAULT_MEMORY_SIZE, RISCV_PAGESIZE, RISCV_PAGE_SHIFTS}; -use super::{fill_page_data, memset, round_page_down, Memory, Page, FLAG_DIRTY}; +use super::super::{ + error::OutOfBoundKind, Error, Register, DEFAULT_MEMORY_SIZE, RISCV_PAGESIZE, RISCV_PAGE_SHIFTS, +}; +use super::{check_no_overflow, fill_page_data, memset, round_page_down, Memory, Page, FLAG_DIRTY}; use bytes::Bytes; use std::cmp::min; @@ -27,7 +29,7 @@ impl SparseMemory { fn fetch_page(&mut self, aligned_addr: u64) -> Result<&mut Page, Error> { let page = aligned_addr / RISCV_PAGESIZE as u64; if page >= self.riscv_pages as u64 { - return Err(Error::MemOutOfBound); + return Err(Error::MemOutOfBound(aligned_addr, OutOfBoundKind::Memory)); } let mut index = self.indices[page as usize]; if index == INVALID_PAGE_INDEX { @@ -112,7 +114,10 @@ impl Memory for SparseMemory { if page < self.riscv_pages as u64 { Ok(self.flags[page as usize]) } else { - Err(Error::MemOutOfBound) + Err(Error::MemOutOfBound( + page << RISCV_PAGE_SHIFTS, + OutOfBoundKind::Memory, + )) } } @@ -121,7 +126,10 @@ impl Memory for SparseMemory { self.flags[page as usize] |= flag; Ok(()) } else { - Err(Error::MemOutOfBound) + Err(Error::MemOutOfBound( + page << RISCV_PAGE_SHIFTS, + OutOfBoundKind::Memory, + )) } } @@ -130,7 +138,10 @@ impl Memory for SparseMemory { self.flags[page as usize] &= !flag; Ok(()) } else { - Err(Error::MemOutOfBound) + Err(Error::MemOutOfBound( + page << RISCV_PAGE_SHIFTS, + OutOfBoundKind::Memory, + )) } } @@ -212,9 +223,7 @@ impl Memory for SparseMemory { if size == 0 { return Ok(Bytes::new()); } - if addr.checked_add(size).ok_or(Error::MemOutOfBound)? > self.memory_size() as u64 { - return Err(Error::MemOutOfBound); - } + check_no_overflow(addr, size, self.memory_size() as u64)?; let mut current_page_addr = round_page_down(addr); let mut current_page_offset = addr - current_page_addr; let mut need_read_len = size; diff --git a/src/memory/wxorx.rs b/src/memory/wxorx.rs index 3578d61f..35eefb61 100644 --- a/src/memory/wxorx.rs +++ b/src/memory/wxorx.rs @@ -1,4 +1,4 @@ -use super::super::{Error, Register, RISCV_PAGESIZE}; +use super::super::{error::OutOfBoundKind, Error, Register, RISCV_PAGESIZE}; use super::{ check_no_overflow, check_permission, get_page_indices, round_page_down, round_page_up, Memory, FLAG_EXECUTABLE, FLAG_FREEZED, FLAG_WRITABLE, @@ -41,21 +41,29 @@ impl Memory for WXorXMemory { source: Option, offset_from_addr: u64, ) -> Result<(), Error> { - if round_page_down(addr) != addr || round_page_up(size) != size { - return Err(Error::MemPageUnalignedAccess); + if round_page_down(addr) != addr { + return Err(Error::MemPageUnalignedAccess(addr)); + } + if round_page_up(size) != size { + return Err(Error::MemPageUnalignedAccess(addr + size)); } - if addr > self.memory_size() as u64 - || size > self.memory_size() as u64 - || addr + size > self.memory_size() as u64 - || offset_from_addr > size - { - return Err(Error::MemOutOfBound); + if addr > self.memory_size() as u64 { + return Err(Error::MemOutOfBound(addr, OutOfBoundKind::Memory)); + } + if size > self.memory_size() as u64 || addr + size > self.memory_size() as u64 { + return Err(Error::MemOutOfBound(addr + size, OutOfBoundKind::Memory)); + } + if offset_from_addr > size { + return Err(Error::MemOutOfBound( + offset_from_addr, + OutOfBoundKind::ExternalData, + )); } for page_addr in (addr..addr + size).step_by(RISCV_PAGESIZE) { let page = page_addr / RISCV_PAGESIZE as u64; if self.fetch_flag(page)? & FLAG_FREEZED != 0 { - return Err(Error::MemWriteOnFreezedPage); + return Err(Error::MemWriteOnFreezedPage(page)); } self.set_flag(page, flags)?; } diff --git a/src/snapshot2.rs b/src/snapshot2.rs index 1f0f3091..05922374 100644 --- a/src/snapshot2.rs +++ b/src/snapshot2.rs @@ -66,11 +66,11 @@ impl> Snapshot2Context { machine.set_max_cycles(snapshot.max_cycles); for (address, flag, id, offset, length) in &snapshot.pages_from_source { if address % PAGE_SIZE != 0 { - return Err(Error::MemPageUnalignedAccess); + return Err(Error::MemPageUnalignedAccess(*address)); } let data = self.data_source().load_data(id, *offset, *length)?; if data.len() as u64 % PAGE_SIZE != 0 { - return Err(Error::MemPageUnalignedAccess); + return Err(Error::MemPageUnalignedAccess(address * data.len() as u64)); } machine.memory_mut().store_bytes(*address, &data)?; for i in 0..(data.len() as u64 / PAGE_SIZE) { @@ -81,10 +81,12 @@ impl> Snapshot2Context { } for (address, flag, content) in &snapshot.dirty_pages { if address % PAGE_SIZE != 0 { - return Err(Error::MemPageUnalignedAccess); + return Err(Error::MemPageUnalignedAccess(*address)); } if content.len() as u64 % PAGE_SIZE != 0 { - return Err(Error::MemPageUnalignedAccess); + return Err(Error::MemPageUnalignedAccess( + address + content.len() as u64, + )); } machine.memory_mut().store_bytes(*address, content)?; for i in 0..(content.len() as u64 / PAGE_SIZE) { diff --git a/tests/test_a_extension.rs b/tests/test_a_extension.rs index 61bdbcc7..a4b9c01b 100644 --- a/tests/test_a_extension.rs +++ b/tests/test_a_extension.rs @@ -8,14 +8,14 @@ pub fn test_write_permission_bug() { let mut machine = machine_build::int_v2_imacb("tests/programs/amo_write_permission"); let ret = machine.run(); assert!(ret.is_err()); - assert_eq!(ret.err(), Some(Error::MemWriteOnExecutablePage)); + assert_eq!(ret.err(), Some(Error::MemWriteOnExecutablePage(16))); #[cfg(has_asm)] { let mut machine_asm = machine_build::asm_v2_imacb("tests/programs/amo_write_permission"); let ret_asm = machine_asm.run(); assert!(ret_asm.is_err()); - assert_eq!(ret_asm.err(), Some(Error::MemWriteOnExecutablePage)); + assert_eq!(ret_asm.err(), Some(Error::MemWriteOnExecutablePage(16))); } } diff --git a/tests/test_asm.rs b/tests/test_asm.rs index 3214c37d..1fe93313 100644 --- a/tests/test_asm.rs +++ b/tests/test_asm.rs @@ -2,6 +2,7 @@ use bytes::Bytes; use ckb_vm::cost_model::constant_cycles; use ckb_vm::decoder::build_decoder; +use ckb_vm::error::OutOfBoundKind; use ckb_vm::machine::asm::traces::{MemoizedDynamicTraceDecoder, MemoizedFixedTraceDecoder}; use ckb_vm::machine::asm::{AsmCoreMachine, AsmMachine}; use ckb_vm::machine::{CoreMachine, VERSION0, VERSION1, VERSION2}; @@ -150,7 +151,7 @@ pub fn test_asm_trace() { .unwrap(); let result = machine.run(); assert!(result.is_err()); - assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage)); + assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage(16))); } #[test] @@ -164,7 +165,7 @@ pub fn test_asm_jump0() { .unwrap(); let result = machine.run(); assert!(result.is_err()); - assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage)); + assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage(0))); } #[test] @@ -180,7 +181,10 @@ pub fn test_asm_write_large_address() { .unwrap(); let result = machine.run(); assert!(result.is_err()); - assert_eq!(result.err(), Some(Error::MemOutOfBound)); + assert_eq!( + result.err(), + Some(Error::MemOutOfBound(0xffffff00, OutOfBoundKind::Memory)) + ); } #[test] @@ -221,7 +225,13 @@ pub fn test_invalid_read64() { .unwrap(); let result = machine.run(); assert!(result.is_err()); - assert_eq!(result.err(), Some(Error::MemOutOfBound)); + assert_eq!( + result.err(), + Some(Error::MemOutOfBound( + 0xffffffffffffffff, + OutOfBoundKind::Memory + )) + ); } #[test] @@ -234,7 +244,7 @@ pub fn test_asm_load_elf_crash_64() { .load_program(&buffer, &vec!["load_elf_crash_64".into()]) .unwrap(); let result = machine.run(); - assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage)); + assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage(16))); } #[test] @@ -247,7 +257,13 @@ pub fn test_asm_wxorx_crash_64() { .load_program(&buffer, &vec!["wxorx_crash_64".into()]) .unwrap(); let result = machine.run(); - assert_eq!(result.err(), Some(Error::MemOutOfBound)); + assert_eq!( + result.err(), + Some(Error::MemOutOfBound( + 0xffffffffffffffff, + OutOfBoundKind::Memory + )) + ); } #[test] @@ -384,7 +400,10 @@ pub fn test_decoder_instructions_cache_pc_out_of_bound_timeout() { .unwrap(); let result = machine.run(); assert!(result.is_err()); - assert_eq!(result.unwrap_err(), Error::MemOutOfBound); + assert_eq!( + result.unwrap_err(), + Error::MemOutOfBound(0x400000, OutOfBoundKind::Memory) + ); } #[test] @@ -484,7 +503,10 @@ pub fn test_big_binary() { let core = DefaultMachineBuilder::new(asm_core).build(); let mut machine = AsmMachine::new(core); let result = machine.load_program(&buffer, &vec!["simple".into()]); - assert_eq!(result, Err(Error::MemOutOfBound)); + assert_eq!( + result, + Err(Error::MemOutOfBound(0x111000, OutOfBoundKind::Memory)) + ); } #[cfg(not(feature = "enable-chaos-mode-by-default"))] @@ -511,5 +533,5 @@ pub fn test_memory_load_crash() { let core = DefaultMachineBuilder::new(asm_core).build(); let mut machine = AsmMachine::new(core); let result = machine.load_program(&buffer, &vec!["memory_crash".into()]); - assert_eq!(result.unwrap_err(), Error::MemWriteOnExecutablePage); + assert_eq!(result.unwrap_err(), Error::MemWriteOnExecutablePage(1023)); } diff --git a/tests/test_dy_memory.rs b/tests/test_dy_memory.rs index ec572aae..d5b8c6d4 100644 --- a/tests/test_dy_memory.rs +++ b/tests/test_dy_memory.rs @@ -1,12 +1,12 @@ +use ckb_vm::{error::OutOfBoundKind, run_with_memory, FlatMemory, SparseMemory}; #[cfg(has_asm)] use ckb_vm::{ machine::{ asm::{AsmCoreMachine, AsmMachine}, - DefaultMachineBuilder, VERSION0, + DefaultMachineBuilder, VERSION0, VERSION2, }, - ISA_IMC, + ISA_A, ISA_B, ISA_IMC, ISA_MOP, }; -use ckb_vm::{run_with_memory, FlatMemory, SparseMemory}; use std::fs; fn run_memory_suc(memory_size: usize, bin_path: String, bin_name: String) { @@ -61,7 +61,10 @@ fn test_memory_out_of_bounds() { SparseMemory::new_with_memory(memory_size), ); assert!(result.is_err()); - assert_eq!(ckb_vm::Error::MemOutOfBound, result.err().unwrap()); + assert_eq!( + ckb_vm::Error::MemOutOfBound(0xfffffffffff3ffb8, OutOfBoundKind::Memory), + result.err().unwrap() + ); let result = run_with_memory::>( &buffer, @@ -69,12 +72,19 @@ fn test_memory_out_of_bounds() { FlatMemory::new_with_memory(memory_size), ); assert!(result.is_err()); - assert_eq!(ckb_vm::Error::MemOutOfBound, result.err().unwrap()); + assert_eq!( + ckb_vm::Error::MemOutOfBound(0xfffffffffff3ffb8, OutOfBoundKind::Memory), + result.err().unwrap() + ); #[cfg(has_asm)] { - let asm_core = - AsmCoreMachine::new_with_memory(ISA_IMC, VERSION0, u64::max_value(), memory_size); + let asm_core = AsmCoreMachine::new_with_memory( + ISA_IMC | ISA_A | ISA_B | ISA_MOP, + VERSION2, + u64::max_value(), + memory_size, + ); let core = DefaultMachineBuilder::new(asm_core).build(); let mut machine = AsmMachine::new(core); machine @@ -82,7 +92,10 @@ fn test_memory_out_of_bounds() { .unwrap(); let result = machine.run(); assert!(result.is_err()); - assert_eq!(ckb_vm::Error::MemOutOfBound, result.err().unwrap()); + assert_eq!( + ckb_vm::Error::MemOutOfBound(0xfffffffffff3ffb8, OutOfBoundKind::Memory), + result.err().unwrap() + ); } } diff --git a/tests/test_misc.rs b/tests/test_misc.rs index 25a40e51..8719d0bc 100644 --- a/tests/test_misc.rs +++ b/tests/test_misc.rs @@ -1,4 +1,5 @@ use ckb_vm::cost_model::constant_cycles; +use ckb_vm::error::OutOfBoundKind; use ckb_vm::machine::VERSION0; use ckb_vm::registers::{A0, A1, A2, A3, A4, A5, A7}; use ckb_vm::{ @@ -109,7 +110,7 @@ pub fn test_trace() { let buffer = fs::read("tests/programs/trace64").unwrap().into(); let result = run::>(&buffer, &vec!["trace64".into()]); assert!(result.is_err()); - assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage)); + assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage(16))); } #[test] @@ -117,7 +118,7 @@ pub fn test_jump0() { let buffer = fs::read("tests/programs/jump0_64").unwrap().into(); let result = run::>(&buffer, &vec!["jump0_64".into()]); assert!(result.is_err()); - assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage)); + assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage(0))); } #[test] @@ -140,7 +141,10 @@ pub fn test_invalid_file_offset64() { .unwrap() .into(); let result = run::>(&buffer, &vec!["invalid_file_offset64".into()]); - assert_eq!(result.err(), Some(Error::ElfSegmentAddrOrSizeError)); + assert_eq!( + result.err(), + Some(Error::ElfSegmentAddrOrSizeError(0x10000)) + ); } #[test] @@ -150,7 +154,7 @@ pub fn test_op_rvc_srli_crash_32() { .unwrap() .into(); let result = run::>(&buffer, &vec!["op_rvc_srli_crash_32".into()]); - assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage)); + assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage(0))); } #[test] @@ -177,14 +181,20 @@ pub fn test_op_rvc_slli_crash_32() { pub fn test_load_elf_crash_64() { let buffer = fs::read("tests/programs/load_elf_crash_64").unwrap().into(); let result = run::>(&buffer, &vec!["load_elf_crash_64".into()]); - assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage)); + assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage(16))); } #[test] pub fn test_wxorx_crash_64() { let buffer = fs::read("tests/programs/wxorx_crash_64").unwrap().into(); let result = run::>(&buffer, &vec!["wxorx_crash_64".into()]); - assert_eq!(result.err(), Some(Error::MemOutOfBound)); + assert_eq!( + result.err(), + Some(Error::MemOutOfBound( + 0xffffffffffffffff, + OutOfBoundKind::Memory + )) + ); } #[test] @@ -194,7 +204,10 @@ pub fn test_flat_crash_64() { DefaultCoreMachine::>::new(ISA_IMC, VERSION0, u64::max_value()); let mut machine = DefaultMachineBuilder::new(core_machine).build(); let result = machine.load_program(&buffer, &vec!["flat_crash_64".into()]); - assert_eq!(result.err(), Some(Error::MemOutOfBound)); + assert_eq!( + result.err(), + Some(Error::MemOutOfBound(0x1100000000, OutOfBoundKind::Memory)) + ); } #[test] @@ -284,7 +297,13 @@ fn assert_memory_load_bytes( }; let ret = memory.load_bytes(addr, outofbound_size as u64); assert!(ret.is_err()); - assert_eq!(ret.err().unwrap(), Error::MemOutOfBound); + // TODO: For randomized tests, the exact address violating memory out of bound + // error, is hard to derive(and will also heavily depend on implementation logic), + // do we really need to assert the exact value causing out-of-bound error here? + assert!(match ret.unwrap_err() { + Error::MemOutOfBound(_, kind) => kind == OutOfBoundKind::Memory, + _ => false, + }); // address out of bound let ret = memory.load_bytes( @@ -295,7 +314,10 @@ fn assert_memory_load_bytes( assert!(ret.is_ok()) } else { assert!(ret.is_err()); - assert_eq!(ret.err().unwrap(), Error::MemOutOfBound); + assert!(match ret.unwrap_err() { + Error::MemOutOfBound(_, kind) => kind == OutOfBoundKind::Memory, + _ => false, + }); } // addr + size is overflow @@ -304,7 +326,10 @@ fn assert_memory_load_bytes( assert!(ret.is_ok()); } else { assert!(ret.is_err()); - assert_eq!(ret.err().unwrap(), Error::MemOutOfBound); + assert!(match ret.unwrap_err() { + Error::MemOutOfBound(_, kind) => kind == OutOfBoundKind::Memory, + _ => false, + }); } } diff --git a/tests/test_mop.rs b/tests/test_mop.rs index b8812f42..ba4a914f 100644 --- a/tests/test_mop.rs +++ b/tests/test_mop.rs @@ -1,6 +1,6 @@ pub mod machine_build; use bytes::Bytes; -use ckb_vm::{registers::A0, CoreMachine, Error, SupportMachine}; +use ckb_vm::{error::OutOfBoundKind, registers::A0, CoreMachine, Error, SupportMachine}; #[test] #[cfg_attr(miri, ignore)] @@ -384,17 +384,32 @@ pub fn test_mop_wide_div_zero() { pub fn test_mop_jump_rel_version1_bug() { let mut machine = machine_build::int_v1_imcb("tests/programs/mop_jump_rel_version1_bug"); let ret = machine.run(); - assert_eq!(ret, Err(Error::MemOutOfBound)); + assert_eq!( + ret, + Err(Error::MemOutOfBound( + 0xffffffff8000f878, + OutOfBoundKind::Memory + )) + ); assert_eq!(*machine.pc(), 0xffffffff8000f878); let mut machine = machine_build::int_v1_mop("tests/programs/mop_jump_rel_version1_bug", vec![]); let ret = machine.run(); - assert_eq!(ret, Err(Error::MemOutOfBound)); + assert_eq!( + ret, + Err(Error::MemOutOfBound(0x8000f878, OutOfBoundKind::Memory)) + ); assert_eq!(*machine.pc(), 0x8000f878); let mut machine = machine_build::int_mop("tests/programs/mop_jump_rel_version1_bug", vec![], 2); let ret = machine.run(); - assert_eq!(ret, Err(Error::MemOutOfBound)); + assert_eq!( + ret, + Err(Error::MemOutOfBound( + 0xffffffff8000f878, + OutOfBoundKind::Memory + )) + ); assert_eq!(*machine.pc(), 0xffffffff8000f878); #[cfg(has_asm)] @@ -402,13 +417,22 @@ pub fn test_mop_jump_rel_version1_bug() { let mut machine_asm = machine_build::asm_v1_mop("tests/programs/mop_jump_rel_version1_bug", vec![]); let ret_asm = machine_asm.run(); - assert_eq!(ret_asm, Err(Error::MemOutOfBound)); + assert_eq!( + ret_asm, + Err(Error::MemOutOfBound(0x8000f878, OutOfBoundKind::Memory)) + ); assert_eq!(*machine_asm.machine.pc(), 0x8000f878); let mut machine_asm = machine_build::asm_mop("tests/programs/mop_jump_rel_version1_bug", vec![], 2); let ret_asm = machine_asm.run(); - assert_eq!(ret_asm, Err(Error::MemOutOfBound)); + assert_eq!( + ret_asm, + Err(Error::MemOutOfBound( + 0xffffffff8000f878, + OutOfBoundKind::Memory + )) + ); assert_eq!(*machine_asm.machine.pc(), 0xffffffff8000f878); } } @@ -418,7 +442,10 @@ pub fn test_mop_jump_rel_version1_reg_not_updated_bug() { let mut machine = machine_build::int_v1_imcb("tests/programs/mop_jump_rel_version1_reg_not_updated_bug"); let ret = machine.run(); - assert_eq!(ret, Err(Error::MemOutOfBound)); + assert_eq!( + ret, + Err(Error::MemOutOfBound(0x401054a, OutOfBoundKind::Memory)) + ); assert_eq!(machine.registers()[A0], 67174520); let mut machine = machine_build::int_v1_mop( @@ -426,7 +453,10 @@ pub fn test_mop_jump_rel_version1_reg_not_updated_bug() { vec![], ); let ret = machine.run(); - assert_eq!(ret, Err(Error::MemOutOfBound)); + assert_eq!( + ret, + Err(Error::MemOutOfBound(0x401054a, OutOfBoundKind::Memory)) + ); assert_eq!(machine.registers()[A0], 0); let mut machine = machine_build::int_mop( @@ -435,7 +465,10 @@ pub fn test_mop_jump_rel_version1_reg_not_updated_bug() { 2, ); let ret = machine.run(); - assert_eq!(ret, Err(Error::MemOutOfBound)); + assert_eq!( + ret, + Err(Error::MemOutOfBound(0x401054a, OutOfBoundKind::Memory)) + ); assert_eq!(machine.registers()[A0], 67174520); #[cfg(has_asm)] @@ -445,7 +478,10 @@ pub fn test_mop_jump_rel_version1_reg_not_updated_bug() { vec![], ); let ret_asm = machine_asm.run(); - assert_eq!(ret_asm, Err(Error::MemOutOfBound)); + assert_eq!( + ret_asm, + Err(Error::MemOutOfBound(0x401054a, OutOfBoundKind::Memory)) + ); assert_eq!(machine_asm.machine.registers()[A0], 0); let mut machine_asm = machine_build::asm_mop( @@ -454,7 +490,10 @@ pub fn test_mop_jump_rel_version1_reg_not_updated_bug() { 2, ); let ret_asm = machine_asm.run(); - assert_eq!(ret_asm, Err(Error::MemOutOfBound)); + assert_eq!( + ret_asm, + Err(Error::MemOutOfBound(0x401054a, OutOfBoundKind::Memory)) + ); assert_eq!(machine_asm.machine.registers()[A0], 67174520); } } @@ -464,7 +503,10 @@ pub fn test_mop_jump_abs_version1_reg_not_updated_bug() { let mut machine = machine_build::int_v1_imcb("tests/programs/mop_jump_abs_version1_reg_not_updated_bug"); let ret = machine.run(); - assert_eq!(ret, Err(Error::MemOutOfBound)); + assert_eq!( + ret, + Err(Error::MemOutOfBound(0x40004d2, OutOfBoundKind::Memory)) + ); assert_eq!(machine.registers()[A0], 67108864); let mut machine = machine_build::int_v1_mop( @@ -472,7 +514,10 @@ pub fn test_mop_jump_abs_version1_reg_not_updated_bug() { vec![], ); let ret = machine.run(); - assert_eq!(ret, Err(Error::MemOutOfBound)); + assert_eq!( + ret, + Err(Error::MemOutOfBound(0x40004d2, OutOfBoundKind::Memory)) + ); assert_eq!(machine.registers()[A0], 0); let mut machine = machine_build::int_mop( @@ -481,7 +526,10 @@ pub fn test_mop_jump_abs_version1_reg_not_updated_bug() { 2, ); let ret = machine.run(); - assert_eq!(ret, Err(Error::MemOutOfBound)); + assert_eq!( + ret, + Err(Error::MemOutOfBound(0x40004d2, OutOfBoundKind::Memory)) + ); assert_eq!(machine.registers()[A0], 67108864); #[cfg(has_asm)] @@ -491,7 +539,10 @@ pub fn test_mop_jump_abs_version1_reg_not_updated_bug() { vec![], ); let ret_asm = machine_asm.run(); - assert_eq!(ret_asm, Err(Error::MemOutOfBound)); + assert_eq!( + ret_asm, + Err(Error::MemOutOfBound(0x40004d2, OutOfBoundKind::Memory)) + ); assert_eq!(machine_asm.machine.registers()[A0], 0); let mut machine_asm = machine_build::asm_mop( @@ -500,7 +551,10 @@ pub fn test_mop_jump_abs_version1_reg_not_updated_bug() { 2, ); let ret_asm = machine_asm.run(); - assert_eq!(ret_asm, Err(Error::MemOutOfBound)); + assert_eq!( + ret_asm, + Err(Error::MemOutOfBound(0x40004d2, OutOfBoundKind::Memory)) + ); assert_eq!(machine_asm.machine.registers()[A0], 67108864); } } diff --git a/tests/test_versions.rs b/tests/test_versions.rs index 74548dc6..a80c03aa 100644 --- a/tests/test_versions.rs +++ b/tests/test_versions.rs @@ -1,4 +1,5 @@ #![cfg(has_asm)] +use ckb_vm::error::OutOfBoundKind; use ckb_vm::machine::asm::{AsmCoreMachine, AsmMachine}; use ckb_vm::machine::{VERSION0, VERSION1}; use ckb_vm::memory::{FLAG_DIRTY, FLAG_FREEZED}; @@ -74,7 +75,10 @@ pub fn test_rust_version0_read_at_boundary() { let mut machine = create_rust_machine("read_at_boundary64".to_string(), VERSION0); let result = machine.run(); assert!(result.is_err()); - assert_eq!(result.err(), Some(Error::MemOutOfBound)); + assert_eq!( + result.err(), + Some(Error::MemOutOfBound(0x400000, OutOfBoundKind::Memory)) + ); } #[test] @@ -170,7 +174,10 @@ pub fn test_asm_version0_read_at_boundary() { let mut machine = create_asm_machine("read_at_boundary64".to_string(), VERSION0); let result = machine.run(); assert!(result.is_err()); - assert_eq!(result.err(), Some(Error::MemOutOfBound)); + assert_eq!( + result.err(), + Some(Error::MemOutOfBound(0x400000, OutOfBoundKind::Memory)) + ); } #[test] @@ -240,7 +247,7 @@ pub fn test_rust_version0_unaligned64() { DefaultMachineBuilder::>::new(core_machine).build(); let result = machine.load_program(&buffer, &vec![program.into()]); assert!(result.is_err()); - assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage)); + assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage(16))); } #[test] @@ -262,7 +269,7 @@ pub fn test_asm_version0_unaligned64() { let mut machine = AsmMachine::new(core); let result = machine.load_program(&buffer, &vec![program.into()]); assert!(result.is_err()); - assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage)); + assert_eq!(result.err(), Some(Error::MemWriteOnExecutablePage(16))); } #[test] From d09434ae7808875961913b075040be68f37e404d Mon Sep 17 00:00:00 2001 From: Xuejie Xiao Date: Fri, 15 Dec 2023 06:33:39 +0000 Subject: [PATCH 2/2] fix: Addition overflows --- src/machine/asm/mod.rs | 13 ++++++++----- src/memory/wxorx.rs | 7 +++++-- src/snapshot2.rs | 6 ++++-- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/machine/asm/mod.rs b/src/machine/asm/mod.rs index bd442262..8d47a5bf 100644 --- a/src/machine/asm/mod.rs +++ b/src/machine/asm/mod.rs @@ -134,7 +134,7 @@ fn check_memory_writable( let page = page + 1; if page as usize >= machine.memory_pages() { return Err(Error::MemOutOfBound( - addr + size as u64, + addr.wrapping_add(size as u64), OutOfBoundKind::Memory, )); } else { @@ -167,7 +167,7 @@ fn check_memory_executable( let page = page + 1; if page as usize >= machine.memory_pages() { return Err(Error::MemOutOfBound( - addr + size as u64, + addr.wrapping_add(size as u64), OutOfBoundKind::Memory, )); } else { @@ -197,7 +197,7 @@ fn check_memory_inited( let page = page + 1; if page as usize >= machine.memory_pages() { return Err(Error::MemOutOfBound( - addr + size as u64, + addr.wrapping_add(size as u64), OutOfBoundKind::Memory, )); } else { @@ -383,14 +383,17 @@ impl Memory for Box { return Err(Error::MemPageUnalignedAccess(addr)); } if round_page_up(size) != size { - return Err(Error::MemPageUnalignedAccess(addr + size)); + return Err(Error::MemPageUnalignedAccess(addr.wrapping_add(size))); } if addr > self.memory_size() as u64 { return Err(Error::MemOutOfBound(addr, OutOfBoundKind::Memory)); } if size > self.memory_size() as u64 || addr + size > self.memory_size() as u64 { - return Err(Error::MemOutOfBound(addr + size, OutOfBoundKind::Memory)); + return Err(Error::MemOutOfBound( + addr.wrapping_add(size), + OutOfBoundKind::Memory, + )); } if offset_from_addr > size { return Err(Error::MemOutOfBound( diff --git a/src/memory/wxorx.rs b/src/memory/wxorx.rs index 35eefb61..8d773852 100644 --- a/src/memory/wxorx.rs +++ b/src/memory/wxorx.rs @@ -45,14 +45,17 @@ impl Memory for WXorXMemory { return Err(Error::MemPageUnalignedAccess(addr)); } if round_page_up(size) != size { - return Err(Error::MemPageUnalignedAccess(addr + size)); + return Err(Error::MemPageUnalignedAccess(addr.wrapping_add(size))); } if addr > self.memory_size() as u64 { return Err(Error::MemOutOfBound(addr, OutOfBoundKind::Memory)); } if size > self.memory_size() as u64 || addr + size > self.memory_size() as u64 { - return Err(Error::MemOutOfBound(addr + size, OutOfBoundKind::Memory)); + return Err(Error::MemOutOfBound( + addr.wrapping_add(size), + OutOfBoundKind::Memory, + )); } if offset_from_addr > size { return Err(Error::MemOutOfBound( diff --git a/src/snapshot2.rs b/src/snapshot2.rs index 05922374..708434d4 100644 --- a/src/snapshot2.rs +++ b/src/snapshot2.rs @@ -70,7 +70,9 @@ impl> Snapshot2Context { } let data = self.data_source().load_data(id, *offset, *length)?; if data.len() as u64 % PAGE_SIZE != 0 { - return Err(Error::MemPageUnalignedAccess(address * data.len() as u64)); + return Err(Error::MemPageUnalignedAccess( + address.wrapping_add(data.len() as u64), + )); } machine.memory_mut().store_bytes(*address, &data)?; for i in 0..(data.len() as u64 / PAGE_SIZE) { @@ -85,7 +87,7 @@ impl> Snapshot2Context { } if content.len() as u64 % PAGE_SIZE != 0 { return Err(Error::MemPageUnalignedAccess( - address + content.len() as u64, + address.wrapping_add(content.len() as u64), )); } machine.memory_mut().store_bytes(*address, content)?;