diff --git a/src/module/mod.rs b/src/module/mod.rs index 972caece..536832d6 100644 --- a/src/module/mod.rs +++ b/src/module/mod.rs @@ -65,6 +65,14 @@ impl RwasmModule { Self::new_checked(sink).unwrap_or_else(|_| unreachable!("rwasm: malformed rwasm binary")) } + /// Decodes one rWasm module and returns the number of bytes consumed. + /// + /// # Note + /// + /// "Checked" refers to the binary encoding only: this performs **no** structural validation of + /// the decoded module. Branch targets, call targets, segment indices, and stack offsets are all + /// taken at face value, so a module accepted here can still trap at any point during execution. + /// Use [`RwasmModule::new_verified`] for bytecode that this crate did not produce itself. pub fn new_checked(sink: &[u8]) -> Result<(Self, usize), DecodeError> { let (inner, bytes_read): (RwasmModuleInner, usize) = bincode::decode_from_slice(sink, bincode::config::legacy())?; @@ -72,6 +80,11 @@ impl RwasmModule { } /// Decodes exactly one rWasm module and rejects trailing bytes. + /// + /// # Note + /// + /// Just like [`RwasmModule::new_checked`], this validates the encoding but not the structure + /// of the decoded module. pub fn new_checked_exact(sink: &[u8]) -> Result { let (module, bytes_read) = Self::new_checked(sink)?; if bytes_read != sink.len() { diff --git a/src/module/verification.rs b/src/module/verification.rs index e9254dcd..31fc0ebf 100644 --- a/src/module/verification.rs +++ b/src/module/verification.rs @@ -1,7 +1,30 @@ use super::{RwasmModule, RwasmModuleInner}; -use crate::{InstructionSet, Opcode, N_MAX_DATA_SEGMENTS, N_MAX_ELEM_SEGMENTS, N_MAX_TABLES}; +use crate::{ + InstructionSet, Opcode, N_MAX_DATA_SEGMENTS, N_MAX_ELEM_SEGMENTS, N_MAX_STACK_SIZE, + N_MAX_TABLES, +}; +use alloc::{vec, vec::Vec}; use bincode::error::DecodeError; +/// Returns how many cells an instruction may address away from the current stack pointer. +/// +/// # Note +/// +/// A well-formed module never reaches further than the largest stack window it reserves: pushes +/// are covered by the `StackCheck` of their own function, and reads below the stack pointer target +/// parameters that some caller had to push within *its* reserved window. [`N_MAX_STACK_SIZE`] is +/// the floor, because that many cells are addressable without reserving anything at all. +fn max_stack_reach(code: &InstructionSet) -> i64 { + code.iter() + .filter_map(|opcode| match opcode { + Opcode::StackCheck(reserved) => Some(i64::from(*reserved)), + _ => None, + }) + .max() + .unwrap_or(0) + .max(N_MAX_STACK_SIZE as i64) +} + #[derive(Debug, Clone, Eq, PartialEq)] pub enum RwasmModuleVerificationError { EmptyCodeSection, @@ -54,6 +77,22 @@ pub enum RwasmModuleVerificationError { InvalidTableIndexPayload { pc: usize, }, + /// An opcode pops more values than the value stack can ever hold at this point. + StackUnderflow { + pc: usize, + height: i64, + pops: u32, + }, + /// An opcode pushes past the maximum height the value stack can ever reach. + StackOverflow { + pc: usize, + height: i64, + pushes: u32, + }, + /// Execution can run past the end of the code section. + FallsThroughCodeSection { + pc: usize, + }, } #[derive(Debug)] @@ -110,9 +149,273 @@ fn verify_module(module: &RwasmModuleInner) -> Result<(), RwasmModuleVerificatio for (pc, opcode) in code.iter().copied().enumerate() { verify_opcode(code, pc, opcode)?; } + verify_stack_usage(module)?; + Ok(()) +} + +/// The emulated height of the value stack at a given program counter. +/// +/// Heights are relative to the entry of the enclosing function, which is why they can legitimately +/// turn negative: a callee reads its parameters from below its own entry stack pointer and drops +/// them again before returning. +#[derive(Copy, Clone, PartialEq, Eq)] +enum Height { + /// The program counter has not been reached yet. + Unvisited, + Known(i64), + /// The height depends on the signature of a host function, which is not part of the module. + Unknown, +} + +impl Height { + /// Merges two heights reaching the same program counter. + fn merge(self, other: Self) -> Self { + match (self, other) { + (Self::Unvisited, other) => other, + (current, Self::Unvisited) => current, + (Self::Known(lhs), Self::Known(rhs)) if lhs == rhs => Self::Known(lhs), + _ => Self::Unknown, + } + } +} + +/// Verifies that no reachable opcode addresses the value stack outside of its bounds. +/// +/// # Note +/// +/// This is an abstract interpretation of the whole code section: every function entry seeds an +/// emulated stack height of zero, which is then propagated along all control flow edges. Opcodes +/// are rejected when they provably reach further than [`max_stack_reach`] cells away from the +/// stack pointer. +/// +/// The pass is deliberately conservative. rWasm does not record function signatures, so the +/// emulated height becomes [`Height::Unknown`] after a call, and from there on only the checks +/// that hold for every possible height are applied. Whatever slips through is still caught at +/// runtime by the bounds checks in [`ValueStackPtr`]. +/// +/// [`ValueStackPtr`]: crate::ValueStackPtr +fn verify_stack_usage(module: &RwasmModuleInner) -> Result<(), RwasmModuleVerificationError> { + let code = &module.code_section; + let reach = max_stack_reach(code); + let mut heights = vec![Height::Unvisited; code.len()]; + let mut worklist: Vec<(usize, Height)> = function_entries(module) + .into_iter() + .map(|entry| (entry, Height::Known(0))) + .collect(); + + while let Some((pc, incoming)) = worklist.pop() { + let current = heights[pc]; + let merged = current.merge(incoming); + if merged == current { + continue; + } + heights[pc] = merged; + let opcode = code[pc]; + let height = verify_stack_effect(pc, opcode, merged, reach)?; + for successor in successors(pc, opcode) { + if successor >= code.len() { + return Err(RwasmModuleVerificationError::FallsThroughCodeSection { pc }); + } + worklist.push((successor, height)); + } + } + Ok(()) +} + +/// Collects every program counter execution can enter a function at. +fn function_entries(module: &RwasmModuleInner) -> Vec { + // The constructor runs from the very beginning of the code section, `source_pc` skips it. + let mut entries = vec![0usize, module.source_pc as usize]; + for opcode in module.code_section.iter().copied() { + let target = match opcode { + Opcode::CallInternal(target) | Opcode::ReturnCallInternal(target) => target, + // A null function reference is never called. + Opcode::RefFunc(target) if target != 0 => target, + _ => continue, + }; + entries.push(target as usize); + } + entries.extend( + module + .elem_section + .iter() + .copied() + .filter(|target| *target != 0) + .map(|target| target as usize), + ); + // Targets are range checked by `verify_opcode` and `verify_module` before we get here. + entries.retain(|entry| *entry < module.code_section.len()); + entries.sort_unstable(); + entries.dedup(); + entries +} + +/// Applies the stack effect of `opcode` to `height` and rejects out-of-bounds accesses. +fn verify_stack_effect( + pc: usize, + opcode: Opcode, + height: Height, + reach: i64, +) -> Result { + let (pops, pushes) = stack_effect(opcode); + let Height::Known(height) = height else { + // Without a height, only the bounds that hold for every possible height apply. + verify_local_depth(pc, opcode, 0, reach)?; + return Ok(Height::Unknown); + }; + + // `LocalSet` writes below the value it just popped, every other local opcode addresses the + // stack as it found it. + let depth_height = match opcode { + Opcode::LocalSet(_) => height - 1, + _ => height, + }; + verify_local_depth(pc, opcode, depth_height, reach)?; + + if height - i64::from(pops) < -reach { + return Err(RwasmModuleVerificationError::StackUnderflow { pc, height, pops }); + } + let height = height - i64::from(pops); + if height + i64::from(pushes) > reach { + return Err(RwasmModuleVerificationError::StackOverflow { pc, height, pushes }); + } + if is_call(opcode) { + // A call swaps the callee's parameters for its results, and rWasm records neither. + return Ok(Height::Unknown); + } + Ok(Height::Known(height + i64::from(pushes))) +} + +/// Returns `true` if `opcode` transfers control to a function whose signature is unknown here. +fn is_call(opcode: Opcode) -> bool { + matches!( + opcode, + Opcode::CallInternal(_) | Opcode::CallIndirect(_) | Opcode::Call(_) | Opcode::ReturnCall(_) + ) +} + +/// Rejects local depths that address a cell outside the value stack. +/// +/// A depth of zero addresses the free cell above the stack pointer, which is never a valid local. +fn verify_local_depth( + pc: usize, + opcode: Opcode, + height: i64, + reach: i64, +) -> Result<(), RwasmModuleVerificationError> { + let (Opcode::LocalGet(depth) | Opcode::LocalSet(depth) | Opcode::LocalTee(depth)) = opcode + else { + return Ok(()); + }; + if depth == 0 || i64::from(depth) > height + reach { + return Err(RwasmModuleVerificationError::LocalDepthOutOfBounds { pc, depth }); + } Ok(()) } +/// Returns the program counters execution can continue at after `opcode`. +fn successors(pc: usize, opcode: Opcode) -> Vec { + match opcode { + // Execution either leaves the function or resumes at a program counter that is seeded as + // a function entry in its own right. + Opcode::Unreachable + | Opcode::Trap(_) + | Opcode::Return + | Opcode::ReturnCallInternal(_) + | Opcode::ReturnCallIndirect(_) => vec![], + Opcode::Br(offset) => vec![branch_target(pc, offset.to_i32())], + Opcode::BrIfEqz(offset) | Opcode::BrIfNez(offset) => { + vec![pc + 1, branch_target(pc, offset.to_i32())] + } + // A branch table is followed by `targets` pairs of opcodes, and the interpreter jumps to + // the first opcode of the selected pair. + Opcode::BrTable(targets) => (0..targets as usize).map(|i| pc + 2 * i + 1).collect(), + // `CallIndirect` and `TableInit` carry the table index in the following opcode. + Opcode::CallIndirect(_) | Opcode::TableInit(_) => vec![pc + 2], + _ => vec![pc + 1], + } +} + +/// Resolves a branch target that [`verify_branch_target`] already proved to be in bounds. +fn branch_target(pc: usize, offset: i32) -> usize { + (pc as i64 + offset as i64) as usize +} + +/// Returns how many values `opcode` pops from and pushes onto the value stack. +/// +/// # Note +/// +/// Values are counted in stack cells, so the 64 bit opcodes account for the two cells an `i64` +/// or an `f64` occupies. +fn stack_effect(opcode: Opcode) -> (u32, u32) { + use Opcode::*; + match opcode { + // stack/system + Unreachable + | Trap(_) + | Br(_) + | ConsumeFuel(_) + | SignatureCheck(_) + | StackCheck(_) + | DataDrop(_) + | ElemDrop(_) + | Return + | ReturnCallInternal(_) => (0, 0), + LocalGet(_) | RefFunc(_) | I32Const(_) | GlobalGet(_) | MemorySize | TableSize(_) => (0, 1), + LocalSet(_) | Drop | GlobalSet(_) | ConsumeFuelStack | BrIfEqz(_) | BrIfNez(_) + | BrTable(_) => (1, 0), + LocalTee(_) => (1, 1), + Select => (3, 1), + BulkConst(locals) => (0, locals), + BulkDrop(locals) => (locals, 0), + // A call pops the callee's parameters and pushes its results, but rWasm records neither; + // `verify_stack_effect` turns the height into `Height::Unknown` instead. + Call(_) | ReturnCall(_) | CallInternal(_) => (0, 0), + // The indirect opcodes additionally pop the function index off the stack. + CallIndirect(_) | ReturnCallIndirect(_) => (1, 0), + + // memory + I32Load(_) | I32Load8S(_) | I32Load8U(_) | I32Load16S(_) | I32Load16U(_) | MemoryGrow => { + (1, 1) + } + I32Store(_) | I32Store8(_) | I32Store16(_) => (2, 0), + MemoryFill | MemoryCopy | MemoryInit(_) => (3, 0), + + // table + TableGet(_) => (1, 1), + TableSet(_) => (2, 0), + TableGrow(_) => (2, 1), + TableFill(_) | TableCopy(_, _) | TableInit(_) => (3, 0), + + // alu + I32Eqz | I32Clz | I32Ctz | I32Popcnt | I32WrapI64 | I32Extend8S | I32Extend16S => (1, 1), + I32Mul64 | I32Add64 => (2, 2), + _ if opcode.is_binary_instruction() => (2, 1), + + // fpu + F32Load(_) | F32Abs | F32Neg | F32Ceil | F32Floor | F32Trunc | F32Nearest | F32Sqrt + | I32TruncF32S | I32TruncF32U | I32TruncSatF32S | I32TruncSatF32U | F32ConvertI32S + | F32ConvertI32U => (1, 1), + F64Load(_) | I64TruncF32S | I64TruncF32U | I64TruncSatF32S | I64TruncSatF32U + | F64ConvertI32S | F64ConvertI32U | F64PromoteF32 => (1, 2), + F32Store(_) => (2, 0), + I32TruncF64S | I32TruncF64U | I32TruncSatF64S | I32TruncSatF64U | F32ConvertI64S + | F32ConvertI64U | F32DemoteF64 => (2, 1), + F64Abs | F64Neg | F64Ceil | F64Floor | F64Trunc | F64Nearest | F64Sqrt | I64TruncF64S + | I64TruncF64U | I64TruncSatF64S | I64TruncSatF64U | F64ConvertI64S | F64ConvertI64U => { + (2, 2) + } + F64Store(_) => (3, 0), + F32Eq | F32Ne | F32Lt | F32Gt | F32Le | F32Ge | F32Add | F32Sub | F32Mul | F32Div + | F32Min | F32Max | F32Copysign => (2, 1), + F64Eq | F64Ne | F64Lt | F64Gt | F64Le | F64Ge => (4, 1), + F64Add | F64Sub | F64Mul | F64Div | F64Min | F64Max | F64Copysign => (4, 2), + + // Unary integer opcodes not covered above keep the height as it is. + _ => (1, 1), + } +} + fn verify_opcode( code: &InstructionSet, pc: usize, @@ -386,6 +689,129 @@ mod tests { ); } + #[test] + fn rejects_local_depth_beyond_the_value_stack() { + let depth = 2 * N_MAX_STACK_SIZE as u32; + for code in [ + instruction_set! { LocalGet(depth) Return }, + instruction_set! { I32Const(1) LocalSet(depth) Return }, + instruction_set! { I32Const(1) LocalTee(depth) Return }, + ] { + let pc = code.len() - 2; + assert_eq!( + verification_error(module_with_code(code)), + RwasmModuleVerificationError::LocalDepthOutOfBounds { pc, depth } + ); + } + } + + #[test] + fn rejects_local_depth_beyond_the_value_stack_after_a_host_call() { + // A host call leaves the emulated height unknown, the constant bound still applies. + let depth = N_MAX_STACK_SIZE as u32 + 1; + assert_eq!( + verification_error(module_with_code( + instruction_set! { Call(70) LocalGet(depth) Return } + )), + RwasmModuleVerificationError::LocalDepthOutOfBounds { pc: 1, depth } + ); + } + + #[test] + fn rejects_popping_below_the_value_stack() { + let mut code = InstructionSet::new(); + for _ in 0..=N_MAX_STACK_SIZE { + code.op_drop(); + } + code.op_return(); + assert_eq!( + verification_error(module_with_code(code)), + RwasmModuleVerificationError::StackUnderflow { + pc: N_MAX_STACK_SIZE, + height: -(N_MAX_STACK_SIZE as i64), + pops: 1, + } + ); + } + + #[test] + fn rejects_pushing_beyond_the_value_stack() { + let mut code = InstructionSet::new(); + for _ in 0..=N_MAX_STACK_SIZE { + code.op_i32_const(1); + } + code.op_return(); + assert_eq!( + verification_error(module_with_code(code)), + RwasmModuleVerificationError::StackOverflow { + pc: N_MAX_STACK_SIZE, + height: N_MAX_STACK_SIZE as i64, + pushes: 1, + } + ); + } + + #[test] + fn rejects_bulk_operands_beyond_the_value_stack() { + let operand = N_MAX_STACK_SIZE as u32 + 1; + assert_eq!( + verification_error(module_with_code( + instruction_set! { BulkConst(operand) Return } + )), + RwasmModuleVerificationError::StackOverflow { + pc: 0, + height: 0, + pushes: operand, + } + ); + assert_eq!( + verification_error(module_with_code( + instruction_set! { BulkDrop(operand) Return } + )), + RwasmModuleVerificationError::StackUnderflow { + pc: 0, + height: 0, + pops: operand, + } + ); + } + + #[test] + fn accepts_bulk_operands_covered_by_a_stack_reservation() { + // A Wasm function may declare far more locals than the default stack holds; the module + // says so through its `StackCheck`, and the reservation itself traps at runtime. + let operand = N_MAX_STACK_SIZE as u32 + 1; + let module = module_with_code(instruction_set! { + StackCheck(operand) + BulkConst(operand) + BulkDrop(operand) + Return + }); + assert_eq!(module.verify(), Ok(())); + } + + #[test] + fn rejects_code_running_past_the_code_section() { + assert_eq!( + verification_error(module_with_code(instruction_set! { I32Const(1) Drop })), + RwasmModuleVerificationError::FallsThroughCodeSection { pc: 1 } + ); + } + + #[test] + fn accepts_locals_addressed_below_the_function_entry() { + // A callee reads its parameters from below its own entry stack pointer, so a local depth + // greater than the emulated height is perfectly normal. + let module = module_with_code(instruction_set! { + CallInternal(2) + Return + LocalGet(1) + LocalSet(2) + Return + }); + assert_eq!(module.verify(), Ok(())); + } + #[test] fn rejects_missing_table_index_payload() { assert_eq!( diff --git a/src/vm/executor.rs b/src/vm/executor.rs index e3b74cfe..3a540004 100644 --- a/src/vm/executor.rs +++ b/src/vm/executor.rs @@ -123,6 +123,12 @@ impl<'a, T> RwasmExecutor<'a, T> { for x in result.iter_mut().rev() { *x = self.sp.pop_value(x.ty()); } + if self.sp.is_out_of_bounds() { + self.value_stack.reset(); + self.call_stack.reset(); + self.store.last_signature = None; + return Err(TrapCode::StackOverflow); + } self.value_stack.sync_stack_ptr(self.sp); // Execution is over, make sure the stack is clear (it's guaranteed by wasm validation) debug_assert_eq!( @@ -192,8 +198,24 @@ impl<'a, T> RwasmExecutor<'a, T> { } } + /// Executes a single `instr` and reports whether the outermost `Return` was reached. + /// + /// # Errors + /// + /// With [`TrapCode::StackOverflow`] if `instr` addressed a cell outside the value stack. The + /// offending access itself was already suppressed by [`ValueStackPtr`], this only stops the + /// execution from carrying on with a corrupted stack. #[inline(always)] pub fn step(&mut self, instr: Opcode) -> Result { + let return_reached = self.execute(instr)?; + if self.sp.is_out_of_bounds() { + return Err(TrapCode::StackOverflow); + } + Ok(return_reached) + } + + #[inline(always)] + fn execute(&mut self, instr: Opcode) -> Result { use Opcode::*; match instr { Unreachable => self.visit_unreachable()?, diff --git a/src/vm/value_stack.rs b/src/vm/value_stack.rs index b4318a75..935343de 100644 --- a/src/vm/value_stack.rs +++ b/src/vm/value_stack.rs @@ -28,6 +28,14 @@ pub struct ValueStack { maximum_len: usize, /// The maximum stack height max_stack_height: usize, + /// Sticky flag raised once an operation tried to address a cell outside the value stack. + /// + /// # Note + /// + /// The flag travels with the [`ValueStackPtr`] handed out by [`ValueStack::stack_ptr`] and + /// comes back through [`ValueStack::sync_stack_ptr`], so the interpreter observes it no + /// matter how often it re-derives its stack pointer. + out_of_bounds: bool, } impl Debug for ValueStack { @@ -78,9 +86,20 @@ impl ValueStack { stack_ptr: 0, maximum_len: 0, max_stack_height: 0, + out_of_bounds: false, } } + /// Returns `true` if some operation tried to address a cell outside the value stack. + /// + /// # Note + /// + /// The offending access was suppressed, so this only reports that the executed bytecode is + /// invalid and that execution has to be aborted with [`TrapCode::StackOverflow`]. + pub fn is_out_of_bounds(&self) -> bool { + self.out_of_bounds + } + pub fn max_stack_height(&self) -> usize { self.max_stack_height } @@ -117,7 +136,8 @@ impl ValueStack { self.stack_ptr, self.capacity() ); - unsafe { self.entries.get_unchecked_mut(..self.stack_ptr) }.to_vec() + let len = self.stack_ptr.min(self.capacity()); + self.entries[..len].to_vec() } /// Returns the base [`ValueStackPtr`] of `self`. @@ -125,7 +145,12 @@ impl ValueStack { /// The returned [`ValueStackPtr`] points to the first value on the [`ValueStack`]. #[inline] fn base_ptr(&mut self) -> ValueStackPtr { - ValueStackPtr::new(self.entries.as_mut_ptr()) + let capacity = self.entries.len(); + let mut base = ValueStackPtr::new(self.entries.as_mut_ptr(), capacity); + if self.out_of_bounds { + base.mark_out_of_bounds(); + } + base } /// Synchronizes [`ValueStack`] with the new [`ValueStackPtr`]. @@ -133,7 +158,8 @@ impl ValueStack { pub fn sync_stack_ptr(&mut self, new_sp: ValueStackPtr) { let offset = new_sp.offset_from(self.base_ptr()); debug_assert!(offset >= 0, "stack underflow: {}", offset); - self.stack_ptr = offset as usize; + self.out_of_bounds |= new_sp.is_out_of_bounds(); + self.stack_ptr = offset.max(0) as usize; #[cfg(debug_assertions)] if self.stack_ptr > self.max_stack_height { self.max_stack_height = self.stack_ptr; @@ -175,37 +201,20 @@ impl ValueStack { stack_ptr: 0, maximum_len, max_stack_height: 0, + out_of_bounds: false, } } - /// Returns the [`UntypedValue`] at the given `index`. - /// - /// # Note - /// - /// This is an optimized convenience method that only asserts - /// that the index is within bounds in `debug` mode. - /// - /// # Safety - /// - /// This is safe since all rwasm bytecode has been validated - /// during translation and therefore cannot result in out-of-bounds accesses. - /// - /// # Panics (Debug) - /// - /// If the `index` is out of bounds. - #[inline] - fn get_release_unchecked_mut(&mut self, index: usize) -> &mut UntypedValue { - debug_assert!(index < self.capacity()); - // Safety: This is safe since all rwasm bytecode has been validated - // during translation and therefore cannot result in out of - // bounds accesses. - unsafe { self.entries.get_unchecked_mut(index) } - } - - /// Drops the last value on the [`ValueStack`]. + /// Drops the last `depth` values on the [`ValueStack`]. #[inline] pub fn drop(&mut self, depth: usize) { - self.stack_ptr -= depth; + match self.stack_ptr.checked_sub(depth) { + Some(stack_ptr) => self.stack_ptr = stack_ptr, + None => { + self.stack_ptr = 0; + self.out_of_bounds = true; + } + } } /// Pushes the [`UntypedValue`] to the end of the [`ValueStack`]. @@ -218,7 +227,11 @@ impl ValueStack { /// before function call prevents this procedure from panicking. #[inline] pub fn push(&mut self, entry: UntypedValue) { - *self.get_release_unchecked_mut(self.stack_ptr) = entry; + let Some(cell) = self.entries.get_mut(self.stack_ptr) else { + self.out_of_bounds = true; + return; + }; + *cell = entry; self.stack_ptr += 1; #[cfg(test)] if self.stack_ptr > self.max_stack_height { @@ -228,9 +241,20 @@ impl ValueStack { #[inline] pub fn pop(&mut self) -> UntypedValue { - debug_assert!(self.stack_ptr > 0); - self.stack_ptr -= 1; - *self.get_release_unchecked_mut(self.stack_ptr) + let entry = self + .stack_ptr + .checked_sub(1) + .and_then(|index| self.entries.get(index).copied()); + match entry { + Some(entry) => { + self.stack_ptr -= 1; + entry + } + None => { + self.out_of_bounds = true; + UntypedValue::default() + } + } } /// Returns the capacity of the [`ValueStack`]. @@ -307,7 +331,10 @@ impl ValueStack { /// Returns an exclusive slice to the last `depth` entries in the value stack. #[inline] pub fn peek_as_slice_mut(&mut self, depth: usize) -> &mut [UntypedValue] { - let start = self.stack_ptr - depth; + let Some(start) = self.stack_ptr.checked_sub(depth) else { + self.out_of_bounds = true; + return &mut []; + }; let end = self.stack_ptr; &mut self.entries[start..end] } @@ -324,6 +351,7 @@ impl ValueStack { pub fn reset(&mut self) { self.stack_ptr = 0; self.max_stack_height = 0; + self.out_of_bounds = false; } } @@ -331,53 +359,108 @@ impl ValueStack { /// /// Allows for efficient mutable access to the values of the [`ValueStack`]. /// +/// # Note +/// +/// Every operation is bounds-checked against the `[src, end)` window of the underlying +/// [`ValueStack`] in **all** build profiles. Bytecode reaching outside that window neither reads +/// nor writes memory: the pointer is parked on the stack base and [`ValueStackPtr:: +/// is_out_of_bounds`] starts reporting `true`, which the interpreter turns into a +/// [`TrapCode::StackOverflow`] before the next instruction runs. Relying on the translator to only +/// emit valid stack offsets is not enough here, because [`RwasmModule::new_verified`] exists to +/// accept bytecode this crate did not produce. +/// /// [`ValueStack`]: super::ValueStack +/// [`RwasmModule::new_verified`]: crate::RwasmModule::new_verified #[derive(Debug, Copy, Clone)] pub struct ValueStackPtr { src: *mut UntypedValue, ptr: *mut UntypedValue, + end: *mut UntypedValue, + out_of_bounds: bool, } unsafe impl Send for ValueStackPtr {} -impl From<*mut UntypedValue> for ValueStackPtr { +impl ValueStackPtr { + /// Creates a [`ValueStackPtr`] addressing the `capacity` cells starting at `ptr`. + pub fn new(ptr: *mut UntypedValue, capacity: usize) -> ValueStackPtr { + Self { + src: ptr, + ptr, + end: ptr.wrapping_add(capacity), + out_of_bounds: false, + } + } + + /// Returns `true` if some operation tried to address a cell outside the value stack. #[inline] - fn from(ptr: *mut UntypedValue) -> Self { - Self { src: ptr, ptr } + pub fn is_out_of_bounds(self) -> bool { + self.out_of_bounds } -} -impl ValueStackPtr { - pub fn new(ptr: *mut UntypedValue) -> ValueStackPtr { - Self { ptr, src: ptr } + /// Records an out-of-bounds access and parks the pointer on the stack base. + /// + /// # Note + /// + /// Parking keeps every follow-up operation harmless until the interpreter observes the flag + /// and traps. + #[cold] + #[inline] + pub(crate) fn mark_out_of_bounds(&mut self) { + self.out_of_bounds = true; + self.ptr = self.src; + } + + /// Returns the number of cells between the stack base and the current pointer. + #[inline] + fn len(self) -> usize { + (self.ptr as usize - self.src as usize) / size_of::() + } + + /// Returns the number of cells between the current pointer and the end of the stack. + #[inline] + fn spare(self) -> usize { + (self.end as usize - self.ptr as usize) / size_of::() + } + + /// Returns the cell `depth` entries below the current pointer if it is addressable. + #[inline] + fn cell_back(&mut self, depth: usize) -> Option<*mut UntypedValue> { + if depth == 0 || depth > self.len() { + self.mark_out_of_bounds(); + return None; + } + Some(self.ptr.wrapping_sub(depth)) } /// Calculates the distance between two [`ValueStackPtr] in units of [`UntypedValue`]. #[inline] pub fn offset_from(self, other: Self) -> isize { - // SAFETY: Within Wasm bytecode execution we are guaranteed by - // Wasm validation and `rwasm` codegen to never run out - // of valid bounds using this method. - unsafe { self.ptr.offset_from(other.ptr) } + let distance = self.ptr as isize - other.ptr as isize; + distance / size_of::() as isize } /// Returns the [`UntypedValue`] at the current stack pointer. #[must_use] #[inline] - fn get(self) -> UntypedValue { - // SAFETY: Within Wasm bytecode execution we are guaranteed by - // Wasm validation and `rwasm` codegen to never run out - // of valid bounds using this method. + fn get(&mut self) -> UntypedValue { + if self.ptr >= self.end { + self.mark_out_of_bounds(); + return UntypedValue::default(); + } + // SAFETY: the check above proves that `ptr` addresses a live cell of the value stack. unsafe { *self.ptr } } /// Writes `value` to the cell pointed at by [`ValueStackPtr`]. #[inline] - fn set(self, value: UntypedValue) { - // SAFETY: Within Wasm bytecode execution we are guaranteed by - // Wasm validation and `rwasm` codegen to never run out - // of valid bounds using this method. - *unsafe { &mut *self.ptr } = value; + fn set(&mut self, value: UntypedValue) { + if self.ptr >= self.end { + self.mark_out_of_bounds(); + return; + } + // SAFETY: the check above proves that `ptr` addresses a live cell of the value stack. + unsafe { *self.ptr = value }; } /// Returns a [`ValueStackPtr`] with a pointer value increased by `delta`. @@ -413,7 +496,7 @@ impl ValueStackPtr { /// [`ValueStack`]: super::ValueStack #[inline] #[must_use] - pub fn last(self) -> UntypedValue { + pub fn last(&mut self) -> UntypedValue { self.nth_back(1) } @@ -423,11 +506,16 @@ impl ValueStackPtr { /// /// Given a `depth` of 1 has the same effect as [`ValueStackPtr::last`]. /// - /// A `depth` of 0 is invalid and undefined. + /// A `depth` of 0, or a depth reaching below the stack base, marks the pointer as out of + /// bounds and yields a default value instead of reading memory. #[inline] #[must_use] - pub fn nth_back(self, depth: usize) -> UntypedValue { - self.into_sub(depth).get() + pub fn nth_back(&mut self, depth: usize) -> UntypedValue { + match self.cell_back(depth) { + // SAFETY: `cell_back` only yields pointers to live cells of the value stack. + Some(cell) => unsafe { *cell }, + None => UntypedValue::default(), + } } /// Writes `value` to the n-th [`UntypedValue`] from the back. @@ -436,32 +524,34 @@ impl ValueStackPtr { /// /// Given a `depth` of 1 has the same effect as mutating [`ValueStackPtr::last`]. /// - /// A `depth` of 0 is invalid and undefined. + /// A `depth` of 0, or a depth reaching below the stack base, marks the pointer as out of + /// bounds and discards the write. #[inline] - pub fn set_nth_back(self, depth: usize, value: UntypedValue) { - self.into_sub(depth).set(value) + pub fn set_nth_back(&mut self, depth: usize, value: UntypedValue) { + // SAFETY: `cell_back` only yields pointers to live cells of the value stack. + if let Some(cell) = self.cell_back(depth) { + unsafe { *cell = value } + } } - /// Bumps the [`ValueStackPtr`] of `self` by one. + /// Bumps the [`ValueStackPtr`] of `self` by `delta`. #[inline] fn inc_by(&mut self, delta: usize) { - // SAFETY: Within Wasm bytecode execution we are guaranteed by - // Wasm validation and `rwasm` codegen to never run out - // of valid bounds using this method. - self.ptr = unsafe { self.ptr.add(delta) }; - debug_assert!(self.ptr >= self.src, "stack underflow: {}", delta); + if delta > self.spare() { + self.mark_out_of_bounds(); + return; + } + self.ptr = self.ptr.wrapping_add(delta); } - /// Decreases the [`ValueStackPtr`] of `self` by one. + /// Decreases the [`ValueStackPtr`] of `self` by `delta`. #[inline] fn dec_by(&mut self, delta: usize) { - // SAFETY: Within Wasm bytecode execution we are guaranteed by - // Wasm validation and `rwasm` codegen to never run out - // of valid bounds using this method. - self.ptr = unsafe { self.ptr.sub(delta) }; - if self.ptr < self.src { - debug_assert!(self.ptr >= self.src, "stack underflow"); + if delta > self.len() { + self.mark_out_of_bounds(); + return; } + self.ptr = self.ptr.wrapping_sub(delta); } /// convert stack pointer to the address number @@ -593,8 +683,8 @@ impl ValueStackPtr { where F: FnOnce(UntypedValue) -> UntypedValue, { - let last = self.into_sub(1); - last.set(f(last.get())) + let last = self.nth_back(1); + self.set_nth_back(1, f(last)) } /// Evaluates the given closure `f` for the 2 top most stack values. @@ -604,9 +694,8 @@ impl ValueStackPtr { F: FnOnce(UntypedValue, UntypedValue) -> UntypedValue, { let rhs = self.pop(); - let last = self.into_sub(1); - let lhs = last.get(); - last.set(f(lhs, rhs)); + let lhs = self.nth_back(1); + self.set_nth_back(1, f(lhs, rhs)); } /// Evaluates the given closure `f` for the 3 top most stack values. @@ -616,9 +705,8 @@ impl ValueStackPtr { F: FnOnce(UntypedValue, UntypedValue, UntypedValue) -> UntypedValue, { let (e2, e3) = self.pop2(); - let last = self.into_sub(1); - let e1 = last.get(); - last.set(f(e1, e2, e3)); + let e1 = self.nth_back(1); + self.set_nth_back(1, f(e1, e2, e3)); } /// Evaluates the given fallible closure `f` for the top most stack value. @@ -631,8 +719,8 @@ impl ValueStackPtr { where F: FnOnce(UntypedValue) -> Result, { - let last = self.into_sub(1); - last.set(f(last.get())?); + let last = self.nth_back(1); + self.set_nth_back(1, f(last)?); Ok(()) } @@ -647,9 +735,8 @@ impl ValueStackPtr { F: FnOnce(UntypedValue, UntypedValue) -> Result, { let rhs = self.pop(); - let last = self.into_sub(1); - let lhs = last.get(); - last.set(f(lhs, rhs)?); + let lhs = self.nth_back(1); + self.set_nth_back(1, f(lhs, rhs)?); Ok(()) } diff --git a/tests/value-stack-bounds.rs b/tests/value-stack-bounds.rs new file mode 100644 index 00000000..3168846b --- /dev/null +++ b/tests/value-stack-bounds.rs @@ -0,0 +1,156 @@ +//! Regression tests for out-of-bounds value stack accesses driven by hand-crafted bytecode. +//! +//! Every test runs its bytecode through the interpreter to prove that the value stack refuses the +//! access, which is the guarantee that holds however the module was decoded. Where the module is +//! statically invalid as well, the test also asserts that `RwasmModule::new_verified` rejects it. +//! +//! The two are not the same set. A callee legitimately reads its parameters from below its own +//! entry stack pointer, so verification cannot reject a shallow underflow without rejecting valid +//! code; those cases are caught by the runtime bounds checks alone. + +use rwasm::{ + instruction_set, ExecutionEngine, ImportLinker, InstructionSet, RwasmModule, + RwasmModuleBuilder, RwasmStore, TrapCode, +}; +use std::sync::Arc; + +fn execute(code_section: InstructionSet) -> Result<(), TrapCode> { + let module = RwasmModuleBuilder::new(code_section).build(); + let mut store = RwasmStore::new( + Arc::new(ImportLinker::default()), + (), + rwasm::always_failing_syscall_handler, + Some(1_000_000), + None, + ); + ExecutionEngine::new().execute(&mut store, &module, &[], &mut []) +} + +fn verify(code_section: InstructionSet) -> Result<(), rwasm::RwasmModuleError> { + let sink = RwasmModuleBuilder::new(code_section).build().serialize(); + RwasmModule::new_verified(&sink).map(|_| ()) +} + +#[test] +fn local_get_beyond_the_stack_traps_instead_of_reading_out_of_bounds() { + let code = instruction_set! { + StackCheck(16) + LocalGet(0x0800_0000) + Drop + Return + }; + assert!(verify(code.clone()).is_err()); + assert_eq!(execute(code), Err(TrapCode::StackOverflow)); +} + +#[test] +fn local_set_beyond_the_stack_traps_instead_of_writing_out_of_bounds() { + let code = instruction_set! { + StackCheck(16) + I32Const(0xdead_beefu32 as i32) + LocalSet(0x0800_0000) + Return + }; + assert!(verify(code.clone()).is_err()); + assert_eq!(execute(code), Err(TrapCode::StackOverflow)); +} + +#[test] +fn local_tee_beyond_the_stack_traps_instead_of_writing_out_of_bounds() { + let code = instruction_set! { + StackCheck(16) + I32Const(0xdead_beefu32 as i32) + LocalTee(0x0800_0000) + Drop + Return + }; + assert!(verify(code.clone()).is_err()); + assert_eq!(execute(code), Err(TrapCode::StackOverflow)); +} + +#[test] +fn dropping_below_the_stack_base_traps() { + let code = instruction_set! { + StackCheck(16) + Drop + Return + }; + assert_eq!(execute(code), Err(TrapCode::StackOverflow)); +} + +#[test] +fn bulk_drop_below_the_stack_base_traps() { + let code = instruction_set! { + StackCheck(16) + BulkDrop(0x0800_0000) + Return + }; + assert!(verify(code.clone()).is_err()); + assert_eq!(execute(code), Err(TrapCode::StackOverflow)); +} + +#[test] +fn popping_below_the_stack_base_traps() { + let code = instruction_set! { + StackCheck(16) + I32Const(1) + I32Add + Return + }; + assert_eq!(execute(code), Err(TrapCode::StackOverflow)); +} + +#[test] +fn pushing_past_the_reserved_stack_window_traps() { + // `StackCheck` reserves a single cell, so the pushes run out of stack long before the last + // one. Verification is coarser and only rejects the push that leaves the addressable window, + // which is floored at `N_MAX_STACK_SIZE` no matter how little the module reserves. + let mut code = instruction_set! { StackCheck(1) }; + for _ in 0..(rwasm::N_MAX_STACK_SIZE + 1) { + code.op_i32_const(1); + } + code.op_return(); + assert!(verify(code.clone()).is_err()); + assert_eq!(execute(code), Err(TrapCode::StackOverflow)); +} + +/// Guards against the verifier rejecting bytecode this crate produces itself. +#[test] +fn compiled_modules_pass_verification() { + use rwasm::{CompilationConfig, ImportName}; + use rwasm_fuel_policy::SyscallFuelParams; + use wasmparser::ValType; + + const I32X3: &[ValType] = &[ValType::I32; 3]; + let mut import_linker = ImportLinker::default(); + for (name, idx, params, results) in [ + ("_debug_log", 70u32, 2usize, 0usize), + ("_input_size", 71, 0, 1), + ("_output_size", 72, 0, 1), + ("_read", 73, 3, 0), + ("_write", 74, 2, 0), + ("_exit", 75, 1, 0), + ("_read_output", 76, 3, 0), + ] { + import_linker.insert_function( + ImportName::new("fluentbase_v1preview", name), + idx, + SyscallFuelParams::default(), + &I32X3[..params], + &I32X3[..results], + ); + } + let import_linker = Arc::new(import_linker); + for wasm_binary in [ + include_bytes!("assets/nitro-verifier-stack-ub.wasm").as_slice(), + include_bytes!("assets/panic-stack-ub.wasm").as_slice(), + include_bytes!("assets/secp256k1-stack-ub.wasm").as_slice(), + ] { + let config = CompilationConfig::default() + .with_entrypoint_name("main".into()) + .with_allow_malformed_entrypoint_func_type(true) + .with_import_linker(import_linker.clone()); + let (module, _) = RwasmModule::compile(config, wasm_binary).expect("rwasm compiles"); + module.verify().expect("compiled module verifies"); + } +}