From e24f188e9a14e6793acc8f52dfa0d9ffc54fb045 Mon Sep 17 00:00:00 2001 From: Hannes Karppila Date: Sun, 6 Aug 2023 14:59:19 +0300 Subject: [PATCH] Return correct PanicReason on memory-related panics (#511) * Return correct PanicReason on write_bytes without ownership * Fix and clarify MemoryOverflow errors * Further simplify memory management code * One last MaxAccessSize fix * Undo some formatting from too new rustfmt * More fixes * Add more edge case tests for store_byte * Simplify store_byte implementation * Remove dbg macro * Rework instruction fetch * Update changelog * Remove MEM_MAX_ACCESS_SIZE * Remove MIN_VM_MAX_RAM_USIZE_MAX * Fix compilation errors --------- Co-authored-by: green --- CHANGELOG.md | 2 + fuel-asm/src/encoding_tests.rs | 14 -- fuel-asm/src/lib.rs | 7 - fuel-asm/src/panic_reason.rs | 4 +- fuel-vm/Cargo.toml | 1 + fuel-vm/src/consts.rs | 10 +- fuel-vm/src/interpreter/blockchain.rs | 164 +++++++----------- fuel-vm/src/interpreter/blockchain/test.rs | 2 + fuel-vm/src/interpreter/contract.rs | 2 +- fuel-vm/src/interpreter/crypto.rs | 41 ++--- .../src/interpreter/executors/instruction.rs | 37 ++-- fuel-vm/src/interpreter/executors/main.rs | 5 - fuel-vm/src/interpreter/flow.rs | 8 +- fuel-vm/src/interpreter/flow/ret_tests.rs | 2 +- fuel-vm/src/interpreter/internal.rs | 2 +- fuel-vm/src/interpreter/log.rs | 10 +- fuel-vm/src/interpreter/memory.rs | 137 +++++++-------- .../interpreter/memory/allocation_tests.rs | 47 ++++- fuel-vm/src/interpreter/memory/tests.rs | 2 +- fuel-vm/src/tests/blockchain.rs | 5 +- 20 files changed, 212 insertions(+), 290 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee0e3115a..cef1bd375 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - [#529](https://github.com/FuelLabs/fuel-vm/pull/529) [#534](https://github.com/FuelLabs/fuel-vm/pull/534): Enforcing async WASM initialization for all NPM wrapper packages. +- [#511](https://github.com/FuelLabs/fuel-vm/pull/511): Changes multiple panic reasons to be more accurate, and internally refactors instruction fetch logic to be less error-prone. + #### Breaking - [#527](https://github.com/FuelLabs/fuel-vm/pull/527): The balances are empty during predicate estimation/verification. diff --git a/fuel-asm/src/encoding_tests.rs b/fuel-asm/src/encoding_tests.rs index af6c11c0a..01bd7c731 100644 --- a/fuel-asm/src/encoding_tests.rs +++ b/fuel-asm/src/encoding_tests.rs @@ -44,18 +44,6 @@ fn opcode() { assert_eq!(instructions, instructions_from_bytes.unwrap()); - let pairs = bytes.chunks(8).map(|chunk| { - let mut arr = [0; core::mem::size_of::()]; - arr.copy_from_slice(chunk); - Word::from_be_bytes(arr) - }); - - let instructions_from_words: Vec = pairs - .into_iter() - .flat_map(raw_instructions_from_word) - .map(|raw| Instruction::try_from(raw).unwrap()) - .collect(); - #[cfg(feature = "serde")] for ins in &instructions { let ins_ser = bincode::serialize(ins).expect("Failed to serialize opcode"); @@ -63,8 +51,6 @@ fn opcode() { bincode::deserialize(&ins_ser).expect("Failed to serialize opcode"); assert_eq!(ins, &ins_de); } - - assert_eq!(instructions, instructions_from_words); } #[test] diff --git a/fuel-asm/src/lib.rs b/fuel-asm/src/lib.rs index 9f8d16154..1ef7fb319 100644 --- a/fuel-asm/src/lib.rs +++ b/fuel-asm/src/lib.rs @@ -883,13 +883,6 @@ impl core::iter::FromIterator for Vec { } } -/// Produce two raw instructions from a word's hi and lo parts. -pub fn raw_instructions_from_word(word: Word) -> [RawInstruction; 2] { - let hi = (word >> 32) as RawInstruction; - let lo = word as RawInstruction; - [hi, lo] -} - /// Given an iterator yielding bytes, produces an iterator yielding `Instruction`s. /// /// This function assumes each consecutive 4 bytes aligns with an instruction. diff --git a/fuel-asm/src/panic_reason.rs b/fuel-asm/src/panic_reason.rs index 6317d64ae..19f2cefe6 100644 --- a/fuel-asm/src/panic_reason.rs +++ b/fuel-asm/src/panic_reason.rs @@ -74,8 +74,8 @@ enum_from! { InvalidImmediateValue = 0x13, /// The provided transaction input is not of type `Coin`. ExpectedCoinInput = 0x14, - /// The requested memory access exceeds the limits of the interpreter. - MaxMemoryAccess = 0x15, + /// This entry is no longer used, and can be repurposed. + Unused0x15 = 0x15, /// Two segments of the interpreter memory should not intersect for write operations. MemoryWriteOverlap = 0x16, /// The requested contract is not listed in the transaction inputs. diff --git a/fuel-vm/Cargo.toml b/fuel-vm/Cargo.toml index 247a47567..0c97a25b8 100644 --- a/fuel-vm/Cargo.toml +++ b/fuel-vm/Cargo.toml @@ -30,6 +30,7 @@ primitive-types = { version = "0.12", default-features = false } rand = { version = "0.8", optional = true } serde = { version = "1.0", features = ["derive", "rc"], optional = true } sha3 = "0.10" +static_assertions = "1.1" strum = { version = "0.24", features = ["derive"], optional = true } tai64 = "4.0" thiserror = "1.0" diff --git a/fuel-vm/src/consts.rs b/fuel-vm/src/consts.rs index 72516dfd5..619966589 100644 --- a/fuel-vm/src/consts.rs +++ b/fuel-vm/src/consts.rs @@ -35,15 +35,7 @@ pub const VM_MAX_RAM: u64 = 1024 * 1024 * FUEL_MAX_MEMORY_SIZE; /// Size of the VM memory, in bytes. pub const MEM_SIZE: usize = VM_MAX_RAM as usize; -/// Maximum memory access size, in bytes. -pub const MEM_MAX_ACCESS_SIZE: u64 = VM_MAX_RAM; - -/// Tighter of the two bounds for VM_MAX_RAM and usize::MAX -pub const MIN_VM_MAX_RAM_USIZE_MAX: u64 = if VM_MAX_RAM < usize::MAX as u64 { - VM_MAX_RAM -} else { - usize::MAX as u64 -}; +static_assertions::const_assert!(VM_MAX_RAM < usize::MAX as u64); // no limits to heap for now. diff --git a/fuel-vm/src/interpreter/blockchain.rs b/fuel-vm/src/interpreter/blockchain.rs index b9027788f..3687f099d 100644 --- a/fuel-vm/src/interpreter/blockchain.rs +++ b/fuel-vm/src/interpreter/blockchain.rs @@ -19,6 +19,7 @@ use super::{ AppendReceipt, }, memory::{ + read_bytes, try_mem_write, try_zeroize, OwnershipRegisters, @@ -29,13 +30,7 @@ use super::{ RuntimeBalances, }; use crate::{ - arith, - arith::{ - add_usize, - checked_add_usize, - checked_add_word, - checked_sub_word, - }, + arith::checked_add_word, call::CallFrame, constraints::{ reg_key::*, @@ -83,10 +78,7 @@ use fuel_types::{ Word, }; -use std::{ - borrow::Borrow, - ops::Range, -}; +use std::borrow::Borrow; #[cfg(test)] mod code_tests; @@ -487,39 +479,28 @@ impl<'vm, S, I> LoadContractCodeCtx<'vm, S, I> { return Err(PanicReason::ExpectedUnallocatedStack.into()) } - let contract_id = a as usize; - let contract_id_end = checked_add_usize(ContractId::LEN, contract_id)?; + // 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 dst_range = MemoryRange::new(ssp, length)?; - let memory_offset = ssp as usize; - let memory_offset_end = checked_add_usize(memory_offset, length)?; + if dst_range.end >= *self.hp as usize { + // Would make stack and heap overlap + return Err(PanicReason::MemoryOverflow.into()) + } - // Validate arguments - if memory_offset_end >= *self.hp as usize - || contract_id_end as Word > VM_MAX_RAM - || length > MEM_MAX_ACCESS_SIZE as usize - || length > self.contract_max_size as usize - { + if length > self.contract_max_size as usize { return Err(PanicReason::MemoryOverflow.into()) } // Clear memory - self.memory[memory_offset..memory_offset_end].fill(0); + self.memory[dst_range.usizes()].fill(0); - // Fetch the contract id - let contract_id: &[u8; ContractId::LEN] = &self.memory - [contract_id..contract_id_end] - .try_into() - .expect("This can't fail, because we checked the bounds above."); - - // Safety: Memory bounds are checked and consistent - let contract_id = ContractId::from_bytes_ref(contract_id); - - self.input_contracts.check(contract_id)?; + self.input_contracts.check(&contract_id)?; // fetch the storage contract - let contract = super::contract::contract(self.storage, contract_id)?; + let contract = super::contract::contract(self.storage, &contract_id)?; let contract = contract.as_ref().as_ref(); if contract_offset > contract.len() { @@ -531,10 +512,12 @@ impl<'vm, S, I> LoadContractCodeCtx<'vm, S, I> { let code = &contract[..len]; + let dst_range = dst_range.truncated(code.len()); + let memory = self .memory - .get_mut(memory_offset..arith::checked_add_usize(memory_offset, len)?) - .ok_or(PanicReason::MemoryOverflow)?; + .get_mut(dst_range.usizes()) + .expect("Checked memory access"); // perform the code copy memory.copy_from_slice(code); @@ -550,23 +533,25 @@ impl<'vm, S, I> LoadContractCodeCtx<'vm, S, I> { // update frame pointer, if we have a stack frame (e.g. fp > 0) if fp > 0 { - let fp_code_size = add_usize(fp, CallFrame::code_size_offset()); - let fp_code_size_end = add_usize(fp_code_size, WORD_SIZE); - - if fp_code_size_end > self.memory.len() { - Err(PanicReason::MemoryOverflow)?; - } + let fp_code_size = MemoryRange::new_overflowing_op( + usize::overflowing_add, + fp, + CallFrame::code_size_offset(), + WORD_SIZE, + )?; - let length = Word::from_be_bytes( - self.memory[fp_code_size..fp_code_size_end] + let old_code_size = Word::from_be_bytes( + self.memory[fp_code_size.usizes()] .try_into() .expect("`fp_code_size_end` is `WORD_SIZE`"), - ) - .checked_add(length as Word) - .ok_or(PanicReason::MemoryOverflow)?; + ); + + let new_code_size = old_code_size + .checked_add(length as Word) + .ok_or(PanicReason::MemoryOverflow)?; - self.memory[fp_code_size..fp_code_size_end] - .copy_from_slice(&length.to_be_bytes()); + self.memory[fp_code_size.usizes()] + .copy_from_slice(&new_code_size.to_be_bytes()); } inc_pc(self.pc) @@ -674,17 +659,9 @@ impl<'vm, S, I> CodeCopyCtx<'vm, S, I> { S: InterpreterStorage, { let contract = CheckedMemConstLen::<{ ContractId::LEN }>::new(b)?; - let cd = checked_add_word(c, d)?; - if d > MEM_MAX_ACCESS_SIZE - || a > checked_sub_word(VM_MAX_RAM, d)? - || cd > VM_MAX_RAM - { - return Err(PanicReason::MemoryOverflow.into()) - } - - let (a, c, d) = (a as usize, c as usize, d as usize); - let cd = cd as usize; + MemoryRange::new(a, d)?; + let c_range = MemoryRange::new(c, d)?; let contract = ContractId::from_bytes_ref(contract.read(self.memory)); @@ -692,10 +669,15 @@ impl<'vm, S, I> CodeCopyCtx<'vm, S, I> { let contract = super::contract::contract(self.storage, contract)?.into_owned(); - if contract.as_ref().len() < d { + 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..cd], self.owner, self.memory)?; + try_mem_write( + a, + &contract.as_ref()[c_range.usizes()], + self.owner, + self.memory, + )?; } inc_pc(self.pc) @@ -759,13 +741,9 @@ impl<'vm, S, I: Iterator> CodeRootCtx<'vm, S, I> { where S: InterpreterStorage, { - let ax = checked_add_word(a, Bytes32::LEN as Word)?; + MemoryRange::new(a, Bytes32::LEN)?; let contract_id = CheckedMemConstLen::<{ ContractId::LEN }>::new(b)?; - if ax > VM_MAX_RAM { - return Err(PanicReason::MemoryOverflow.into()) - } - let contract_id = ContractId::from_bytes_ref(contract_id.read(self.memory)); self.input_contracts.check(contract_id)?; @@ -954,10 +932,6 @@ where self.recipient_mem_address, )?; - if self.msg_data_len > MEM_MAX_ACCESS_SIZE { - return Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)) - } - if self.msg_data_len > self.max_message_data_length { return Err(RuntimeError::Recoverable(PanicReason::MessageDataTooLong)) } @@ -1012,11 +986,9 @@ where } struct StateReadQWord { - /// The destination memory address is - /// stored in this range of memory. - destination_address_memory_range: Range, - /// The starting storage key location is stored - /// in this range of memory. + /// The destination memory address is stored in this range of memory. + destination_address_memory_range: MemoryRange, + /// The starting storage key location is stored in this range of memory. origin_key_memory_range: CheckedMemConstLen<{ Bytes32::LEN }>, /// Number of slots to read. num_slots: Word, @@ -1029,28 +1001,16 @@ impl StateReadQWord { num_slots: Word, ownership_registers: OwnershipRegisters, ) -> Result { - let mem_range = MemoryRange::new( + let destination_address_memory_range = MemoryRange::new( destination_memory_address, (Bytes32::LEN as Word).saturating_mul(num_slots), )?; - if !ownership_registers.has_ownership_range(&mem_range) { - return Err(PanicReason::MemoryOwnership.into()) - } - if ownership_registers.context.is_external() { - return Err(PanicReason::ExpectedInternalContext.into()) - } - let dest_end = checked_add_word( - destination_memory_address, - Bytes32::LEN.saturating_mul(num_slots as usize) as Word, - )?; + ownership_registers.verify_ownership(&destination_address_memory_range)?; + ownership_registers.verify_internal_context()?; let origin_key_memory_range = CheckedMemConstLen::<{ Bytes32::LEN }>::new(origin_key_memory_address)?; - if dest_end > VM_MAX_RAM { - return Err(PanicReason::MemoryOverflow.into()) - } Ok(Self { - destination_address_memory_range: (destination_memory_address as usize) - ..(dest_end as usize), + destination_address_memory_range, origin_key_memory_range, num_slots, }) @@ -1083,7 +1043,7 @@ fn state_read_qword( *result_register = all_set as Word; - memory[input.destination_address_memory_range].copy_from_slice(&result); + memory[input.destination_address_memory_range.usizes()].copy_from_slice(&result); inc_pc(pc)?; @@ -1091,12 +1051,10 @@ fn state_read_qword( } struct StateWriteQWord { - /// The starting storage key location is stored - /// in this range of memory. + /// The starting storage key location is stored in this range of memory. starting_storage_key_memory_range: CheckedMemConstLen<{ Bytes32::LEN }>, - /// The source data memory address is - /// stored in this range of memory. - source_address_memory_range: Range, + /// The source data memory address is stored in this range of memory. + source_address_memory_range: MemoryRange, } impl StateWriteQWord { @@ -1105,20 +1063,18 @@ impl StateWriteQWord { source_memory_address: Word, num_slots: Word, ) -> Result { - let source_end = checked_add_word( + let source_address_memory_range = MemoryRange::new( source_memory_address, - Bytes32::LEN.saturating_mul(num_slots as usize) as Word, + (Bytes32::LEN as Word).saturating_mul(num_slots), )?; + let starting_storage_key_memory_range = CheckedMemConstLen::<{ Bytes32::LEN }>::new( starting_storage_key_memory_address, )?; - if source_end > VM_MAX_RAM { - return Err(PanicReason::MemoryOverflow.into()) - } + Ok(Self { - source_address_memory_range: (source_memory_address as usize) - ..(source_end as usize), + source_address_memory_range, starting_storage_key_memory_range, }) } @@ -1135,7 +1091,7 @@ fn state_write_qword( let destination_key = Bytes32::from_bytes_ref(input.starting_storage_key_memory_range.read(memory)); - let values: Vec<_> = memory[input.source_address_memory_range] + let values: Vec<_> = memory[input.source_address_memory_range.usizes()] .chunks_exact(Bytes32::LEN) .flat_map(|chunk| Some(Bytes32::from(<[u8; 32]>::try_from(chunk).ok()?))) .collect(); diff --git a/fuel-vm/src/interpreter/blockchain/test.rs b/fuel-vm/src/interpreter/blockchain/test.rs index 7f39be064..9939cad21 100644 --- a/fuel-vm/src/interpreter/blockchain/test.rs +++ b/fuel-vm/src/interpreter/blockchain/test.rs @@ -1,3 +1,5 @@ +use std::ops::Range; + use crate::{ context::Context, interpreter::memory::Memory, diff --git a/fuel-vm/src/interpreter/contract.rs b/fuel-vm/src/interpreter/contract.rs index 92faf9fd7..24a9cc54f 100644 --- a/fuel-vm/src/interpreter/contract.rs +++ b/fuel-vm/src/interpreter/contract.rs @@ -7,6 +7,7 @@ use super::{ set_variable_output, AppendReceipt, }, + memory::read_bytes, ExecutableTransaction, Interpreter, RuntimeBalances, @@ -51,7 +52,6 @@ use fuel_types::{ ContractId, }; -use crate::interpreter::memory::read_bytes; use std::borrow::Cow; #[cfg(test)] diff --git a/fuel-vm/src/interpreter/crypto.rs b/fuel-vm/src/interpreter/crypto.rs index 81ceab4c3..9edcee829 100644 --- a/fuel-vm/src/interpreter/crypto.rs +++ b/fuel-vm/src/interpreter/crypto.rs @@ -17,13 +17,9 @@ use crate::{ constraints::reg_key::*, consts::*, error::RuntimeError, + prelude::MemoryRange, }; -use crate::arith::{ - checked_add_word, - checked_sub_word, -}; -use fuel_asm::PanicReason; use fuel_crypto::{ Hasher, Message, @@ -186,21 +182,10 @@ pub(crate) fn keccak256( Digest, Keccak256, }; - - let bc = checked_add_word(b, c)?; - - if a > checked_sub_word(VM_MAX_RAM, Bytes32::LEN as Word)? - || c > MEM_MAX_ACCESS_SIZE - || bc > MIN_VM_MAX_RAM_USIZE_MAX - { - return Err(PanicReason::MemoryOverflow.into()) - } - - let (a, b, bc) = (a as usize, b as usize, bc as usize); + let src_range = MemoryRange::new(b, c)?; let mut h = Keccak256::new(); - - h.update(&memory[b..bc]); + h.update(&memory[src_range.usizes()]); try_mem_write(a, h.finalize().as_slice(), owner, memory)?; @@ -215,18 +200,14 @@ pub(crate) fn sha256( b: Word, c: Word, ) -> Result<(), RuntimeError> { - let bc = checked_add_word(b, c)?; - - if a > checked_sub_word(VM_MAX_RAM, Bytes32::LEN as Word)? - || c > MEM_MAX_ACCESS_SIZE - || bc > MIN_VM_MAX_RAM_USIZE_MAX - { - return Err(PanicReason::MemoryOverflow.into()) - } - - let (a, b, bc) = (a as usize, b as usize, bc as usize); - - try_mem_write(a, Hasher::hash(&memory[b..bc]).as_ref(), owner, memory)?; + let src_range = MemoryRange::new(b, c)?; + + try_mem_write( + a, + Hasher::hash(&memory[src_range.usizes()]).as_ref(), + owner, + memory, + )?; inc_pc(pc) } diff --git a/fuel-vm/src/interpreter/executors/instruction.rs b/fuel-vm/src/interpreter/executors/instruction.rs index 11a302bf3..536080daf 100644 --- a/fuel-vm/src/interpreter/executors/instruction.rs +++ b/fuel-vm/src/interpreter/executors/instruction.rs @@ -1,5 +1,4 @@ use crate::{ - consts::*, error::{ InterpreterError, RuntimeError, @@ -33,32 +32,26 @@ where S: InterpreterStorage, Tx: ExecutableTransaction, { - /// Execute the current instruction pair located in `$m[$pc]`. + /// Execute the current instruction located in `$m[$pc]`. pub fn execute(&mut self) -> Result { - // Safety: `chunks_exact` is guaranteed to return a well-formed slice - let [hi, lo] = self.memory[self.registers[RegId::PC] as usize..] - .chunks_exact(WORD_SIZE) - .next() - .map(|b| b.try_into().expect("Has to be correct size slice")) - .map(Word::from_be_bytes) - .map(fuel_asm::raw_instructions_from_word) - .ok_or(InterpreterError::Panic(PanicReason::MemoryOverflow))?; - - // Store the expected `$pc` after executing `hi` - let pc = self.registers[RegId::PC] + Instruction::SIZE as Word; - let state = self.instruction(hi)?; - - // TODO optimize - // Should execute `lo` only if there is no rupture in the flow - that means - // either a breakpoint or some instruction that would skip `lo` such as - // `RET`, `JI` or `CALL` - if self.registers[RegId::PC] == pc && state.should_continue() { - self.instruction(lo) + if let Some(raw_instruction) = self.fetch_instruction() { + self.instruction(raw_instruction) } else { - Ok(state) + Err(InterpreterError::Panic(PanicReason::MemoryOverflow)) } } + /// Reads the current instruction located in `$m[$pc]`, + /// returning `None` on any memory access violation. + fn fetch_instruction(&self) -> Option { + let start: usize = self.registers[RegId::PC].try_into().ok()?; + let end = start.checked_add(Instruction::SIZE)?; + let bytes = self.memory.get(start..end)?; + Some(RawInstruction::from_be_bytes( + bytes.try_into().expect("Slice len mismatch"), + )) + } + /// Execute a provided instruction pub fn instruction + Copy>( &mut self, diff --git a/fuel-vm/src/interpreter/executors/main.rs b/fuel-vm/src/interpreter/executors/main.rs index a132b2901..ddb50b90e 100644 --- a/fuel-vm/src/interpreter/executors/main.rs +++ b/fuel-vm/src/interpreter/executors/main.rs @@ -7,7 +7,6 @@ use crate::{ IntoChecked, ParallelExecutor, }, - consts::*, context::Context, error::{ Bug, @@ -601,10 +600,6 @@ where pub(crate) fn run_program(&mut self) -> Result { loop { - if self.registers[RegId::PC] >= VM_MAX_RAM { - return Err(InterpreterError::Panic(PanicReason::MemoryOverflow)) - } - // Check whether the instruction will be executed in a call context let in_call = !self.frames.is_empty(); diff --git a/fuel-vm/src/interpreter/flow.rs b/fuel-vm/src/interpreter/flow.rs index 6750876f2..0b99005b1 100644 --- a/fuel-vm/src/interpreter/flow.rs +++ b/fuel-vm/src/interpreter/flow.rs @@ -228,18 +228,14 @@ impl RetCtx<'_> { } pub(crate) fn ret_data(self, a: Word, b: Word) -> Result { - if b > MEM_MAX_ACCESS_SIZE || a > VM_MAX_RAM - b { - return Err(PanicReason::MemoryOverflow.into()) - } - - let ab = (a + b) as usize; + let range = MemoryRange::new(a, b)?; let receipt = Receipt::return_data( self.current_contract.unwrap_or_else(ContractId::zeroed), a, self.registers[RegId::PC], self.registers[RegId::IS], - self.append.memory[a as usize..ab].to_vec(), + self.append.memory[range.usizes()].to_vec(), ); let digest = *receipt .digest() diff --git a/fuel-vm/src/interpreter/flow/ret_tests.rs b/fuel-vm/src/interpreter/flow/ret_tests.rs index 7c863b87f..c59beb93d 100644 --- a/fuel-vm/src/interpreter/flow/ret_tests.rs +++ b/fuel-vm/src/interpreter/flow/ret_tests.rs @@ -102,7 +102,7 @@ fn test_return() { &mut memory, &mut context, ) - .ret_data(0, MEM_MAX_ACCESS_SIZE + 1); + .ret_data(0, VM_MAX_RAM + 1); assert_eq!( r, Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)) diff --git a/fuel-vm/src/interpreter/internal.rs b/fuel-vm/src/interpreter/internal.rs index 633c86d63..e9ab226b7 100644 --- a/fuel-vm/src/interpreter/internal.rs +++ b/fuel-vm/src/interpreter/internal.rs @@ -239,7 +239,7 @@ pub(crate) fn set_flag( pub(crate) fn inc_pc(mut pc: RegMut) -> Result<(), RuntimeError> { pc.checked_add(Instruction::SIZE as Word) - .ok_or_else(|| PanicReason::ArithmeticOverflow.into()) + .ok_or_else(|| PanicReason::MemoryOverflow.into()) .map(|i| *pc = i) } diff --git a/fuel-vm/src/interpreter/log.rs b/fuel-vm/src/interpreter/log.rs index d69d150ea..f8d789da0 100644 --- a/fuel-vm/src/interpreter/log.rs +++ b/fuel-vm/src/interpreter/log.rs @@ -8,6 +8,7 @@ use super::{ receipts::ReceiptsCtx, ExecutableTransaction, Interpreter, + MemoryRange, }; use crate::{ constraints::reg_key::*, @@ -16,7 +17,6 @@ use crate::{ error::RuntimeError, }; -use fuel_asm::PanicReason; use fuel_tx::{ Receipt, Script, @@ -126,11 +126,7 @@ impl LogInput<'_> { c: Word, d: Word, ) -> Result<(), RuntimeError> { - if d > MEM_MAX_ACCESS_SIZE || c > VM_MAX_RAM - d { - return Err(PanicReason::MemoryOverflow.into()) - } - - let cd = (c + d) as usize; + let range = MemoryRange::new(c, d)?; let receipt = Receipt::log_data( internal_contract_or_default(self.context, self.fp, self.memory), @@ -139,7 +135,7 @@ impl LogInput<'_> { c, *self.pc, *self.is, - self.memory[c as usize..cd].to_vec(), + self.memory[range.usizes()].to_vec(), ); append_receipt( diff --git a/fuel-vm/src/interpreter/memory.rs b/fuel-vm/src/interpreter/memory.rs index 785bed204..0bad779d2 100644 --- a/fuel-vm/src/interpreter/memory.rs +++ b/fuel-vm/src/interpreter/memory.rs @@ -43,7 +43,7 @@ pub trait ToAddr { impl ToAddr for usize { fn to_addr(self) -> Result { - if self >= MEM_SIZE { + if self > MEM_SIZE { return Err(PanicReason::MemoryOverflow.into()) } Ok(self) @@ -109,6 +109,31 @@ impl MemoryRange { Self::new(address, SIZE) } + /// Uses a overflow-capturing operator to combine `base` and `offset` + /// into the start of a memory range, returning an error on overflow, + /// and then checks that the resulting range is within the VM memory. + pub fn new_overflowing_op( + overflowing_op: F, + base: T, + offset: T, + size: A, + ) -> Result + where + F: FnOnce(T, T) -> (T, bool), + { + let (addr, overflow) = overflowing_op(base, offset); + if overflow { + return Err(PanicReason::MemoryOverflow.into()) + } + Self::new(addr, size) + } + + /// Truncate a memory range to a new size if it's smaller then current one. + pub fn truncated(&self, limit: usize) -> Self { + Self::new(self.start, self.len().min(limit)) + .expect("Cannot overflow on truncation") + } + /// Return `true` if the length is `0`. pub fn is_empty(&self) -> bool { self.len() == 0 @@ -294,15 +319,9 @@ pub(crate) fn load_byte( b: Word, c: Word, ) -> Result<(), RuntimeError> { - let bc = b.saturating_add(c) as usize; - - if bc >= VM_MAX_RAM as RegisterId { - Err(PanicReason::MemoryOverflow.into()) - } else { - *result = memory[bc] as Word; - - inc_pc(pc) - } + let range = MemoryRange::new_overflowing_op(Word::overflowing_add, b, c, 1u64)?; + *result = memory[range.start] as Word; + inc_pc(pc) } pub(crate) fn load_word( @@ -327,18 +346,8 @@ pub(crate) fn store_byte( b: Word, c: Word, ) -> Result<(), RuntimeError> { - let (ac, overflow) = a.overflowing_add(c); - let range = ac..(ac + 1); - if overflow - || ac >= VM_MAX_RAM - || !(owner.has_ownership_stack(&range) || owner.has_ownership_heap(&range)) - { - Err(PanicReason::MemoryOverflow.into()) - } else { - memory[ac as usize] = b as u8; - - inc_pc(pc) - } + write_bytes(memory, owner, a.saturating_add(c), [b as u8])?; + inc_pc(pc) } pub(crate) fn store_word( @@ -381,12 +390,9 @@ pub(crate) fn memclear( b: Word, ) -> Result<(), RuntimeError> { let range = MemoryRange::new(a, b)?; - if b > MEM_MAX_ACCESS_SIZE || !owner.has_ownership_range(&range) { - Err(PanicReason::MemoryOverflow.into()) - } else { - memory[range.usizes()].fill(0); - inc_pc(pc) - } + owner.verify_ownership(&range)?; + memory[range.usizes()].fill(0); + inc_pc(pc) } pub(crate) fn memcopy( @@ -400,13 +406,7 @@ pub(crate) fn memcopy( let dst_range = MemoryRange::new(a, c)?; let src_range = MemoryRange::new(b, c)?; - if c > MEM_MAX_ACCESS_SIZE { - return Err(PanicReason::MemoryOverflow.into()) - } - - if !owner.has_ownership_range(&dst_range) { - return Err(PanicReason::MemoryOwnership.into()) - } + owner.verify_ownership(&dst_range)?; if dst_range.start <= src_range.start && src_range.start < dst_range.end || src_range.start <= dst_range.start && dst_range.start < src_range.end @@ -436,18 +436,10 @@ pub(crate) fn memeq( c: Word, d: Word, ) -> Result<(), RuntimeError> { - let (bd, overflow) = b.overflowing_add(d); - let (cd, of) = c.overflowing_add(d); - let overflow = overflow || of; - - if overflow || bd > VM_MAX_RAM || cd > VM_MAX_RAM || d > MEM_MAX_ACCESS_SIZE { - Err(PanicReason::MemoryOverflow.into()) - } else { - *result = - (memory[b as usize..bd as usize] == memory[c as usize..cd as usize]) as Word; - - inc_pc(pc) - } + let range1 = MemoryRange::new(b, d)?; + let range2 = MemoryRange::new(c, d)?; + *result = (memory[range1.usizes()] == memory[range2.usizes()]) as Word; + inc_pc(pc) } #[derive(Debug)] @@ -474,6 +466,25 @@ impl OwnershipRegisters { } } + pub(crate) fn verify_ownership( + &self, + range: &MemoryRange, + ) -> Result<(), PanicReason> { + if self.has_ownership_range(range) { + Ok(()) + } else { + Err(PanicReason::MemoryOwnership) + } + } + + pub(crate) fn verify_internal_context(&self) -> Result<(), PanicReason> { + if self.context.is_internal() { + Ok(()) + } else { + Err(PanicReason::ExpectedInternalContext) + } + } + pub(crate) fn has_ownership_range(&self, range: &MemoryRange) -> bool { let range = range.words(); self.has_ownership_stack(&range) || self.has_ownership_heap(&range) @@ -520,15 +531,11 @@ impl OwnershipRegisters { pub(crate) fn try_mem_write( addr: A, data: &[u8], - registers: OwnershipRegisters, + owner: OwnershipRegisters, memory: &mut [u8; MEM_SIZE], ) -> Result<(), RuntimeError> { let range = MemoryRange::new(addr, data.len())?; - - if !registers.has_ownership_range(&range) { - return Err(PanicReason::MemoryOwnership.into()) - } - + owner.verify_ownership(&range)?; memory[range.usizes()].copy_from_slice(data); Ok(()) } @@ -536,15 +543,11 @@ pub(crate) fn try_mem_write( pub(crate) fn try_zeroize( addr: A, len: B, - registers: OwnershipRegisters, + owner: OwnershipRegisters, memory: &mut [u8; MEM_SIZE], ) -> Result<(), RuntimeError> { let range = MemoryRange::new(addr, len)?; - - if !registers.has_ownership_range(&range) { - return Err(PanicReason::MemoryOwnership.into()) - } - + owner.verify_ownership(&range)?; memory[range.usizes()].fill(0); Ok(()) } @@ -555,14 +558,9 @@ pub(crate) fn read_bytes( memory: &[u8; MEM_SIZE], addr: Word, ) -> Result<[u8; COUNT], RuntimeError> { - let addr = addr as usize; - let (end, overflow) = addr.overflowing_add(COUNT); - - if overflow || end > VM_MAX_RAM as RegisterId { - return Err(PanicReason::MemoryOverflow.into()) - } - - Ok(<[u8; COUNT]>::try_from(&memory[addr..end]).unwrap_or_else(|_| unreachable!())) + let range = MemoryRange::new_const::<_, COUNT>(addr)?; + Ok(<[u8; COUNT]>::try_from(&memory[range.usizes()]) + .unwrap_or_else(|_| unreachable!())) } /// Writes a constant-sized byte array to memory, performing overflow, memory range and @@ -574,10 +572,7 @@ pub(crate) fn write_bytes( bytes: [u8; COUNT], ) -> Result<(), RuntimeError> { let range = MemoryRange::new_const::<_, COUNT>(addr)?; - if !owner.has_ownership_range(&range) { - return Err(PanicReason::MemoryOverflow.into()) - } - + owner.verify_ownership(&range)?; memory[range.usizes()].copy_from_slice(&bytes); Ok(()) } diff --git a/fuel-vm/src/interpreter/memory/allocation_tests.rs b/fuel-vm/src/interpreter/memory/allocation_tests.rs index 55cf4da3a..4c09eac31 100644 --- a/fuel-vm/src/interpreter/memory/allocation_tests.rs +++ b/fuel-vm/src/interpreter/memory/allocation_tests.rs @@ -19,10 +19,10 @@ fn test_malloc(mut hp: Word, sp: Word, a: Word) -> Result { } #[test_case(true, 1, 10 => Ok(()); "Can clear some bytes")] -#[test_case(false, 1, 10 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "No ownership")] +#[test_case(false, 1, 10 => Err(RuntimeError::Recoverable(PanicReason::MemoryOwnership)); "No ownership")] #[test_case(true, 0, 10 => Ok(()); "Memory range starts at 0")] #[test_case(true, MEM_SIZE as Word - 10, 10 => Ok(()); "Memory range ends at last address")] -#[test_case(true, 1, MEM_MAX_ACCESS_SIZE + 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Memory range size exceeds limit")] +#[test_case(true, 1, VM_MAX_RAM + 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Memory range size exceeds limit")] fn test_memclear(has_ownership: bool, a: Word, b: Word) -> Result<(), RuntimeError> { let mut memory: Memory = vec![1u8; MEM_SIZE].try_into().unwrap(); let mut pc = 4; @@ -92,11 +92,11 @@ fn test_memcopy( #[test_case(1, 20, 10 => Ok(()); "Can compare some bytes")] #[test_case(MEM_SIZE as Word, 1, 2 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "b+d > MAX_RAM")] #[test_case(1, MEM_SIZE as Word, 2 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "c+d > MAX_RAM")] -#[test_case(1, 1, MEM_MAX_ACCESS_SIZE + 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "d > MEM_MAX_ACCESS_SIZE")] +#[test_case(1, 1, VM_MAX_RAM + 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "d > VM_MAX_RAM")] #[test_case(u64::MAX/2, 1, u64::MAX/2 + 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "b+d overflows")] #[test_case(1, u64::MAX/2, u64::MAX/2 + 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "c+d overflows")] #[test_case(0, 0, 0 => Ok(()); "smallest input values")] -#[test_case(0, MEM_MAX_ACCESS_SIZE/2, MEM_MAX_ACCESS_SIZE/2 => Ok(()); "maximum range of addressable memory")] +#[test_case(0, VM_MAX_RAM/2, VM_MAX_RAM/2 => Ok(()); "maximum range of addressable memory")] fn test_memeq(b: Word, c: Word, d: Word) -> Result<(), RuntimeError> { let mut memory: Memory = vec![1u8; MEM_SIZE].try_into().unwrap(); let r = (b as usize).min(MEM_SIZE) @@ -208,7 +208,7 @@ fn test_load_word(b: Word, c: Word) -> Result<(), RuntimeError> { #[test_case(true, 20, 30, 40 => Ok(()); "Can store a byte")] #[test_case(false, VM_MAX_RAM - 1, 100, 2 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Memory overflow on heap")] -#[test_case(false, 0, 100, VM_MAX_RAM - 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Memory overflow on stack")] +#[test_case(false, 0, 100, VM_MAX_RAM - 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOwnership)); "Memory overflow on stack")] #[test_case(true, VM_MAX_RAM, 1, 1 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Memory overflow by address range")] fn test_store_byte( has_ownership: bool, @@ -240,9 +240,44 @@ fn test_store_byte( Ok(()) } +#[rstest::rstest] +fn test_store_byte_more( + #[values(0, 1, VM_MAX_RAM - 1, VM_MAX_RAM)] a: Word, + #[values(0, 1, 0xff, 0x100)] b: Word, + #[values(0, 1, 2)] c: Word, +) -> Result<(), RuntimeError> { + let mut memory: Memory = vec![1u8; MEM_SIZE].try_into().unwrap(); + let mut pc = 4; + + // Full ownership in heap + let owner = OwnershipRegisters { + sp: 0, + ssp: 0, + hp: 0, + prev_hp: VM_MAX_RAM, + context: Context::Script { + block_height: Default::default(), + }, + }; + + let is_error = a + c >= memory.len() as u64; + match store_byte(&mut memory, owner, RegMut::new(&mut pc), a, b, c) { + Ok(_) => { + assert!(!is_error); + assert_eq!(memory[(a + c) as usize], b as u8); + } + Err(e) => { + assert!(is_error); + assert_eq!(e, RuntimeError::Recoverable(PanicReason::MemoryOverflow)); + } + } + + Ok(()) +} + #[test_case(true, 20, 30, 40 => Ok(()); "Can store a word")] #[test_case(true, 20, 30, VM_MAX_RAM => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Fails due to memory overflow")] -#[test_case(false, 20, 30, 40 => Err(RuntimeError::Recoverable(PanicReason::MemoryOverflow)); "Fails due to not having ownership of the range")] +#[test_case(false, 20, 30, 40 => Err(RuntimeError::Recoverable(PanicReason::MemoryOwnership)); "Fails due to not having ownership of the range")] fn test_store_word( has_ownership: bool, a: Word, diff --git a/fuel-vm/src/interpreter/memory/tests.rs b/fuel-vm/src/interpreter/memory/tests.rs index 0b0b75cd6..13980a537 100644 --- a/fuel-vm/src/interpreter/memory/tests.rs +++ b/fuel-vm/src/interpreter/memory/tests.rs @@ -193,7 +193,7 @@ fn stack_alloc_ownership() { fn test_ownership(reg: OwnershipRegisters, range: Range) -> bool { let range = MemoryRange::new(range.start, range.end - range.start).expect("Invalid range"); - reg.has_ownership_range(&range) + reg.verify_ownership(&range).is_ok() } fn set_index(index: usize, val: u8, mut array: [u8; 100]) -> [u8; 100] { diff --git a/fuel-vm/src/tests/blockchain.rs b/fuel-vm/src/tests/blockchain.rs index 2b4841ca4..73ee8daf9 100644 --- a/fuel-vm/src/tests/blockchain.rs +++ b/fuel-vm/src/tests/blockchain.rs @@ -35,7 +35,6 @@ use fuel_asm::{ op, Instruction, PanicReason::{ - ArithmeticOverflow, ContractNotInInputs, ErrorFlag, ExpectedUnallocatedStack, @@ -701,7 +700,7 @@ fn code_root_a_plus_32_overflow() { op::croo(reg_a, RegId::ZERO), ]; - check_expected_reason_for_instructions(code_root, ArithmeticOverflow); + check_expected_reason_for_instructions(code_root, MemoryOverflow); } #[test] @@ -1160,7 +1159,7 @@ fn state_w_qword_b_plus_32_over() { op::swwq(RegId::ZERO, SET_STATUS_REG, reg_a, RegId::ONE), ]; - check_expected_reason_for_instructions(state_write_qword, ArithmeticOverflow); + check_expected_reason_for_instructions(state_write_qword, MemoryOverflow); } #[test]