Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
fix: Higher address bits are truncated in x64 assembly machine

Note this is a bug that affects behavior, we can only fix it in VERSION2 and beyond
  • Loading branch information
xxuejie committed Mar 26, 2024
1 parent e416ba4 commit 665cbcc
Show file tree
Hide file tree
Showing 7 changed files with 121 additions and 13 deletions.
4 changes: 4 additions & 0 deletions definitions/src/generate_asm_constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,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_VERSION {}",
(&m.version as *const u32 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
Expand Down
1 change: 1 addition & 0 deletions src/machine/asm/cdefinitions_generated.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#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_VERSION 316
#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
Expand Down
16 changes: 12 additions & 4 deletions src/machine/asm/execute_aarch64.S
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,16 @@
#define WRITE_RS3(v) \
str v, REGISTER_ADDRESS(RS3)

/*
* This is added to replicate a bug in x64 assembly VM
*/
#define LOAD_PC(reg1, reg1w, reg2, reg2w, temp_regw) \
ldr reg1, [MACHINE, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_PC] SEP \
ubfx reg2, reg1, 0, 32 SEP \
ldrb temp_regw, [MACHINE, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_VERSION] SEP \
cmp temp_regw, 2 SEP \
csel reg2, reg2, reg1, lo

#define NEXT_INST \
ldr TEMP1, [INST_ARGS] SEP \
add INST_ARGS, INST_ARGS, 8 SEP \
Expand Down Expand Up @@ -305,8 +315,7 @@ ckb_vm_x64_execute:
add MEMORY_OFFSET_ADDRESS, MEMORY_OFFSET_ADDRESS, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_MEMORY_L

.CKB_VM_ASM_LABEL_OP_CUSTOM_TRACE_END:
ldr TEMP2, [MACHINE, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_PC]
mov TEMP3, TEMP2
LOAD_PC(TEMP2, TEMP2w, TEMP3, TEMP3w, TEMP4w)
lsr TEMP2, TEMP2, 2
and TEMP2, TEMP2, 8191
mov TEMP1, CKB_VM_ASM_TRACE_STRUCT_SIZE
Expand Down Expand Up @@ -346,8 +355,7 @@ ckb_vm_x64_execute:
ldarb TEMP2w, [PAUSE]
cmp TEMP2, ZERO_VALUE
bne .exit_pause
ldr TEMP2, [MACHINE, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_PC]
mov TEMP3, TEMP2
LOAD_PC(TEMP2, TEMP2w, TEMP3, TEMP3w, TEMP4w)
lsr TEMP2, TEMP2, 2
and TEMP2, TEMP2, 8191
mov TEMP1, CKB_VM_ASM_TRACE_STRUCT_SIZE
Expand Down
20 changes: 15 additions & 5 deletions src/machine/asm/execute_x64.S
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,19 @@
#define WRITE_RS3(v) \
movq v, REGISTER_ADDRESS(RS3); \

/*
* This macro is added to cope with an implementation bug in the x64 assembly code,
* where address bigger than u32 limit will be truncated unexpectedly.
*
* Useful tip: https://stackoverflow.com/a/66416462
*/
#define LOAD_PC(reg1, reg1d, reg2, reg2d, temp_regd) \
movq PC_ADDRESS, reg1; \
mov reg1d, reg2d; \
movzbl CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_VERSION(MACHINE), temp_regd; \
cmp $2, temp_regd; \
cmovae reg1, reg2

/*
* Notice we could replace the last 3 instructions with the following:
*
Expand Down Expand Up @@ -419,8 +432,7 @@ ckb_vm_x64_execute:
movq CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_MEMORY_SIZE(MACHINE), MEMORY_SIZE
.p2align 3
.CKB_VM_ASM_LABEL_OP_CUSTOM_TRACE_END:
movq PC_ADDRESS, %rax
mov %eax, %ecx
LOAD_PC(%rax, %eax, %rcx, %ecx, TEMP3d)
shr $2, %eax
andq $8191, %rax
imul $CKB_VM_ASM_TRACE_STRUCT_SIZE, %eax
Expand All @@ -440,7 +452,6 @@ ckb_vm_x64_execute:
addq %rdx, PC_ADDRESS
/* Prefetch trace info for the consecutive block */
movq PC_ADDRESS, %rax
mov %eax, %ecx
shr $2, %eax
andq $8191, %rax
imul $CKB_VM_ASM_TRACE_STRUCT_SIZE, %eax
Expand All @@ -453,8 +464,7 @@ ckb_vm_x64_execute:
movzbl 0(PAUSE), %eax
cmp $0, %rax
jnz .exit_pause
movq PC_ADDRESS, %rax
mov %eax, %ecx
LOAD_PC(%rax, %eax, %rcx, %ecx, TEMP3d)
shr $2, %eax
andq $8191, %rax
imul $CKB_VM_ASM_TRACE_STRUCT_SIZE, %eax
Expand Down
10 changes: 8 additions & 2 deletions src/machine/trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use super::{
},
Error,
},
CoreMachine, DefaultMachine, Machine, SupportMachine,
CoreMachine, DefaultMachine, Machine, SupportMachine, VERSION2,
};
use bytes::Bytes;

