diff --git a/.changelog/unreleased/improvements/3315-wasmer-read-only-mem.md b/.changelog/unreleased/improvements/3315-wasmer-read-only-mem.md new file mode 100644 index 0000000000..317c36dcc7 --- /dev/null +++ b/.changelog/unreleased/improvements/3315-wasmer-read-only-mem.md @@ -0,0 +1,2 @@ +- Avoid growing wasm memory when performing read-only accesses. + ([\#3315](https://github.com/anoma/namada/pull/3315)) \ No newline at end of file diff --git a/crates/namada/src/vm/wasm/memory.rs b/crates/namada/src/vm/wasm/memory.rs index 894e4efc44..95272ccdba 100644 --- a/crates/namada/src/vm/wasm/memory.rs +++ b/crates/namada/src/vm/wasm/memory.rs @@ -26,6 +26,8 @@ use crate::vm::types::VpInput; #[allow(missing_docs)] #[derive(Error, Debug)] pub enum Error { + #[error("Attempted to modify read-only memory")] + ReadOnly, #[error("Offset {0}+{1} overflows 32 bits storage")] OverflowingOffset(u64, usize), #[error("Failed initializing the memory: {0}")] @@ -186,14 +188,17 @@ pub fn write_vp_inputs( /// Check that the given offset and length fits into the memory bounds. If not, /// it will try to grow the memory. -// TODO(namada#3314): avoid growing memory if we're only performing reads; -// return an Err instead -fn check_bounds( - store: &mut impl wasmer::AsStoreMut, +fn check_bounds( + store: &mut S, memory: &Memory, base_addr: u64, offset: usize, -) -> Result<()> { + grow_callback: F, +) -> Result<()> +where + S: wasmer::AsStoreMut, + F: Fn(u64, &Memory, &mut S) -> Result<()>, +{ let store_mut = store.as_store_mut(); let memview = memory.view(&store_mut); @@ -216,26 +221,7 @@ fn check_bounds( }) .ok_or(Error::OverflowingOffset(base_addr, offset))?; if memview.data_size() < desired_offset { - let cur_pages = memview.size().0 as usize; - let capacity = checked!(cur_pages * WASM_PAGE_SIZE)?; - // usizes should be at least 32 bits wide on most architectures, - // so this cast shouldn't cause panics, given the invariant that - // `desired_offset` is at most a 32 bit wide value. moreover, - // `capacity` should not be larger than `memory.data_size()`, - // so this subtraction should never fail - let desired_offset = usize::try_from(desired_offset)?; - let missing = checked!(desired_offset - capacity)?; - // extrapolate the number of pages missing to allow addressing - // the desired memory offset - let req_pages = - checked!((missing + WASM_PAGE_SIZE - 1) / WASM_PAGE_SIZE)?; - let req_pages: u32 = u32::try_from(req_pages)?; - tracing::debug!(req_pages, "Attempting to grow wasm memory"); - memory.grow(store, req_pages).map_err(Error::Grow)?; - tracing::debug!( - mem_size = memory.view(&store.as_store_mut()).data_size(), - "Wasm memory size has been successfully extended" - ); + grow_callback(desired_offset, memory, store)?; } Ok(()) } @@ -247,7 +233,13 @@ fn read_memory_bytes( offset: u64, len: usize, ) -> Result> { - check_bounds(store, memory, offset, len)?; + check_bounds( + store, + memory, + offset, + len, + |_desired_offset, _memory, _store| Err(Error::ReadOnly), + )?; let mut buf = vec![0; len]; memory.view(&store.as_store_mut()).read(offset, &mut buf)?; Ok(buf) @@ -261,7 +253,43 @@ fn write_memory_bytes( bytes: impl AsRef<[u8]>, ) -> Result<()> { let buf = bytes.as_ref(); - check_bounds(store, memory, offset, buf.len() as _)?; + check_bounds( + store, + memory, + offset, + buf.len() as _, + |desired_offset, memory, store| { + let store_mut = store.as_store_mut(); + let memview = memory.view(&store_mut); + + let cur_pages = memview.size().0 as usize; + let capacity = checked!(cur_pages * WASM_PAGE_SIZE)?; + + // usizes should be at least 32 bits wide on most architectures, + // so this cast shouldn't cause panics, given the invariant that + // `desired_offset` is at most a 32 bit wide value. moreover, + // `capacity` should not be larger than `memory.data_size()`, + // so this subtraction should never fail + let desired_offset = usize::try_from(desired_offset)?; + let missing = checked!(desired_offset - capacity)?; + + // extrapolate the number of pages missing to allow addressing + // the desired memory offset + let req_pages = + checked!((missing + WASM_PAGE_SIZE - 1) / WASM_PAGE_SIZE)?; + let req_pages: u32 = u32::try_from(req_pages)?; + + tracing::debug!(req_pages, "Attempting to grow wasm memory"); + + memory.grow(store, req_pages).map_err(Error::Grow)?; + tracing::debug!( + mem_size = memory.view(&store.as_store_mut()).data_size(), + "Wasm memory size has been successfully extended" + ); + + Ok(()) + }, + )?; memory.view(&store.as_store_mut()).write(offset, buf)?; Ok(()) } diff --git a/crates/namada/src/vm/wasm/run.rs b/crates/namada/src/vm/wasm/run.rs index 53adc441d3..eb54aa62de 100644 --- a/crates/namada/src/vm/wasm/run.rs +++ b/crates/namada/src/vm/wasm/run.rs @@ -940,14 +940,17 @@ mod tests { use borsh_ext::BorshSerializeExt; use itertools::Either; + use namada_sdk::arith::checked; use namada_state::StorageWrite; use namada_test_utils::TestWasms; use namada_token::DenominatedAmount; use namada_tx::data::{Fee, TxType}; use namada_tx::{Code, Data}; use test_log::test; + use wasmer::WASM_PAGE_SIZE; use wasmer_vm::TrapCode; + use super::memory::{TX_MEMORY_INIT_PAGES, VP_MEMORY_INIT_PAGES}; use super::*; use crate::state::testing::TestState; use crate::tx::data::eval_vp::EvalVp; @@ -985,15 +988,15 @@ mod tests { let error = execute_tx_with_code(tx_code).expect_err(PANIC_MSG); assert!( matches!( - assert_rt_mem_error(&error, PANIC_MSG), + assert_tx_rt_mem_error(&error, PANIC_MSG), memory::Error::OverflowingOffset(18446744073709551615, 1), ), "{PANIC_MSG}" ); } - /// Extract a wasm runtime memory error from some [`Error`]. - fn assert_rt_mem_error<'err>( + /// Extract a tx wasm runtime memory error from some [`Error`]. + fn assert_tx_rt_mem_error<'err>( error: &'err Error, assert_msg: &str, ) -> &'err memory::Error { @@ -1014,6 +1017,29 @@ mod tests { .unwrap_or_else(|| panic!("{assert_msg}: {tx_mem_err}")) } + /// Extract a vp wasm runtime memory error from some [`Error`]. + fn assert_vp_rt_mem_error<'err>( + error: &'err Error, + assert_msg: &str, + ) -> &'err memory::Error { + let Error::RuntimeError(rt_error) = error else { + panic!("{assert_msg}: {error}"); + }; + let source_err = + rt_error.source().expect("No runtime error source found"); + let downcasted_tx_rt_err: &vp_host_fns::RuntimeError = source_err + .downcast_ref() + .unwrap_or_else(|| panic!("{assert_msg}: {source_err}")); + let vp_host_fns::RuntimeError::MemoryError(vp_mem_err) = + downcasted_tx_rt_err + else { + panic!("{assert_msg}: {downcasted_tx_rt_err}"); + }; + vp_mem_err + .downcast_ref() + .unwrap_or_else(|| panic!("{assert_msg}: {vp_mem_err}")) + } + /// Test that when a transaction wasm goes over the stack-height limit, the /// execution is aborted. #[test] @@ -1851,6 +1877,116 @@ mod tests { assert!(matches!(result.unwrap_err(), Error::GasError(_))); } + #[test] + fn test_tx_ro_memory_wont_grow() { + // a transaction that accesses memory out of bounds + let out_of_bounds_index = + checked!(2usize * TX_MEMORY_INIT_PAGES as usize * WASM_PAGE_SIZE) + .unwrap(); + let tx_code = wasmer::wat2wasm(format!( + r#" + (module + (import "env" "namada_tx_read" (func (param i64 i64) (result i64))) + (func (param i64 i64) (result i64) + i64.const {out_of_bounds_index} + i64.const 1 + (call 0) + ) + (memory 16) + (export "memory" (memory 0)) + (export "_apply_tx" (func 1)) + ) + "# + ).as_bytes()) + .expect("unexpected error converting wat2wasm") + .into_owned(); + + const PANIC_MSG: &str = + "Test should have failed with a wasm runtime memory error"; + + let error = execute_tx_with_code(tx_code).expect_err(PANIC_MSG); + assert!( + matches!( + assert_tx_rt_mem_error(&error, PANIC_MSG), + memory::Error::ReadOnly, + ), + "{PANIC_MSG}" + ); + } + + #[test] + fn test_vp_ro_memory_wont_grow() { + // vp code that accesses memory out of bounds + let out_of_bounds_index = + checked!(2usize * VP_MEMORY_INIT_PAGES as usize * WASM_PAGE_SIZE) + .unwrap(); + let vp_code = wasmer::wat2wasm(format!( + r#" + (module + (type (;0;) (func (param i64 i64 i64 i64 i64 i64 i64 i64) (result i64))) + (import "env" "namada_vp_read_pre" (func (param i64 i64) (result i64))) + + (func $_validate_tx (type 0) (param i64 i64 i64 i64 i64 i64 i64 i64) (result i64) + i64.const {out_of_bounds_index} + i64.const 1 + (call 0) + ) + + (table (;0;) 1 1 funcref) + (memory (;0;) 16) + (global (;0;) (mut i32) (i32.const 1048576)) + (export "memory" (memory 0)) + (export "_validate_tx" (func $_validate_tx))) + "#).as_bytes(), + ) + .expect("unexpected error converting wat2wasm").into_owned(); + + const PANIC_MSG: &str = + "Test should have failed with a wasm runtime memory error"; + + let error = execute_vp_with_code(vp_code).expect_err(PANIC_MSG); + assert!( + matches!( + assert_vp_rt_mem_error(&error, PANIC_MSG), + memory::Error::ReadOnly, + ), + "{PANIC_MSG}" + ); + } + + fn execute_vp_with_code(vp_code: Vec) -> Result<()> { + let mut outer_tx = Tx::from_type(TxType::Raw); + outer_tx.push_default_inner_tx(); + let tx_index = TxIndex::default(); + let mut state = TestState::default(); + let addr = state.in_mem_mut().address_gen.generate_address("rng seed"); + let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( + &TxGasMeter::new_from_sub_limit(TX_GAS_LIMIT.into()), + )); + let keys_changed = BTreeSet::new(); + let verifiers = BTreeSet::new(); + let (vp_cache, _) = wasm::compilation_cache::common::testing::cache(); + // store the vp code + let code_hash = Hash::sha256(&vp_code); + let code_len = vp_code.len() as u64; + let key = Key::wasm_code(&code_hash); + let len_key = Key::wasm_code_len(&code_hash); + state.write(&key, vp_code).unwrap(); + state.write(&len_key, code_len).unwrap(); + + vp( + code_hash, + &outer_tx.batch_ref_first_tx(), + &tx_index, + &addr, + &state, + &gas_meter, + &keys_changed, + &verifiers, + vp_cache, + ) + } + fn execute_tx_with_code(tx_code: Vec) -> Result> { let tx_data = vec![]; let tx_index = TxIndex::default(); @@ -1954,36 +2090,7 @@ mod tests { ) .expect("unexpected error converting wat2wasm").into_owned(); - let mut outer_tx = Tx::from_type(TxType::Raw); - outer_tx.push_default_inner_tx(); - let tx_index = TxIndex::default(); - let mut state = TestState::default(); - let addr = state.in_mem_mut().address_gen.generate_address("rng seed"); - let gas_meter = RefCell::new(VpGasMeter::new_from_tx_meter( - &TxGasMeter::new_from_sub_limit(TX_GAS_LIMIT.into()), - )); - let keys_changed = BTreeSet::new(); - let verifiers = BTreeSet::new(); - let (vp_cache, _) = wasm::compilation_cache::common::testing::cache(); - // store the vp code - let code_hash = Hash::sha256(&vp_code); - let code_len = vp_code.len() as u64; - let key = Key::wasm_code(&code_hash); - let len_key = Key::wasm_code_len(&code_hash); - state.write(&key, vp_code).unwrap(); - state.write(&len_key, code_len).unwrap(); - - vp( - code_hash, - &outer_tx.batch_ref_first_tx(), - &tx_index, - &addr, - &state, - &gas_meter, - &keys_changed, - &verifiers, - vp_cache, - ) + execute_vp_with_code(vp_code) } fn get_trap_code(error: &Error) -> Either {