From 0f417199186042028269830401687efe23e0ae18 Mon Sep 17 00:00:00 2001 From: Alessandro Decina Date: Wed, 30 Aug 2023 16:57:24 +0700 Subject: [PATCH] direct mapping: misc fixes (#32649) * transaction_context: update make_data_mut comment * bpf_loader: cpi: pass SerializeAccountMetadata to CallerAccount::from* We now have a way to provide CallerAccount with trusted values coming from our internal serialization code and not from untrusted vm space * bpf_loader: direct_mapping: enforce account info pointers to be immutable When direct mapping is enabled, we might need to update account data memory regions across CPI calls. Since the only way we have to retrieve the regions is based on their vm addresses, we enforce vm addresses to be stable. Accounts can still be mutated and resized of course, but it must be done in place. This also locks all other AccountInfo pointers, since there's no legitimate reason to make them point to anything else. * bpf_loader: cpi: access ref_to_len_in_vm through VmValue Direct mapping needs to translate vm values at each access since permissions of the underlying memory might have changed. * direct mapping: improve memory permission tracking across CPI calls Ensure that the data and realloc regions of an account always track the account's permissions. In order to do this, we also need to split realloc regions in their own self contained regions, where before we had: [account fields][account data][account realloc + more account fields + next account fields][next account data][...] we now have: [account fields][account data][account realloc][more account fields + next account fields][next account data][...] Tested in TEST_[FORBID|ALLOW]_WRITE_AFTER_OWNERSHIP_CHANGE* Additionally when direct mapping is on, we must update all perms at once before doing account data updates. Otherwise, updating an account might write into another account whose perms we haven't updated yet. Tested in TEST_FORBID_LEN_UPDATE_AFTER_OWNERSHIP_CHANGE. * bpf_loader: serialization: address review comment don't return vm_addr from push_account_region * bpf_loader: rename push_account_region to push_account_data_region * cpi: fix slow edge case zeroing extra account capacity after shrinking an account When returning from CPI we need to zero all the account memory up to the original length only if we know we're potentially dealing with uninitialized memory. When we know that the spare capacity has deterministic content, we only need to zero new_len..prev_len. This fixes a slow edge case that was triggerable by the following scenario: - load a large account (say 10MB) into the vm - shrink to 10 bytes - would memset 10..10MB - shrink to 9 bytes - would memset 9..10MB - shrink to 8 bytes - would memset 8..10MB - ... Now instead in the scenario above the following will happen: - load a large account (say 10MB) into the vm - shrink to 10 bytes - memsets 10..10MB - shrink to 9 bytes - memsets 9..10 - shrink to 8 bytes - memset 8..9 - ... * bpf_loader: add account_data_region_memory_state helper Shared between serialization and CPI to figure out the MemoryState of an account. * cpi: direct_mapping: error out if ref_to_len_in_vm points to account memory If ref_to_len_in_vm is allowed to be in account memory, calles could mutate it, essentially letting callees directly mutate callers memory. * bpf_loader: direct_mapping: map AccessViolation -> InstructionError Return the proper ReadonlyDataModified / ExecutableDataModified / ExternalAccountDataModified depending on where the violation occurs * bpf_loader: cpi: remove unnecessary infallible slice::get call --- Cargo.lock | 1 + program-runtime/src/invoke_context.rs | 4 + programs/bpf_loader/Cargo.toml | 1 + programs/bpf_loader/src/lib.rs | 82 ++- programs/bpf_loader/src/serialization.rs | 142 ++-- programs/bpf_loader/src/syscalls/cpi.rs | 826 ++++++++++++++++++----- programs/bpf_loader/src/syscalls/mod.rs | 2 + programs/sbf/Cargo.lock | 1 + sdk/src/transaction_context.rs | 4 +- 9 files changed, 797 insertions(+), 266 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b171214e65866a..d0bf159c257ecb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5372,6 +5372,7 @@ dependencies = [ "log", "memoffset 0.9.0", "rand 0.8.5", + "scopeguard", "solana-measure", "solana-program-runtime", "solana-sdk", diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index 3506671dadc4d4..a105048ac3388c 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -153,6 +153,10 @@ pub struct SyscallContext { #[derive(Debug, Clone)] pub struct SerializedAccountMetadata { pub original_data_len: usize, + pub vm_data_addr: u64, + pub vm_key_addr: u64, + pub vm_lamports_addr: u64, + pub vm_owner_addr: u64, } pub struct InvokeContext<'a> { diff --git a/programs/bpf_loader/Cargo.toml b/programs/bpf_loader/Cargo.toml index e7123ae0cb4d00..411d9f3654b974 100644 --- a/programs/bpf_loader/Cargo.toml +++ b/programs/bpf_loader/Cargo.toml @@ -15,6 +15,7 @@ byteorder = { workspace = true } libsecp256k1 = { workspace = true } log = { workspace = true } rand = { workspace = true } +scopeguard = { workspace = true } solana-measure = { workspace = true } solana-program-runtime = { workspace = true } solana-sdk = { workspace = true } diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 99d3f583c95881..57b77559f2373d 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -1495,12 +1495,19 @@ fn execute<'a, 'b: 'a>( let log_collector = invoke_context.get_log_collector(); let transaction_context = &invoke_context.transaction_context; let instruction_context = transaction_context.get_current_instruction_context()?; - let program_id = *instruction_context.get_last_program_key(transaction_context)?; + let (program_id, is_loader_deprecated) = { + let program_account = + instruction_context.try_borrow_last_program_account(transaction_context)?; + ( + *program_account.get_key(), + *program_account.get_owner() == bpf_loader_deprecated::id(), + ) + }; #[cfg(any(target_os = "windows", not(target_arch = "x86_64")))] let use_jit = false; #[cfg(all(not(target_os = "windows"), target_arch = "x86_64"))] let use_jit = executable.get_compiled_program().is_some(); - let bpf_account_data_direct_mapping = invoke_context + let direct_mapping = invoke_context .feature_set .is_active(&bpf_account_data_direct_mapping::id()); @@ -1511,18 +1518,26 @@ fn execute<'a, 'b: 'a>( invoke_context .feature_set .is_active(&cap_bpf_program_instruction_accounts::ID), - !bpf_account_data_direct_mapping, + !direct_mapping, )?; serialize_time.stop(); - // save the account addresses so in case of AccessViolation below we can - // map to InstructionError::ReadonlyDataModified, which is easier to - // diagnose from developers - let account_region_addrs = regions + // save the account addresses so in case we hit an AccessViolation error we + // can map to a more specific error + let account_region_addrs = accounts_metadata .iter() - .map(|r| r.vm_addr..r.vm_addr.saturating_add(r.len)) + .map(|m| { + let vm_end = m + .vm_data_addr + .saturating_add(m.original_data_len as u64) + .saturating_add(if !is_loader_deprecated { + MAX_PERMITTED_DATA_INCREASE as u64 + } else { + 0 + }); + m.vm_data_addr..vm_end + }) .collect::>(); - let addr_is_account_data = |addr: u64| account_region_addrs.iter().any(|r| r.contains(&addr)); let mut create_vm_time = Measure::start("create_vm"); let mut execute_time; @@ -1574,22 +1589,43 @@ fn execute<'a, 'b: 'a>( }; Err(Box::new(error) as Box) } - ProgramResult::Err(error) => { - let error = match error.downcast_ref() { - Some(EbpfError::AccessViolation( + ProgramResult::Err(mut error) => { + if direct_mapping { + if let Some(EbpfError::AccessViolation( _pc, AccessType::Store, address, _size, _section_name, - )) if addr_is_account_data(*address) => { - // We can get here if direct_mapping is enabled and a program tries to - // write to a readonly account. Map the error to ReadonlyDataModified so - // it's easier for devs to diagnose what happened. - Box::new(InstructionError::ReadonlyDataModified) + )) = error.downcast_ref() + { + // If direct_mapping is enabled and a program tries to write to a readonly + // region we'll get a memory access violation. Map it to a more specific + // error so it's easier for developers to see what happened. + if let Some((instruction_account_index, _)) = account_region_addrs + .iter() + .enumerate() + .find(|(_, vm_region)| vm_region.contains(address)) + { + let transaction_context = &invoke_context.transaction_context; + let instruction_context = + transaction_context.get_current_instruction_context()?; + + let account = instruction_context.try_borrow_instruction_account( + transaction_context, + instruction_account_index as IndexOfAccount, + )?; + + error = Box::new(if account.is_executable() { + InstructionError::ExecutableDataModified + } else if account.is_writable() { + InstructionError::ExternalAccountDataModified + } else { + InstructionError::ReadonlyDataModified + }) + } } - _ => error, - }; + } Err(error) } _ => Ok(()), @@ -1615,12 +1651,8 @@ fn execute<'a, 'b: 'a>( let mut deserialize_time = Measure::start("deserialize"); let execute_or_deserialize_result = execution_result.and_then(|_| { - deserialize_parameters( - invoke_context, - parameter_bytes.as_slice(), - !bpf_account_data_direct_mapping, - ) - .map_err(|error| Box::new(error) as Box) + deserialize_parameters(invoke_context, parameter_bytes.as_slice(), !direct_mapping) + .map_err(|error| Box::new(error) as Box) }); deserialize_time.stop(); diff --git a/programs/bpf_loader/src/serialization.rs b/programs/bpf_loader/src/serialization.rs index 768ba792116b26..1650b845a9a24a 100644 --- a/programs/bpf_loader/src/serialization.rs +++ b/programs/bpf_loader/src/serialization.rs @@ -6,7 +6,7 @@ use { solana_rbpf::{ aligned_memory::{AlignedMemory, Pod}, ebpf::{HOST_ALIGN, MM_INPUT_START}, - memory_region::MemoryRegion, + memory_region::{MemoryRegion, MemoryState}, }, solana_sdk::{ bpf_loader_deprecated, @@ -55,8 +55,12 @@ impl Serializer { self.buffer.fill_write(num, value) } - pub fn write(&mut self, value: T) { + pub fn write(&mut self, value: T) -> u64 { self.debug_assert_alignment::(); + let vaddr = self + .vaddr + .saturating_add(self.buffer.len() as u64) + .saturating_sub(self.region_start as u64); // Safety: // in serialize_parameters_(aligned|unaligned) first we compute the // required size then we write into the newly allocated buffer. There's @@ -68,22 +72,38 @@ impl Serializer { unsafe { self.buffer.write_unchecked(value); } + + vaddr } - fn write_all(&mut self, value: &[u8]) { + fn write_all(&mut self, value: &[u8]) -> u64 { + let vaddr = self + .vaddr + .saturating_add(self.buffer.len() as u64) + .saturating_sub(self.region_start as u64); // Safety: // see write() - the buffer is guaranteed to be large enough unsafe { self.buffer.write_all_unchecked(value); } + + vaddr } - fn write_account(&mut self, account: &mut BorrowedAccount<'_>) -> Result<(), InstructionError> { - if self.copy_account_data { + fn write_account( + &mut self, + account: &mut BorrowedAccount<'_>, + ) -> Result { + let vm_data_addr = if self.copy_account_data { + let vm_data_addr = self.vaddr.saturating_add(self.buffer.len() as u64); self.write_all(account.get_data()); + vm_data_addr } else { - self.push_account_region(account)?; - } + self.push_region(true); + let vaddr = self.vaddr; + self.push_account_data_region(account)?; + vaddr + }; if self.aligned { let align_offset = @@ -100,59 +120,55 @@ impl Serializer { self.fill_write(MAX_PERMITTED_DATA_INCREASE + BPF_ALIGN_OF_U128, 0) .map_err(|_| InstructionError::InvalidArgument)?; self.region_start += BPF_ALIGN_OF_U128.saturating_sub(align_offset); + // put the realloc padding in its own region + self.push_region(account.can_data_be_changed().is_ok()); } } - Ok(()) + Ok(vm_data_addr) } - fn push_account_region( + fn push_account_data_region( &mut self, account: &mut BorrowedAccount<'_>, ) -> Result<(), InstructionError> { - self.push_region(); - let account_len = account.get_data().len(); - if account_len > 0 { - let region = if account.can_data_be_changed().is_ok() { - if account.is_shared() { - // If the account is still shared it means it wasn't written to yet during this - // transaction. We map it as CoW and it'll be copied the first time something - // tries to write into it. - let index_in_transaction = account.get_index_in_transaction(); - - MemoryRegion::new_cow( - account.get_data(), - self.vaddr, - index_in_transaction as u64, - ) - } else { - // The account isn't shared anymore, meaning it was written to earlier during - // this transaction. We can just map as writable no need for any fancy CoW - // business. + if !account.get_data().is_empty() { + let region = match account_data_region_memory_state(account) { + MemoryState::Readable => MemoryRegion::new_readonly(account.get_data(), self.vaddr), + MemoryState::Writable => { MemoryRegion::new_writable(account.get_data_mut()?, self.vaddr) } - } else { - MemoryRegion::new_readonly(account.get_data(), self.vaddr) + MemoryState::Cow(index_in_transaction) => { + MemoryRegion::new_cow(account.get_data(), self.vaddr, index_in_transaction) + } }; self.vaddr += region.len; self.regions.push(region); } + Ok(()) } - fn push_region(&mut self) { + fn push_region(&mut self, writable: bool) { let range = self.region_start..self.buffer.len(); - let region = MemoryRegion::new_writable( - self.buffer.as_slice_mut().get_mut(range.clone()).unwrap(), - self.vaddr, - ); + let region = if writable { + MemoryRegion::new_writable( + self.buffer.as_slice_mut().get_mut(range.clone()).unwrap(), + self.vaddr, + ) + } else { + MemoryRegion::new_readonly( + self.buffer.as_slice().get(range.clone()).unwrap(), + self.vaddr, + ) + }; self.regions.push(region); self.region_start = range.end; self.vaddr += range.len() as u64; } fn finish(mut self) -> (AlignedMemory, Vec) { - self.push_region(); + self.push_region(true); debug_assert_eq!(self.region_start, self.buffer.len()); (self.buffer, self.regions) } @@ -315,19 +331,23 @@ fn serialize_parameters_unaligned( s.write(position as u8); } SerializeAccount::Account(_, mut account) => { - accounts_metadata.push(SerializedAccountMetadata { - original_data_len: account.get_data().len(), - }); s.write::(NON_DUP_MARKER); s.write::(account.is_signer() as u8); s.write::(account.is_writable() as u8); - s.write_all(account.get_key().as_ref()); - s.write::(account.get_lamports().to_le()); + let vm_key_addr = s.write_all(account.get_key().as_ref()); + let vm_lamports_addr = s.write::(account.get_lamports().to_le()); s.write::((account.get_data().len() as u64).to_le()); - s.write_account(&mut account)?; - s.write_all(account.get_owner().as_ref()); + let vm_data_addr = s.write_account(&mut account)?; + let vm_owner_addr = s.write_all(account.get_owner().as_ref()); s.write::(account.is_executable() as u8); s.write::((account.get_rent_epoch()).to_le()); + accounts_metadata.push(SerializedAccountMetadata { + original_data_len: account.get_data().len(), + vm_key_addr, + vm_lamports_addr, + vm_owner_addr, + vm_data_addr, + }); } }; } @@ -406,7 +426,7 @@ fn serialize_parameters_aligned( ), InstructionError, > { - let mut account_lengths = Vec::with_capacity(accounts.len()); + let mut accounts_metadata = Vec::with_capacity(accounts.len()); // Calculate size in order to alloc once let mut size = size_of::(); for account in &accounts { @@ -444,23 +464,27 @@ fn serialize_parameters_aligned( for account in accounts { match account { SerializeAccount::Account(_, mut borrowed_account) => { - account_lengths.push(SerializedAccountMetadata { - original_data_len: borrowed_account.get_data().len(), - }); s.write::(NON_DUP_MARKER); s.write::(borrowed_account.is_signer() as u8); s.write::(borrowed_account.is_writable() as u8); s.write::(borrowed_account.is_executable() as u8); s.write_all(&[0u8, 0, 0, 0]); - s.write_all(borrowed_account.get_key().as_ref()); - s.write_all(borrowed_account.get_owner().as_ref()); - s.write::(borrowed_account.get_lamports().to_le()); + let vm_key_addr = s.write_all(borrowed_account.get_key().as_ref()); + let vm_owner_addr = s.write_all(borrowed_account.get_owner().as_ref()); + let vm_lamports_addr = s.write::(borrowed_account.get_lamports().to_le()); s.write::((borrowed_account.get_data().len() as u64).to_le()); - s.write_account(&mut borrowed_account)?; + let vm_data_addr = s.write_account(&mut borrowed_account)?; s.write::((borrowed_account.get_rent_epoch()).to_le()); + accounts_metadata.push(SerializedAccountMetadata { + original_data_len: borrowed_account.get_data().len(), + vm_key_addr, + vm_owner_addr, + vm_lamports_addr, + vm_data_addr, + }); } SerializeAccount::Duplicate(position) => { - account_lengths.push(account_lengths.get(position as usize).unwrap().clone()); + accounts_metadata.push(accounts_metadata.get(position as usize).unwrap().clone()); s.write::(position as u8); s.write_all(&[0u8, 0, 0, 0, 0, 0, 0]); } @@ -471,7 +495,7 @@ fn serialize_parameters_aligned( s.write_all(program_id.as_ref()); let (mem, regions) = s.finish(); - Ok((mem, regions, account_lengths)) + Ok((mem, regions, accounts_metadata)) } pub fn deserialize_parameters_aligned>( @@ -579,6 +603,18 @@ pub fn deserialize_parameters_aligned>( Ok(()) } +pub(crate) fn account_data_region_memory_state(account: &BorrowedAccount<'_>) -> MemoryState { + if account.can_data_be_changed().is_ok() { + if account.is_shared() { + MemoryState::Cow(account.get_index_in_transaction() as u64) + } else { + MemoryState::Writable + } + } else { + MemoryState::Readable + } +} + #[cfg(test)] #[allow(clippy::indexing_slicing)] mod tests { diff --git a/programs/bpf_loader/src/syscalls/cpi.rs b/programs/bpf_loader/src/syscalls/cpi.rs index 3a690ec3609a99..e18052acc0c691 100644 --- a/programs/bpf_loader/src/syscalls/cpi.rs +++ b/programs/bpf_loader/src/syscalls/cpi.rs @@ -1,6 +1,12 @@ use { super::*, - crate::declare_syscall, + crate::{declare_syscall, serialization::account_data_region_memory_state}, + scopeguard::defer, + solana_program_runtime::invoke_context::SerializedAccountMetadata, + solana_rbpf::{ + ebpf, + memory_region::{MemoryRegion, MemoryState}, + }, solana_sdk::{ feature_set::enable_bpf_loader_set_authority_checked_ix, stable_layout::stable_instruction::StableInstruction, @@ -9,14 +15,68 @@ use { }, transaction_context::BorrowedAccount, }, - std::{mem, ptr, slice}, + std::{mem, ptr}, }; +fn check_account_info_pointer( + invoke_context: &InvokeContext, + vm_addr: u64, + expected_vm_addr: u64, + field: &str, +) -> Result<(), Error> { + if vm_addr != expected_vm_addr { + ic_msg!( + invoke_context, + "Invalid account info pointer `{}': {:#x} != {:#x}", + field, + vm_addr, + expected_vm_addr + ); + return Err(SyscallError::InvalidPointer.into()); + } + Ok(()) +} + +enum VmValue<'a, 'b, T> { + VmAddress { + vm_addr: u64, + memory_mapping: &'b MemoryMapping<'a>, + check_aligned: bool, + }, + // Once direct mapping is activated, this variant can be removed and the + // enum can be made a struct. + Translated(&'a mut T), +} + +impl<'a, 'b, T> VmValue<'a, 'b, T> { + fn get(&self) -> Result<&T, Error> { + match self { + VmValue::VmAddress { + vm_addr, + memory_mapping, + check_aligned, + } => translate_type(memory_mapping, *vm_addr, *check_aligned), + VmValue::Translated(addr) => Ok(*addr), + } + } + + fn get_mut(&mut self) -> Result<&mut T, Error> { + match self { + VmValue::VmAddress { + vm_addr, + memory_mapping, + check_aligned, + } => translate_type_mut(memory_mapping, *vm_addr, *check_aligned), + VmValue::Translated(addr) => Ok(*addr), + } + } +} + /// Host side representation of AccountInfo or SolAccountInfo passed to the CPI syscall. /// /// At the start of a CPI, this can be different from the data stored in the /// corresponding BorrowedAccount, and needs to be synched. -struct CallerAccount<'a> { +struct CallerAccount<'a, 'b> { lamports: &'a mut u64, owner: &'a mut Pubkey, // The original data length of the account at the start of the current @@ -28,34 +88,49 @@ struct CallerAccount<'a> { // mapped inside the vm (see serialize_parameters() in // BpfExecutor::execute). // - // When direct mapping is off, this includes both the account data _and_ the - // realloc padding. When direct mapping is on, account data is mapped in its - // own separate memory region which is directly mutated from inside the vm, - // and the serialized buffer includes only the realloc padding. + // This is only set when direct mapping is off (see the relevant comment in + // CallerAccount::from_account_info). serialized_data: &'a mut [u8], // Given the corresponding input AccountInfo::data, vm_data_addr points to // the pointer field and ref_to_len_in_vm points to the length field. vm_data_addr: u64, - ref_to_len_in_vm: &'a mut u64, + ref_to_len_in_vm: VmValue<'b, 'a, u64>, // To be removed once `feature_set::move_serialized_len_ptr_in_cpi` is active everywhere serialized_len_ptr: *mut u64, executable: bool, rent_epoch: u64, } -impl<'a> CallerAccount<'a> { +impl<'a, 'b> CallerAccount<'a, 'b> { // Create a CallerAccount given an AccountInfo. fn from_account_info( invoke_context: &InvokeContext, - memory_mapping: &MemoryMapping, - is_loader_deprecated: bool, + memory_mapping: &'b MemoryMapping<'a>, _vm_addr: u64, account_info: &AccountInfo, - original_data_len: usize, - ) -> Result, Error> { + account_metadata: &SerializedAccountMetadata, + ) -> Result, Error> { + let direct_mapping = invoke_context + .feature_set + .is_active(&feature_set::bpf_account_data_direct_mapping::id()); + + if direct_mapping { + check_account_info_pointer( + invoke_context, + account_info.key as *const _ as u64, + account_metadata.vm_key_addr, + "key", + )?; + check_account_info_pointer( + invoke_context, + account_info.owner as *const _ as u64, + account_metadata.vm_owner_addr, + "owner", + )?; + } + // account_info points to host memory. The addresses used internally are // in vm space so they need to be translated. - let lamports = { // Double translate lamports out of RefCell let ptr = translate_type::( @@ -63,8 +138,17 @@ impl<'a> CallerAccount<'a> { account_info.lamports.as_ptr() as u64, invoke_context.get_check_aligned(), )?; + if direct_mapping { + check_account_info_pointer( + invoke_context, + *ptr, + account_metadata.vm_lamports_addr, + "lamports", + )?; + } translate_type_mut::(memory_mapping, *ptr, invoke_context.get_check_aligned())? }; + let owner = translate_type_mut::( memory_mapping, account_info.owner as *const _ as u64, @@ -78,6 +162,14 @@ impl<'a> CallerAccount<'a> { account_info.data.as_ptr() as *const _ as u64, invoke_context.get_check_aligned(), )?; + if direct_mapping { + check_account_info_pointer( + invoke_context, + data.as_ptr() as u64, + account_metadata.vm_data_addr, + "data", + )?; + } consume_compute_meter( invoke_context, @@ -86,14 +178,30 @@ impl<'a> CallerAccount<'a> { .unwrap_or(u64::MAX), )?; - let translated = translate( - memory_mapping, - AccessType::Store, - (account_info.data.as_ptr() as *const u64 as u64) - .saturating_add(size_of::() as u64), - 8, - )? as *mut u64; - let ref_to_len_in_vm = unsafe { &mut *translated }; + let ref_to_len_in_vm = if direct_mapping { + let vm_addr = (account_info.data.as_ptr() as *const u64 as u64) + .saturating_add(size_of::() as u64); + // In the same vein as the other check_account_info_pointer() checks, we don't lock + // this pointer to a specific address but we don't want it to be inside accounts, or + // callees might be able to write to the pointed memory. + if vm_addr >= ebpf::MM_INPUT_START { + return Err(SyscallError::InvalidPointer.into()); + } + VmValue::VmAddress { + vm_addr, + memory_mapping, + check_aligned: invoke_context.get_check_aligned(), + } + } else { + let translated = translate( + memory_mapping, + AccessType::Store, + (account_info.data.as_ptr() as *const u64 as u64) + .saturating_add(size_of::() as u64), + 8, + )? as *mut u64; + VmValue::Translated(unsafe { &mut *translated }) + }; let serialized_len_ptr = if invoke_context .feature_set .is_active(&feature_set::move_serialized_len_ptr_in_cpi::id()) @@ -110,29 +218,29 @@ impl<'a> CallerAccount<'a> { }; let vm_data_addr = data.as_ptr() as u64; - let bpf_account_data_direct_mapping = invoke_context - .feature_set - .is_active(&feature_set::bpf_account_data_direct_mapping::id()); - let serialized_data = translate_slice_mut::( - memory_mapping, - if bpf_account_data_direct_mapping { - vm_data_addr.saturating_add(original_data_len as u64) - } else { - vm_data_addr - }, - if bpf_account_data_direct_mapping { - if is_loader_deprecated { - 0 - } else { - MAX_PERMITTED_DATA_INCREASE - } - } else { - data.len() - } as u64, - invoke_context.get_check_aligned(), - invoke_context.get_check_size(), - )?; - + let serialized_data = if direct_mapping { + // when direct mapping is enabled, the permissions on the + // realloc region can change during CPI so we must delay + // translating until when we know whether we're going to mutate + // the realloc region or not. Consider this case: + // + // [caller can't write to an account] <- we are here + // [callee grows and assigns account to the caller] + // [caller can now write to the account] + // + // If we always translated the realloc area here, we'd get a + // memory access violation since we can't write to the account + // _yet_, but we will be able to once the caller returns. + &mut [] + } else { + translate_slice_mut::( + memory_mapping, + vm_data_addr, + data.len() as u64, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )? + }; ( serialized_data, vm_data_addr, @@ -144,7 +252,7 @@ impl<'a> CallerAccount<'a> { Ok(CallerAccount { lamports, owner, - original_data_len, + original_data_len: account_metadata.original_data_len, serialized_data, vm_data_addr, ref_to_len_in_vm, @@ -157,15 +265,47 @@ impl<'a> CallerAccount<'a> { // Create a CallerAccount given a SolAccountInfo. fn from_sol_account_info( invoke_context: &InvokeContext, - memory_mapping: &MemoryMapping, - is_loader_deprecated: bool, + memory_mapping: &'b MemoryMapping<'a>, vm_addr: u64, account_info: &SolAccountInfo, - original_data_len: usize, - ) -> Result, Error> { + account_metadata: &SerializedAccountMetadata, + ) -> Result, Error> { + let direct_mapping = invoke_context + .feature_set + .is_active(&feature_set::bpf_account_data_direct_mapping::id()); + + if direct_mapping { + check_account_info_pointer( + invoke_context, + account_info.key_addr, + account_metadata.vm_key_addr, + "key", + )?; + + check_account_info_pointer( + invoke_context, + account_info.owner_addr, + account_metadata.vm_owner_addr, + "owner", + )?; + + check_account_info_pointer( + invoke_context, + account_info.lamports_addr, + account_metadata.vm_lamports_addr, + "lamports", + )?; + + check_account_info_pointer( + invoke_context, + account_info.data_addr, + account_metadata.vm_data_addr, + "data", + )?; + } + // account_info points to host memory. The addresses used internally are // in vm space so they need to be translated. - let lamports = translate_type_mut::( memory_mapping, account_info.lamports_addr, @@ -176,7 +316,6 @@ impl<'a> CallerAccount<'a> { account_info.owner_addr, invoke_context.get_check_aligned(), )?; - let vm_data_addr = account_info.data_addr; consume_compute_meter( invoke_context, @@ -186,28 +325,18 @@ impl<'a> CallerAccount<'a> { .unwrap_or(u64::MAX), )?; - let bpf_account_data_direct_mapping = invoke_context - .feature_set - .is_active(&feature_set::bpf_account_data_direct_mapping::id()); - let serialized_data = translate_slice_mut::( - memory_mapping, - if bpf_account_data_direct_mapping { - vm_data_addr.saturating_add(original_data_len as u64) - } else { - vm_data_addr - }, - if bpf_account_data_direct_mapping { - if is_loader_deprecated { - 0 - } else { - MAX_PERMITTED_DATA_INCREASE as u64 - } - } else { - account_info.data_len - }, - invoke_context.get_check_aligned(), - invoke_context.get_check_size(), - )?; + let serialized_data = if direct_mapping { + // See comment in CallerAccount::from_account_info() + &mut [] + } else { + translate_slice_mut::( + memory_mapping, + account_info.data_addr, + account_info.data_len, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )? + }; // we already have the host addr we want: &mut account_info.data_len. // The account info might be read only in the vm though, so we translate @@ -216,13 +345,28 @@ impl<'a> CallerAccount<'a> { let data_len_vm_addr = vm_addr .saturating_add(&account_info.data_len as *const u64 as u64) .saturating_sub(account_info as *const _ as *const u64 as u64); - let data_len_addr = translate( - memory_mapping, - AccessType::Store, - data_len_vm_addr, - size_of::() as u64, - )?; - let ref_to_len_in_vm = unsafe { &mut *(data_len_addr as *mut u64) }; + + let ref_to_len_in_vm = if direct_mapping { + // In the same vein as the other check_account_info_pointer() checks, we don't lock this + // pointer to a specific address but we don't want it to be inside accounts, or callees + // might be able to write to the pointed memory. + if data_len_vm_addr >= ebpf::MM_INPUT_START { + return Err(SyscallError::InvalidPointer.into()); + } + VmValue::VmAddress { + vm_addr: data_len_vm_addr, + memory_mapping, + check_aligned: invoke_context.get_check_aligned(), + } + } else { + let data_len_addr = translate( + memory_mapping, + AccessType::Store, + data_len_vm_addr, + size_of::() as u64, + )?; + VmValue::Translated(unsafe { &mut *(data_len_addr as *mut u64) }) + }; let ref_of_len_in_input_buffer = (account_info.data_addr as *mut u8 as u64).saturating_sub(8); @@ -242,18 +386,31 @@ impl<'a> CallerAccount<'a> { Ok(CallerAccount { lamports, owner, - original_data_len, + original_data_len: account_metadata.original_data_len, serialized_data, - vm_data_addr, + vm_data_addr: account_info.data_addr, ref_to_len_in_vm, serialized_len_ptr, executable: account_info.executable, rent_epoch: account_info.rent_epoch, }) } + + fn realloc_region( + &self, + memory_mapping: &'b MemoryMapping<'_>, + is_loader_deprecated: bool, + ) -> Result, Error> { + account_realloc_region( + memory_mapping, + self.vm_data_addr, + self.original_data_len, + is_loader_deprecated, + ) + } } -type TranslatedAccounts<'a> = Vec<(IndexOfAccount, Option>)>; +type TranslatedAccounts<'a, 'b> = Vec<(IndexOfAccount, Option>)>; /// Implemented by language specific data structure translators trait SyscallInvokeSigned { @@ -262,15 +419,15 @@ trait SyscallInvokeSigned { memory_mapping: &MemoryMapping, invoke_context: &mut InvokeContext, ) -> Result; - fn translate_accounts<'a>( + fn translate_accounts<'a, 'b>( instruction_accounts: &[InstructionAccount], program_indices: &[IndexOfAccount], account_infos_addr: u64, account_infos_len: u64, is_loader_deprecated: bool, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, invoke_context: &mut InvokeContext, - ) -> Result, Error>; + ) -> Result, Error>; fn translate_signers( program_id: &Pubkey, signers_seeds_addr: u64, @@ -356,15 +513,15 @@ impl SyscallInvokeSigned for SyscallInvokeSignedRust { }) } - fn translate_accounts<'a>( + fn translate_accounts<'a, 'b>( instruction_accounts: &[InstructionAccount], program_indices: &[IndexOfAccount], account_infos_addr: u64, account_infos_len: u64, is_loader_deprecated: bool, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, invoke_context: &mut InvokeContext, - ) -> Result, Error> { + ) -> Result, Error> { let (account_infos, account_info_keys) = translate_account_infos( account_infos_addr, account_infos_len, @@ -590,15 +747,15 @@ impl SyscallInvokeSigned for SyscallInvokeSignedC { }) } - fn translate_accounts<'a>( + fn translate_accounts<'a, 'b>( instruction_accounts: &[InstructionAccount], program_indices: &[IndexOfAccount], account_infos_addr: u64, account_infos_len: u64, is_loader_deprecated: bool, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, invoke_context: &mut InvokeContext, - ) -> Result, Error> { + ) -> Result, Error> { let (account_infos, account_info_keys) = translate_account_infos( account_infos_addr, account_infos_len, @@ -707,7 +864,7 @@ where // Finish translating accounts, build CallerAccount values and update callee // accounts in preparation of executing the callee. -fn translate_and_update_accounts<'a, T, F>( +fn translate_and_update_accounts<'a, 'b, T, F>( instruction_accounts: &[InstructionAccount], program_indices: &[IndexOfAccount], account_info_keys: &[&Pubkey], @@ -715,11 +872,17 @@ fn translate_and_update_accounts<'a, T, F>( account_infos_addr: u64, is_loader_deprecated: bool, invoke_context: &mut InvokeContext, - memory_mapping: &MemoryMapping, + memory_mapping: &'b MemoryMapping<'a>, do_translate: F, -) -> Result, Error> +) -> Result, Error> where - F: Fn(&InvokeContext, &MemoryMapping, bool, u64, &T, usize) -> Result, Error>, + F: Fn( + &InvokeContext, + &'b MemoryMapping<'a>, + u64, + &T, + &SerializedAccountMetadata, + ) -> Result, Error>, { let transaction_context = &invoke_context.transaction_context; let instruction_context = transaction_context.get_current_instruction_context()?; @@ -768,7 +931,7 @@ where } else if let Some(caller_account_index) = account_info_keys.iter().position(|key| *key == account_key) { - let original_data_len = accounts_metadata + let serialized_metadata = accounts_metadata .get(instruction_account.index_in_caller as usize) .ok_or_else(|| { ic_msg!( @@ -777,22 +940,20 @@ where account_key ); Box::new(InstructionError::MissingAccount) - })? - .original_data_len; + })?; // build the CallerAccount corresponding to this account. let caller_account = do_translate( invoke_context, memory_mapping, - is_loader_deprecated, account_infos_addr.saturating_add( caller_account_index.saturating_mul(mem::size_of::()) as u64, ), account_infos .get(caller_account_index) .ok_or(SyscallError::InvalidLength)?, - original_data_len, + serialized_metadata, )?; // before initiating CPI, the caller may have modified the @@ -801,6 +962,7 @@ where // changes. update_callee_account( invoke_context, + memory_mapping, is_loader_deprecated, &caller_account, callee_account, @@ -996,6 +1158,25 @@ fn cpi_common( .feature_set .is_active(&feature_set::bpf_account_data_direct_mapping::id()); + if direct_mapping { + // Update all perms at once before doing account data updates. This + // isn't strictly required as we forbid updates to an account to touch + // other accounts, but since we did have bugs around this in the past, + // it's better to be safe than sorry. + for (index_in_caller, caller_account) in accounts.iter() { + if let Some(caller_account) = caller_account { + let callee_account = instruction_context + .try_borrow_instruction_account(transaction_context, *index_in_caller)?; + update_caller_account_perms( + memory_mapping, + caller_account, + &callee_account, + is_loader_deprecated, + )?; + } + } + } + for (index_in_caller, caller_account) in accounts.iter_mut() { if let Some(caller_account) = caller_account { let mut callee_account = instruction_context @@ -1024,6 +1205,7 @@ fn cpi_common( // changes. fn update_callee_account( invoke_context: &InvokeContext, + memory_mapping: &MemoryMapping, is_loader_deprecated: bool, caller_account: &CallerAccount, mut callee_account: BorrowedAccount<'_>, @@ -1038,25 +1220,33 @@ fn update_callee_account( if direct_mapping { let prev_len = callee_account.get_data().len(); - let post_len = *caller_account.ref_to_len_in_vm as usize; + let post_len = *caller_account.ref_to_len_in_vm.get()? as usize; match callee_account .can_data_be_resized(post_len) .and_then(|_| callee_account.can_data_be_changed()) { Ok(()) => { - callee_account.set_data_length(post_len)?; let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len); - if !is_loader_deprecated && realloc_bytes_used > 0 { + // bpf_loader_deprecated programs don't have a realloc region + if is_loader_deprecated && realloc_bytes_used > 0 { + return Err(InstructionError::InvalidRealloc.into()); + } + callee_account.set_data_length(post_len)?; + if realloc_bytes_used > 0 { + let serialized_data = translate_slice::( + memory_mapping, + caller_account + .vm_data_addr + .saturating_add(caller_account.original_data_len as u64), + realloc_bytes_used as u64, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )?; callee_account .get_data_mut()? .get_mut(caller_account.original_data_len..post_len) .ok_or(SyscallError::InvalidLength)? - .copy_from_slice( - caller_account - .serialized_data - .get(0..realloc_bytes_used) - .ok_or(SyscallError::InvalidLength)?, - ); + .copy_from_slice(serialized_data); } } Err(err) if prev_len != post_len => { @@ -1114,6 +1304,43 @@ fn update_callee_account( Ok(()) } +fn update_caller_account_perms( + memory_mapping: &MemoryMapping, + caller_account: &CallerAccount, + callee_account: &BorrowedAccount<'_>, + is_loader_deprecated: bool, +) -> Result<(), Error> { + let CallerAccount { + original_data_len, + vm_data_addr, + .. + } = caller_account; + + let data_region = account_data_region(memory_mapping, *vm_data_addr, *original_data_len)?; + if let Some(region) = data_region { + region + .state + .set(account_data_region_memory_state(callee_account)); + } + let realloc_region = account_realloc_region( + memory_mapping, + *vm_data_addr, + *original_data_len, + is_loader_deprecated, + )?; + if let Some(region) = realloc_region { + region + .state + .set(if callee_account.can_data_be_changed().is_ok() { + MemoryState::Writable + } else { + MemoryState::Readable + }); + } + + Ok(()) +} + // Update the given account after executing CPI. // // caller_account and callee_account describe to the same account. At CPI exit @@ -1133,29 +1360,39 @@ fn update_caller_account( *caller_account.lamports = callee_account.get_lamports(); *caller_account.owner = *callee_account.get_owner(); - if direct_mapping && caller_account.original_data_len > 0 { - // Since each instruction account is directly mapped in a memory region - // with a *fixed* length, upon returning from CPI we must ensure that the - // current capacity is at least the original length (what is mapped in - // memory), so that the account's memory region never points to an - // invalid address. - let min_capacity = caller_account.original_data_len; - if callee_account.capacity() < min_capacity { - callee_account.reserve(min_capacity.saturating_sub(callee_account.capacity()))? - } + let mut zero_all_mapped_spare_capacity = false; + if direct_mapping { + if let Some(region) = account_data_region( + memory_mapping, + caller_account.vm_data_addr, + caller_account.original_data_len, + )? { + // Since each instruction account is directly mapped in a memory region + // with a *fixed* length, upon returning from CPI we must ensure that the + // current capacity is at least the original length (what is mapped in + // memory), so that the account's memory region never points to an + // invalid address. + let min_capacity = caller_account.original_data_len; + if callee_account.capacity() < min_capacity { + callee_account.reserve(min_capacity.saturating_sub(callee_account.capacity()))?; + zero_all_mapped_spare_capacity = true; + } - // If an account's data pointer has changed - because of CoW or because - // of using AccountSharedData directly (deprecated) - we must update the - // corresponding MemoryRegion in the caller's address space. Address - // spaces are fixed so we don't need to update the MemoryRegion's length. - let region = memory_mapping.region(AccessType::Load, caller_account.vm_data_addr)?; - let callee_ptr = callee_account.get_data().as_ptr() as u64; - if region.host_addr.get() != callee_ptr { - region.host_addr.set(callee_ptr); + // If an account's data pointer has changed - because of CoW, reserve() as called above + // or because of using AccountSharedData directly (deprecated) - we must update the + // corresponding MemoryRegion in the caller's address space. Address spaces are fixed so + // we don't need to update the MemoryRegion's length. + let callee_ptr = callee_account.get_data().as_ptr() as u64; + if region.host_addr.get() != callee_ptr { + region.host_addr.set(callee_ptr); + zero_all_mapped_spare_capacity = true; + } } } - let prev_len = *caller_account.ref_to_len_in_vm as usize; + + let prev_len = *caller_account.ref_to_len_in_vm.get()? as usize; let post_len = callee_account.get_data().len(); + let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len); if prev_len != post_len { let max_increase = if direct_mapping && !invoke_context.get_check_aligned() { 0 @@ -1173,13 +1410,33 @@ fn update_caller_account( ); return Err(Box::new(InstructionError::InvalidRealloc)); } + + // If the account has been shrunk, we're going to zero the unused memory + // *that was previously used*. if post_len < prev_len { if direct_mapping { - if post_len < caller_account.original_data_len { - // zero the spare capacity in the account data. We only need - // to zero up to the original data length, everything else - // is not accessible from the vm anyway. - let spare_len = caller_account.original_data_len.saturating_sub(post_len); + // We have two separate regions to zero out: the account data + // and the realloc region. + // + // Here we zero the account data region. + let spare_len = if zero_all_mapped_spare_capacity { + // In the unlikely case where the account data vector has + // changed - which can happen during CoW - we zero the whole + // extra capacity up to the original data length. + // + // The extra capacity up to original data length is + // accessible from the vm and since it's uninitialized + // memory, it could be a source of non determinism. + caller_account.original_data_len + } else { + // If the allocation has not changed, we only zero the + // difference between the previous and current lengths. The + // rest of the memory contains whatever it contained before, + // which is deterministic. + prev_len + } + .saturating_sub(post_len); + if spare_len > 0 { let dst = callee_account .spare_data_capacity_mut()? .get_mut(..spare_len) @@ -1189,14 +1446,57 @@ fn update_caller_account( unsafe { ptr::write_bytes(dst, 0, spare_len) }; } - // zero the spare capacity in the realloc padding - let spare_realloc = unsafe { - slice::from_raw_parts_mut( - caller_account.serialized_data.as_mut_ptr(), - prev_len.saturating_sub(caller_account.original_data_len), - ) - }; - spare_realloc.fill(0); + // Here we zero the realloc region. + // + // This is done for compatibility but really only necessary for + // the fringe case of a program calling itself, see + // TEST_CPI_ACCOUNT_UPDATE_CALLER_GROWS_CALLEE_SHRINKS. + // + // Zeroing the realloc region isn't necessary in the normal + // invoke case because consider the following scenario: + // + // 1. Caller grows an account (prev_len > original_data_len) + // 2. Caller assigns the account to the callee (needed for 3 to + // work) + // 3. Callee shrinks the account (post_len < prev_len) + // + // In order for the caller to assign the account to the callee, + // the caller _must_ either set the account length to zero, + // therefore making prev_len > original_data_len impossible, + // or it must zero the account data, therefore making the + // zeroing we do here redundant. + if prev_len > caller_account.original_data_len { + // If we get here and prev_len > original_data_len, then + // we've already returned InvalidRealloc for the + // bpf_loader_deprecated case. + debug_assert!(!is_loader_deprecated); + + // Temporarily configure the realloc region as writable then set it back to + // whatever state it had. + let realloc_region = caller_account + .realloc_region(memory_mapping, is_loader_deprecated)? + .unwrap(); // unwrapping here is fine, we already asserted !is_loader_deprecated + let original_state = realloc_region.state.replace(MemoryState::Writable); + defer! { + realloc_region.state.set(original_state); + }; + + // We need to zero the unused space in the realloc region, starting after the + // last byte of the new data which might be > original_data_len. + let dirty_realloc_start = caller_account.original_data_len.max(post_len); + // and we want to zero up to the old length + let dirty_realloc_len = prev_len.saturating_sub(dirty_realloc_start); + let serialized_data = translate_slice_mut::( + memory_mapping, + caller_account + .vm_data_addr + .saturating_add(dirty_realloc_start as u64), + dirty_realloc_len as u64, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )?; + serialized_data.fill(0); + } } else { caller_account .serialized_data @@ -1206,8 +1506,8 @@ fn update_caller_account( } } - // with direct_mapping on, serialized_data is fixed and holds the - // realloc padding + // when direct mapping is enabled we don't cache the serialized data in + // caller_account.serialized_data. See CallerAccount::from_account_info. if !direct_mapping { caller_account.serialized_data = translate_slice_mut::( memory_mapping, @@ -1218,7 +1518,7 @@ fn update_caller_account( )?; } // this is the len field in the AccountInfo::data slice - *caller_account.ref_to_len_in_vm = post_len as u64; + *caller_account.ref_to_len_in_vm.get_mut()? = post_len as u64; // this is the len field in the serialized parameters if invoke_context @@ -1239,7 +1539,6 @@ fn update_caller_account( } } } - let realloc_bytes_used = post_len.saturating_sub(caller_account.original_data_len); if !direct_mapping { let to_slice = &mut caller_account.serialized_data; let from_slice = callee_account @@ -1250,21 +1549,88 @@ fn update_caller_account( return Err(Box::new(InstructionError::AccountDataTooSmall)); } to_slice.copy_from_slice(from_slice); - } else if !is_loader_deprecated && realloc_bytes_used > 0 { - let to_slice = caller_account - .serialized_data - .get_mut(0..realloc_bytes_used) - .ok_or(SyscallError::InvalidLength)?; + } else if realloc_bytes_used > 0 { + // In the is_loader_deprecated case, we must have failed with + // InvalidRealloc by now. + debug_assert!(!is_loader_deprecated); + + let to_slice = { + // If a callee reallocs an account, we write into the caller's + // realloc region regardless of whether the caller has write + // permissions to the account or not. If the callee has been able to + // make changes, it means they had permissions to do so, and here + // we're just going to reflect those changes to the caller's frame. + // + // Therefore we temporarily configure the realloc region as writable + // then set it back to whatever state it had. + let realloc_region = caller_account + .realloc_region(memory_mapping, is_loader_deprecated)? + .unwrap(); // unwrapping here is fine, we asserted !is_loader_deprecated + let original_state = realloc_region.state.replace(MemoryState::Writable); + defer! { + realloc_region.state.set(original_state); + }; + + translate_slice_mut::( + memory_mapping, + caller_account + .vm_data_addr + .saturating_add(caller_account.original_data_len as u64), + realloc_bytes_used as u64, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + )? + }; let from_slice = callee_account .get_data() .get(caller_account.original_data_len..post_len) .ok_or(SyscallError::InvalidLength)?; + if to_slice.len() != from_slice.len() { + return Err(Box::new(InstructionError::AccountDataTooSmall)); + } to_slice.copy_from_slice(from_slice); } Ok(()) } +fn account_data_region<'a>( + memory_mapping: &'a MemoryMapping<'_>, + vm_data_addr: u64, + original_data_len: usize, +) -> Result, Error> { + if original_data_len == 0 { + return Ok(None); + } + + // We can trust vm_data_addr to point to the correct region because we + // enforce that in CallerAccount::from_(sol_)account_info. + let data_region = memory_mapping.region(AccessType::Load, vm_data_addr)?; + // vm_data_addr must always point to the beginning of the region + debug_assert_eq!(data_region.vm_addr, vm_data_addr); + Ok(Some(data_region)) +} + +fn account_realloc_region<'a>( + memory_mapping: &'a MemoryMapping<'_>, + vm_data_addr: u64, + original_data_len: usize, + is_loader_deprecated: bool, +) -> Result, Error> { + if is_loader_deprecated { + return Ok(None); + } + + let realloc_vm_addr = vm_data_addr.saturating_add(original_data_len as u64); + let realloc_region = memory_mapping.region(AccessType::Load, realloc_vm_addr)?; + debug_assert_eq!(realloc_region.vm_addr, realloc_vm_addr); + debug_assert!((MAX_PERMITTED_DATA_INCREASE + ..MAX_PERMITTED_DATA_INCREASE.saturating_add(BPF_ALIGN_OF_U128)) + .contains(&(realloc_region.len as usize))); + debug_assert!(!matches!(realloc_region.state.get(), MemoryState::Cow(_))); + Ok(Some(realloc_region)) +} + #[allow(clippy::indexing_slicing)] #[allow(clippy::integer_arithmetic)] #[cfg(test)] @@ -1441,7 +1807,8 @@ mod tests { let key = Pubkey::new_unique(); let vm_addr = MM_INPUT_START; - let (_mem, region) = MockAccountInfo::new(key, &account).into_region(vm_addr); + let (_mem, region, account_metadata) = + MockAccountInfo::new(key, &account).into_region(vm_addr); let config = Config { aligned_memory_mapping: false, @@ -1454,17 +1821,16 @@ mod tests { let caller_account = CallerAccount::from_account_info( &invoke_context, &memory_mapping, - false, vm_addr, account_info, - account.data().len(), + &account_metadata, ) .unwrap(); assert_eq!(*caller_account.lamports, account.lamports()); assert_eq!(caller_account.owner, account.owner()); assert_eq!(caller_account.original_data_len, account.data().len()); assert_eq!( - *caller_account.ref_to_len_in_vm as usize, + *caller_account.ref_to_len_in_vm.get().unwrap() as usize, account.data().len() ); assert_eq!(caller_account.serialized_data, account.data()); @@ -1592,7 +1958,10 @@ mod tests { .unwrap(); let data_len = callee_account.get_data().len(); - assert_eq!(data_len, *caller_account.ref_to_len_in_vm as usize); + assert_eq!( + data_len, + *caller_account.ref_to_len_in_vm.get().unwrap() as usize + ); assert_eq!(data_len, serialized_len()); assert_eq!(data_len, caller_account.serialized_data.len()); assert_eq!( @@ -1737,12 +2106,23 @@ mod tests { let data_len = callee_account.get_data().len(); // the account info length must get updated - assert_eq!(data_len, *caller_account.ref_to_len_in_vm as usize); + assert_eq!( + data_len, + *caller_account.ref_to_len_in_vm.get().unwrap() as usize + ); // the length slot in the serialization parameters must be updated assert_eq!(data_len, serialized_len()); - // with direct mapping on, serialized_data contains the realloc padding - let realloc_area = &caller_account.serialized_data; + let realloc_area = translate_slice::( + &memory_mapping, + caller_account + .vm_data_addr + .saturating_add(caller_account.original_data_len as u64), + MAX_PERMITTED_DATA_INCREASE as u64, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + ) + .unwrap(); if data_len < original_data_len { // if an account gets resized below its original data length, @@ -1918,6 +2298,17 @@ mod tests { false, ); + let config = Config { + aligned_memory_mapping: false, + ..Config::default() + }; + let memory_mapping = MemoryMapping::new( + mock_caller_account.regions.split_off(0), + &config, + &SBPFVersion::V2, + ) + .unwrap(); + let caller_account = mock_caller_account.caller_account(); let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -1927,6 +2318,7 @@ mod tests { update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -1962,6 +2354,17 @@ mod tests { false, ); + let config = Config { + aligned_memory_mapping: false, + ..Config::default() + }; + let memory_mapping = MemoryMapping::new( + mock_caller_account.regions.split_off(0), + &config, + &SBPFVersion::V2, + ) + .unwrap(); + let mut caller_account = mock_caller_account.caller_account(); let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -1971,6 +2374,7 @@ mod tests { update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -1984,11 +2388,12 @@ mod tests { // close the account let mut data = Vec::new(); caller_account.serialized_data = &mut data; - *caller_account.ref_to_len_in_vm = 0; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = 0; let mut owner = system_program::id(); caller_account.owner = &mut owner; update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2022,6 +2427,17 @@ mod tests { false, ); + let config = Config { + aligned_memory_mapping: false, + ..Config::default() + }; + let memory_mapping = MemoryMapping::new( + mock_caller_account.regions.split_off(0), + &config, + &SBPFVersion::V2, + ) + .unwrap(); + let mut caller_account = mock_caller_account.caller_account(); let callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2030,6 +2446,7 @@ mod tests { assert!(matches!( update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2040,13 +2457,14 @@ mod tests { // without direct mapping let mut data = b"foobarbaz".to_vec(); - *caller_account.ref_to_len_in_vm = data.len() as u64; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = data.len() as u64; caller_account.serialized_data = &mut data; let callee_account = borrow_instruction_account!(invoke_context, 0); assert!(matches!( update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2057,13 +2475,14 @@ mod tests { // with direct mapping let mut data = b"baz".to_vec(); - *caller_account.ref_to_len_in_vm = 9; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = 9; caller_account.serialized_data = &mut data; let callee_account = borrow_instruction_account!(invoke_context, 0); assert!(matches!( update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2096,6 +2515,17 @@ mod tests { true, ); + let config = Config { + aligned_memory_mapping: false, + ..Config::default() + }; + let memory_mapping = MemoryMapping::new( + mock_caller_account.regions.split_off(0), + &config, + &SBPFVersion::V2, + ) + .unwrap(); + let mut caller_account = mock_caller_account.caller_account(); let mut callee_account = borrow_instruction_account!(invoke_context, 0); @@ -2105,17 +2535,27 @@ mod tests { // with enough padding to hold the realloc padding callee_account.get_data_mut().unwrap(); - let mut data = b"baz".to_vec(); - caller_account.serialized_data = &mut data; + let serialized_data = translate_slice_mut::( + &memory_mapping, + caller_account + .vm_data_addr + .saturating_add(caller_account.original_data_len as u64), + 3, + invoke_context.get_check_aligned(), + invoke_context.get_check_size(), + ) + .unwrap(); + serialized_data.copy_from_slice(b"baz"); for (len, expected) in [ (9, b"foobarbaz".to_vec()), // > original_data_len, copies from realloc region (6, b"foobar".to_vec()), // == original_data_len, truncates (3, b"foo".to_vec()), // < original_data_len, truncates ] { - *caller_account.ref_to_len_in_vm = len as u64; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = len as u64; update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2129,11 +2569,12 @@ mod tests { // close the account let mut data = Vec::new(); caller_account.serialized_data = &mut data; - *caller_account.ref_to_len_in_vm = 0; + *caller_account.ref_to_len_in_vm.get_mut().unwrap() = 0; let mut owner = system_program::id(); caller_account.owner = &mut owner; update_callee_account( &invoke_context, + &memory_mapping, false, &caller_account, callee_account, @@ -2152,6 +2593,16 @@ mod tests { let key = transaction_accounts[1].0; let original_data_len = account.data().len(); + let vm_addr = MM_INPUT_START; + let (_mem, region, account_metadata) = + MockAccountInfo::new(key, &account).into_region(vm_addr); + + let config = Config { + aligned_memory_mapping: false, + ..Config::default() + }; + let memory_mapping = MemoryMapping::new(vec![region], &config, &SBPFVersion::V2).unwrap(); + mock_invoke_context!( invoke_context, transaction_context, @@ -2160,21 +2611,8 @@ mod tests { &[0], &[1, 1] ); - mock_create_vm!( - _vm, - Vec::new(), - vec![SerializedAccountMetadata { original_data_len }], - &mut invoke_context - ); - - let vm_addr = MM_INPUT_START; - let (_mem, region) = MockAccountInfo::new(key, &account).into_region(vm_addr); - let config = Config { - aligned_memory_mapping: false, - ..Config::default() - }; - let memory_mapping = MemoryMapping::new(vec![region], &config, &SBPFVersion::V2).unwrap(); + mock_create_vm!(_vm, Vec::new(), vec![account_metadata], &mut invoke_context); let accounts = SyscallInvokeSignedRust::translate_accounts( &[ @@ -2216,6 +2654,7 @@ mod tests { data: Vec, len: u64, regions: Vec, + direct_mapping: bool, } impl MockCallerAccount { @@ -2275,6 +2714,7 @@ mod tests { data: d, len: data.len() as u64, regions, + direct_mapping, } } @@ -2288,15 +2728,19 @@ mod tests { } } - fn caller_account(&mut self) -> CallerAccount<'_> { - let data = &mut self.data[mem::size_of::()..]; + fn caller_account(&mut self) -> CallerAccount<'_, '_> { + let data = if self.direct_mapping { + &mut [] + } else { + &mut self.data[mem::size_of::()..] + }; CallerAccount { lamports: &mut self.lamports, owner: &mut self.owner, original_data_len: self.len as usize, serialized_data: data, vm_data_addr: self.vm_addr + mem::size_of::() as u64, - ref_to_len_in_vm: &mut self.len, + ref_to_len_in_vm: VmValue::Translated(&mut self.len), serialized_len_ptr: std::ptr::null_mut(), executable: false, rent_epoch: 0, @@ -2471,7 +2915,7 @@ mod tests { } } - fn into_region(self, vm_addr: u64) -> (Vec, MemoryRegion) { + fn into_region(self, vm_addr: u64) -> (Vec, MemoryRegion, SerializedAccountMetadata) { let size = mem::size_of::() + mem::size_of::() * 2 + mem::size_of::>>() @@ -2532,7 +2976,17 @@ mod tests { } let region = MemoryRegion::new_writable(data.as_mut_slice(), vm_addr as u64); - (data, region) + ( + data, + region, + SerializedAccountMetadata { + original_data_len: self.data.len(), + vm_key_addr: key_addr as u64, + vm_lamports_addr: lamports_addr as u64, + vm_owner_addr: owner_addr as u64, + vm_data_addr: data_addr as u64, + }, + ) } } diff --git a/programs/bpf_loader/src/syscalls/mod.rs b/programs/bpf_loader/src/syscalls/mod.rs index 9190fd70e2aae4..d8e39961291501 100644 --- a/programs/bpf_loader/src/syscalls/mod.rs +++ b/programs/bpf_loader/src/syscalls/mod.rs @@ -123,6 +123,8 @@ pub enum SyscallError { }, #[error("InvalidAttribute")] InvalidAttribute, + #[error("Invalid pointer")] + InvalidPointer, } type Error = Box; diff --git a/programs/sbf/Cargo.lock b/programs/sbf/Cargo.lock index 1b32410a4fc776..4aa3d33d156643 100644 --- a/programs/sbf/Cargo.lock +++ b/programs/sbf/Cargo.lock @@ -4613,6 +4613,7 @@ dependencies = [ "libsecp256k1 0.6.0", "log", "rand 0.8.5", + "scopeguard", "solana-measure", "solana-program-runtime", "solana-sdk", diff --git a/sdk/src/transaction_context.rs b/sdk/src/transaction_context.rs index 39132524495d3e..cdfb162fc475a1 100644 --- a/sdk/src/transaction_context.rs +++ b/sdk/src/transaction_context.rs @@ -1000,8 +1000,8 @@ impl<'a> BorrowedAccount<'a> { // transaction reallocs, we don't have to copy the whole account data a // second time to fullfill the realloc. // - // NOTE: The account memory region CoW code in Serializer::push_account_region() implements - // the same logic and must be kept in sync. + // NOTE: The account memory region CoW code in bpf_loader::create_vm() implements the same + // logic and must be kept in sync. if self.account.is_shared() { self.account.reserve(MAX_PERMITTED_DATA_INCREASE); }