diff --git a/Cargo.lock b/Cargo.lock index b171214e65..d0bf159c25 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 3506671dad..a105048ac3 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 e7123ae0cb..411d9f3654 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 99d3f583c9..57b77559f2 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 768ba79211..1650b845a9 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 3a690ec360..e18052acc0 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 9190fd70e2..d8e3996129 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 1b32410a4f..4aa3d33d15 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 3913252449..cdfb162fc4 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); }