Expand Down Expand Up @@ -147,7 +147,13 @@ impl<Inner: SupportMachine> TraceMachine<Inner> {
}
let pc = self.machine.pc().to_u64();
let slot = calculate_slot(pc);
if pc != self.traces[slot].address || self.traces[slot].instruction_count == 0 {
// This is to replicate a bug in x64 VM
let address_match = if self.machine.version() < VERSION2 {
(pc as u32 as u64) == self.traces[slot].address
} else {
pc == self.traces[slot].address
};
if (!address_match) || self.traces[slot].instruction_count == 0 {
self.traces[slot] = Trace::default();
let mut current_pc = pc;
let mut i = 0;
Expand Down
Binary file added tests/programs/asm_trace_bug
Binary file not shown.
83 changes: 81 additions & 2 deletions tests/test_versions.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
#![cfg(has_asm)]
use ckb_vm::cost_model::constant_cycles;
use ckb_vm::machine::asm::{AsmCoreMachine, AsmMachine};
use ckb_vm::machine::{VERSION0, VERSION1};
use ckb_vm::machine::{VERSION0, VERSION1, VERSION2};
use ckb_vm::memory::{FLAG_DIRTY, FLAG_FREEZED};
use ckb_vm::{
CoreMachine, DefaultCoreMachine, DefaultMachine, DefaultMachineBuilder, Error, Memory,
SparseMemory, WXorXMemory, ISA_IMC, RISCV_PAGESIZE,
SparseMemory, TraceMachine, WXorXMemory, ISA_A, ISA_B, ISA_IMC, ISA_MOP, RISCV_PAGESIZE,
};
use std::fs;

Expand Down Expand Up @@ -331,3 +332,81 @@ pub fn test_asm_version1_cadd_hints() {
assert!(result.is_ok());
assert_eq!(result.unwrap(), 0);
}

#[test]
pub fn test_asm_version1_asm_trace_bug() {
let buffer = fs::read("tests/programs/asm_trace_bug").unwrap().into();

let mut machine = {
let asm_core = AsmCoreMachine::new(ISA_IMC | ISA_A | ISA_B | ISA_MOP, VERSION1, 2000);
let machine = DefaultMachineBuilder::<Box<AsmCoreMachine>>::new(asm_core)
.instruction_cycle_func(Box::new(constant_cycles))
.build();
AsmMachine::new(machine)
};
machine.load_program(&buffer, &[]).unwrap();
let result = machine.run();

assert_eq!(result, Err(Error::CyclesExceeded));
}

#[test]
pub fn test_asm_version2_asm_trace_bug() {
let buffer = fs::read("tests/programs/asm_trace_bug").unwrap().into();

let mut machine = {
let asm_core = AsmCoreMachine::new(ISA_IMC | ISA_A | ISA_B | ISA_MOP, VERSION2, 2000);
let machine = DefaultMachineBuilder::<Box<AsmCoreMachine>>::new(asm_core)
.instruction_cycle_func(Box::new(constant_cycles))
.build();
AsmMachine::new(machine)
};
machine.load_program(&buffer, &[]).unwrap();
let result = machine.run();

assert_eq!(result, Err(Error::MemOutOfBound));
}

#[test]
pub fn test_trace_version1_asm_trace_bug() {
let buffer = fs::read("tests/programs/asm_trace_bug").unwrap().into();

let mut machine = {
let core_machine = DefaultCoreMachine::<u64, WXorXMemory<SparseMemory<u64>>>::new(
ISA_IMC | ISA_A | ISA_B | ISA_MOP,
VERSION1,
2000,
);
TraceMachine::new(
DefaultMachineBuilder::new(core_machine)
.instruction_cycle_func(Box::new(constant_cycles))
.build(),
)
};
machine.load_program(&buffer, &[]).unwrap();
let result = machine.run();

assert_eq!(result, Err(Error::CyclesExceeded));
}

#[test]
pub fn test_trace_version2_asm_trace_bug() {
let buffer = fs::read("tests/programs/asm_trace_bug").unwrap().into();

let mut machine = {
let core_machine = DefaultCoreMachine::<u64, WXorXMemory<SparseMemory<u64>>>::new(
ISA_IMC | ISA_A | ISA_B | ISA_MOP,
VERSION2,
2000,
);
TraceMachine::new(
DefaultMachineBuilder::new(core_machine)
.instruction_cycle_func(Box::new(constant_cycles))
.build(),
)
};
machine.load_program(&buffer, &[]).unwrap();
let result = machine.run();

assert_eq!(result, Err(Error::MemOutOfBound));
}

0 comments on commit 665cbcc

Please sign in to comment.