diff --git a/CHANGELOG.md b/CHANGELOG.md index e7ce1af565..a3fcd88f24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - [#546](https://github.com/FuelLabs/fuel-vm/pull/546): Improve debug formatting of instruction in panic receipts. +### Fixed + +#### Breaking + +- [#524](https://github.com/FuelLabs/fuel-vm/pull/524): Fix a crash in `CCP` instruction when overflowing contract bounds. Fix a bug in `CCP` where overflowing contract bounds in a different way would not actually copy the contract bytes, but just zeroes out the section. Fix a bug in `LDC` where it would revert the transaction when the contract bounds were exceeded, when it's just supposed to fill the rest of the bytes with zeroes. + ## [Version 0.36.0] ### Changed diff --git a/fuel-types/src/bytes.rs b/fuel-types/src/bytes.rs index 5415e9e59d..e6bea4a5c1 100644 --- a/fuel-types/src/bytes.rs +++ b/fuel-types/src/bytes.rs @@ -21,6 +21,26 @@ pub const fn padded_len(bytes: &[u8]) -> usize { padded_len_usize(bytes.len()) } +/// Return the word-padded length of an arbitrary length +pub const fn padded_len_word(len: Word) -> Word { + let pad = len % (WORD_SIZE as Word); + + // `pad != 0` is checked because we shouldn't pad in case the length is already + // well-formed. + // + // Example being `w := WORD_SIZE` and `x := 2 · w` + // + // 1) With the check (correct result) + // f(x) -> x + (x % w != 0) · (w - x % w) + // f(x) -> x + 0 · w + // f(x) -> x + // + // 2) Without the check (incorrect result) + // f(x) -> x + w - x % w + // f(x) -> x + w + len + (pad != 0) as Word * ((WORD_SIZE as Word) - pad) +} + /// Return the word-padded length of an arbitrary length pub const fn padded_len_usize(len: usize) -> usize { let pad = len % WORD_SIZE; diff --git a/fuel-vm/src/interpreter/blockchain.rs b/fuel-vm/src/interpreter/blockchain.rs index 3687f099d6..e2d5413699 100644 --- a/fuel-vm/src/interpreter/blockchain.rs +++ b/fuel-vm/src/interpreter/blockchain.rs @@ -19,9 +19,9 @@ use super::{ AppendReceipt, }, memory::{ + copy_from_slice_zero_fill_noownerchecks, read_bytes, try_mem_write, - try_zeroize, OwnershipRegisters, }, ExecutableTransaction, @@ -39,12 +39,7 @@ use crate::{ }, consts::*, context::Context, - error::{ - Bug, - BugId, - BugVariant, - RuntimeError, - }, + error::RuntimeError, interpreter::{ receipts::ReceiptsCtx, InputContracts, @@ -463,9 +458,9 @@ impl<'vm, S, I> LoadContractCodeCtx<'vm, S, I> { /// ``` pub(crate) fn load_contract_code( mut self, - a: Word, - b: Word, - c: Word, + contract_id_addr: Word, + contract_offset: Word, + length_unpadded: Word, ) -> Result<(), RuntimeError> where I: Iterator, @@ -479,10 +474,12 @@ impl<'vm, S, I> LoadContractCodeCtx<'vm, S, I> { return Err(PanicReason::ExpectedUnallocatedStack.into()) } - // Validate arguments - let contract_id = ContractId::from(read_bytes(self.memory, a)?); - let contract_offset = b as usize; - let length = bytes::padded_len_usize(c as usize); + let contract_id = ContractId::from(read_bytes(self.memory, contract_id_addr)?); + let contract_offset: usize = contract_offset + .try_into() + .map_err(|_| PanicReason::MemoryOverflow)?; + + let length = bytes::padded_len_word(length_unpadded); let dst_range = MemoryRange::new(ssp, length)?; if dst_range.end >= *self.hp as usize { @@ -490,48 +487,31 @@ impl<'vm, S, I> LoadContractCodeCtx<'vm, S, I> { return Err(PanicReason::MemoryOverflow.into()) } - if length > self.contract_max_size as usize { + if length > self.contract_max_size { return Err(PanicReason::MemoryOverflow.into()) } - // Clear memory - self.memory[dst_range.usizes()].fill(0); - self.input_contracts.check(&contract_id)?; - // fetch the storage contract + // Fetch the storage contract let contract = super::contract::contract(self.storage, &contract_id)?; let contract = contract.as_ref().as_ref(); - if contract_offset > contract.len() { - return Err(PanicReason::MemoryOverflow.into()) - } - - let contract = &contract[contract_offset..]; - let len = contract.len().min(length); - - let code = &contract[..len]; - - let dst_range = dst_range.truncated(code.len()); - - let memory = self - .memory - .get_mut(dst_range.usizes()) - .expect("Checked memory access"); - - // perform the code copy - memory.copy_from_slice(code); - - self.sp - //TODO this is looser than the compare against [RegId::HP,RegId::SSP+length] - .checked_add(length as Word) - .map(|sp| { - *self.sp = sp; - *self.ssp = sp; - }) - .ok_or_else(|| Bug::new(BugId::ID007, BugVariant::StackPointerOverflow))?; + // Mark stack space as allocated + let new_stack = dst_range.words().end; + *self.sp = new_stack; + *self.ssp = new_stack; + + // Copy the code. Ownership checks are not used as the stack is adjusted above. + copy_from_slice_zero_fill_noownerchecks( + self.memory, + contract, + dst_range.start, + contract_offset, + length, + )?; - // update frame pointer, if we have a stack frame (e.g. fp > 0) + // Update frame pointer, if we have a stack frame (e.g. fp > 0) if fp > 0 { let fp_code_size = MemoryRange::new_overflowing_op( usize::overflowing_add, @@ -649,36 +629,40 @@ struct CodeCopyCtx<'vm, S, I> { impl<'vm, S, I> CodeCopyCtx<'vm, S, I> { pub(crate) fn code_copy( mut self, - a: Word, - b: Word, - c: Word, - d: Word, + dst_addr: Word, + contract_id_addr: Word, + contract_offset: Word, + length: Word, ) -> Result<(), RuntimeError> where I: Iterator, S: InterpreterStorage, { - let contract = CheckedMemConstLen::<{ ContractId::LEN }>::new(b)?; - - MemoryRange::new(a, d)?; - let c_range = MemoryRange::new(c, d)?; - - let contract = ContractId::from_bytes_ref(contract.read(self.memory)); + let contract_id = ContractId::from(read_bytes(self.memory, contract_id_addr)?); + let offset: usize = contract_offset + .try_into() + .map_err(|_| PanicReason::MemoryOverflow)?; + + // Check target memory range ownership + if !self + .owner + .has_ownership_range(&MemoryRange::new(dst_addr, length)?) + { + return Err(PanicReason::MemoryOverflow.into()) + } - self.input_contracts.check(contract)?; + self.input_contracts.check(&contract_id)?; - let contract = super::contract::contract(self.storage, contract)?.into_owned(); + let contract = super::contract::contract(self.storage, &contract_id)?; - if contract.as_ref().len() < d as usize { - try_zeroize(a, d, self.owner, self.memory)?; - } else { - try_mem_write( - a, - &contract.as_ref()[c_range.usizes()], - self.owner, - self.memory, - )?; - } + // Owner checks already performed above + copy_from_slice_zero_fill_noownerchecks( + self.memory, + contract.as_ref().as_ref(), + dst_addr, + offset, + length, + )?; inc_pc(self.pc) } diff --git a/fuel-vm/src/interpreter/memory.rs b/fuel-vm/src/interpreter/memory.rs index a63d05b154..0b796e54c1 100644 --- a/fuel-vm/src/interpreter/memory.rs +++ b/fuel-vm/src/interpreter/memory.rs @@ -169,6 +169,13 @@ impl MemoryRange { Self(start..end) } + /// Splits range at given relative offset. Panics if offset > range length. + pub fn split_at_offset(self, at: usize) -> (Self, Self) { + let mid = self.0.start + at; + assert!(mid <= self.0.end); + (Self(self.0.start..mid), Self(mid..self.0.end)) + } + /// This function is safe because it is only used to shrink the range /// and worst case the range will be empty. pub fn shrink_end(&mut self, by: usize) { @@ -707,3 +714,23 @@ pub(crate) fn write_bytes( memory[range.usizes()].copy_from_slice(&bytes); Ok(()) } + +/// Attempt copy from slice to memory, filling zero bytes when exceeding slice boundaries. +/// Performs overflow and memory range checks, but no ownership checks. +pub(crate) fn copy_from_slice_zero_fill_noownerchecks( + memory: &mut [u8; MEM_SIZE], + src: &[u8], + dst_addr: A, + src_offset: usize, + len: B, +) -> Result<(), RuntimeError> { + let range = MemoryRange::new(dst_addr, len)?; + + let src_end = src_offset.saturating_add(range.len()).min(src.len()); + let data = src.get(src_offset..src_end).unwrap_or_default(); + let (r_data, r_zero) = range.split_at_offset(data.len()); + memory[r_data.usizes()].copy_from_slice(data); + memory[r_zero.usizes()].fill(0); + + Ok(()) +} diff --git a/fuel-vm/src/interpreter/memory/tests.rs b/fuel-vm/src/interpreter/memory/tests.rs index 13980a5379..482abb31c1 100644 --- a/fuel-vm/src/interpreter/memory/tests.rs +++ b/fuel-vm/src/interpreter/memory/tests.rs @@ -312,3 +312,47 @@ fn test_try_zeroize( let memory: [u8; 100] = memory[..100].try_into().unwrap(); (r, memory) } + +// Zero-sized write +#[test_case(0, 0, 0, &[1, 2, 3, 4] => (true, [0xff, 0xff, 0xff, 0xff, 0xff]))] +#[test_case(1, 0, 0, &[1, 2, 3, 4] => (true, [0xff, 0xff, 0xff, 0xff, 0xff]))] +#[test_case(0, 0, 1, &[1, 2, 3, 4] => (true, [0xff, 0xff, 0xff, 0xff, 0xff]))] +// Dst address checks +#[test_case(0, 4, 0, &[1, 2, 3, 4] => (true, [1, 2, 3, 4, 0xff]))] +#[test_case(1, 4, 0, &[1, 2, 3, 4] => (true, [0xff, 1, 2, 3, 4]))] +#[test_case(2, 4, 0, &[1, 2, 3, 4] => (true, [0xff, 0xff, 1, 2, 3]))] +#[test_case(2, 2, 0, &[1, 2, 3, 4] => (true, [0xff, 0xff, 1, 2, 0xff]))] +// Zero padding when exceeding slice size +#[test_case(0, 2, 2, &[1, 2, 3, 4] => (true, [3, 4, 0xff, 0xff, 0xff]))] +#[test_case(0, 2, 3, &[1, 2, 3, 4] => (true, [4, 0, 0xff, 0xff, 0xff]))] +#[test_case(0, 2, 4, &[1, 2, 3, 4] => (true, [0, 0, 0xff, 0xff, 0xff]))] +#[test_case(0, 2, 5, &[1, 2, 3, 4] => (true, [0, 0, 0xff, 0xff, 0xff]))] +// Zero padding when exceeding slice size, but with nonzero dst address +#[test_case(1, 2, 2, &[1, 2, 3, 4] => (true, [0xff, 3, 4, 0xff, 0xff]))] +#[test_case(1, 2, 3, &[1, 2, 3, 4] => (true, [0xff, 4, 0, 0xff, 0xff]))] +#[test_case(1, 2, 4, &[1, 2, 3, 4] => (true, [0xff, 0, 0, 0xff, 0xff]))] +#[test_case(1, 2, 5, &[1, 2, 3, 4] => (true, [0xff, 0, 0, 0xff, 0xff]))] +// Zero-sized src slice +#[test_case(1, 0, 0, &[] => (true, [0xff, 0xff, 0xff, 0xff, 0xff]))] +#[test_case(1, 2, 0, &[] => (true, [0xff, 0, 0, 0xff, 0xff]))] +#[test_case(1, 2, 1, &[] => (true, [0xff, 0, 0, 0xff, 0xff]))] +#[test_case(1, 2, 2, &[] => (true, [0xff, 0, 0, 0xff, 0xff]))] +#[test_case(1, 2, 3, &[] => (true, [0xff, 0, 0, 0xff, 0xff]))] +fn test_copy_from_slice_zero_fill_noownerchecks( + addr: usize, + len: usize, + src_offset: usize, + src_data: &[u8], +) -> (bool, [u8; 5]) { + let mut memory: Memory = vec![0xffu8; MEM_SIZE].try_into().unwrap(); + let r = copy_from_slice_zero_fill_noownerchecks( + &mut memory, + src_data, + addr, + src_offset, + len, + ) + .is_ok(); + let memory: [u8; 5] = memory[..5].try_into().unwrap(); + (r, memory) +} diff --git a/fuel-vm/src/tests/blockchain.rs b/fuel-vm/src/tests/blockchain.rs index 73ee8daf9f..5f085c57a5 100644 --- a/fuel-vm/src/tests/blockchain.rs +++ b/fuel-vm/src/tests/blockchain.rs @@ -539,7 +539,7 @@ fn ldc_reason_helper( assert_ne!(actual_contract_id, &Some(contract_id)); }; } else { - panic!("Script should have panicked"); + panic!("Script should have panicked before logging"); } } @@ -606,26 +606,170 @@ fn ldc_contract_not_in_inputs() { } #[test] -fn ldc_contract_offset_over_length() { - // Then deploy another contract that attempts to read the first one - let reg_a = 0x20; - let reg_b = 0x21; +fn load_contract_code_out_of_contract_offset_over_length() { + let mut test_context = TestBuilder::new(2322u64); + let gas_limit = 1_000_000; - let load_contract = vec![ - op::move_(reg_a, RegId::HP), // r[a] := $hp - op::xor(reg_b, reg_b, reg_b), // r[b] := 0 - op::ori(reg_b, reg_b, 12), /* r[b] += 12 (will be - * padded to 16) */ - op::ldc(reg_a, reg_a, reg_b), // Load first two words from the contract - op::move_(reg_a, RegId::SSP), // r[b] := $ssp - op::subi(reg_a, reg_a, 8 * 2), // r[a] -= 16 (start of the loaded code) - op::xor(reg_b, reg_b, reg_b), // r[b] := 0 - op::ori(reg_b, reg_b, 16), // r[b] += 16 (length of the loaded code) - op::logd(RegId::ZERO, RegId::ZERO, reg_a, reg_b), // Log digest of the loaded code - op::noop(), // Patched to the jump later + let program_ops = vec![ + op::movi(0x10, 0x11), + op::movi(0x11, 0x2a), + op::add(0x12, 0x10, 0x11), + op::log(0x10, 0x11, 0x12, 0x00), + op::ret(0x20), + ]; + + let program = program_ops.clone().into_iter().collect::>(); + let contract_size = program.len(); + let contract_id = test_context + .setup_contract(program_ops, None, None) + .contract_id; + + let (script, _) = script_with_data_offset!( + data_offset, + vec![ + op::movi(0x20, data_offset as Immediate18), + op::add(0x11, RegId::ZERO, 0x20), + op::movi(0x12, (contract_size + 1) as Immediate18), + op::movi(0x13, contract_size as Immediate18), + op::ldc(0x11, 0x12, 0x13), + op::addi(0x21, 0x20, ContractId::LEN as Immediate12), + op::meq(0x30, 0x21, RegId::SP, 0x13), + op::ret(0x30), + ], + TxParameters::DEFAULT.tx_offset() + ); + + let mut script_data = contract_id.to_vec(); + script_data.extend(program.as_slice()); + + let result = test_context + .start_script(script, script_data) + .gas_limit(gas_limit) + .contract_input(contract_id) + .fee_input() + .contract_output(&contract_id) + .execute(); + + let receipts = result.receipts(); + let ret = receipts + .first() + .expect("A `RET` opcode was part of the program."); + + assert_eq!(0, ret.val().expect("Return value")); +} + +#[test] +fn code_copy_shorter_zero_padding() { + let mut test_context = TestBuilder::new(2322u64); + let gas_limit = 1_000_000; + + let program_ops = vec![ + op::movi(0x10, 0x11), + op::movi(0x11, 0x2a), + op::add(0x12, 0x10, 0x11), + op::log(0x10, 0x11, 0x12, 0x00), + op::ret(0x20), ]; - ldc_reason_helper(load_contract, MemoryOverflow, true); + let program = program_ops.clone().into_iter().collect::>(); + let contract_size = program.len(); + let contract_id = test_context + .setup_contract(program_ops, None, None) + .contract_id; + + let (script, _) = script_with_data_offset!( + data_offset, + vec![ + op::movi(0x10, 2048), + op::aloc(0x10), + op::addi(0x10, RegId::HP, contract_size as Immediate12), + op::movi(0x10, 1234), + op::addi(0x10, RegId::HP, (contract_size + 1) as Immediate12), + op::movi(0x10, 1234), + op::movi(0x20, data_offset as Immediate18), + op::add(0x11, RegId::ZERO, 0x20), + op::movi(0x13, (contract_size + 2) as Immediate18), + op::ccp(RegId::HP, 0x11, RegId::ZERO, 0x13), + op::addi(0x21, 0x20, ContractId::LEN as Immediate12), + op::meq(0x30, 0x21, RegId::HP, 0x13), + op::ret(0x30), + ], + TxParameters::DEFAULT.tx_offset() + ); + + let mut script_data = contract_id.to_vec(); + script_data.extend(program.as_slice()); + script_data.extend(vec![0; 2]); + + let result = test_context + .start_script(script, script_data) + .gas_limit(gas_limit) + .contract_input(contract_id) + .fee_input() + .contract_output(&contract_id) + .execute(); + + let receipts = result.receipts(); + let ret = receipts + .first() + .expect("A `RET` opcode was part of the program."); + + assert_eq!(1, ret.val().expect("A constant `1` was returned.")); +} + +#[test] +fn code_copy_out_of_contract_offset_over_length() { + let mut test_context = TestBuilder::new(2322u64); + let gas_limit = 1_000_000; + + let program_ops = vec![ + op::movi(0x10, 0x11), + op::movi(0x11, 0x2a), + op::add(0x12, 0x10, 0x11), + op::log(0x10, 0x11, 0x12, 0x00), + op::ret(0x20), + ]; + + let program = program_ops.clone().into_iter().collect::>(); + let contract_size = program.len(); + let contract_id = test_context + .setup_contract(program_ops, None, None) + .contract_id; + + let (script, _) = script_with_data_offset!( + data_offset, + vec![ + op::movi(0x10, 2048), + op::aloc(0x10), + op::movi(0x20, data_offset as Immediate18), + op::add(0x11, RegId::ZERO, 0x20), + op::movi(0x12, (contract_size + 1) as Immediate18), + op::movi(0x13, contract_size as Immediate18), + op::ccp(RegId::HP, 0x11, 0x12, 0x13), + op::addi(0x21, 0x20, ContractId::LEN as Immediate12), + op::meq(0x30, 0x21, RegId::HP, 0x13), + op::ret(0x30), + ], + TxParameters::DEFAULT.tx_offset() + ); + + let mut script_data = contract_id.to_vec(); + script_data.extend(vec![0; contract_size].as_slice()); + + let result = test_context + .start_script(script, script_data) + .gas_limit(gas_limit) + .contract_input(contract_id) + .fee_input() + .contract_output(&contract_id) + .execute(); + + let receipts = result.receipts(); + let ret = receipts + .first() + .expect("A `RET` opcode was part of the program."); + + assert_eq!(1, ret.val().expect("Return value")); } #[test]