chore: Add useful information to certain Error variants (#393)

* 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.

* fix: Addition overflows

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;
 

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

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

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<u8, Error> {
+pub fn convert_flags(p_flags: u32, allow_freeze_writable: bool, vaddr: u64) -> Result<u8, Error> {
     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<R: Register>(program: &Bytes, version: u32) -> Result<ProgramMe
                 .p_offset
                 .wrapping_add(program_header.p_filesz);
             if slice_start > 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,
             });

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 {}

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<R: Register>(
 ) -> 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(())

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

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:

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
 /*